Summary:	DAHLIN-00005: [patch] zt_specchan_open race because ZT_FLAG_OPEN testing and setting is not atomic
Reporter:	matti (matti)	Labels:
Date Opened:	2007-03-26 07:55:02	Date Closed:	2010-10-20 07:23:21
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) clear_before_close.diff ( 1) clear_check.diff ( 2) diff ( 3) diff2360 ( 4) diff2384 ( 5) diffRevision2360 ( 6) diffRevision2384 ( 7) prevent_open_race.diff
Description:	Function zt_specchan_open in file zaptel.c can have a race condition because ZT_FLAG_OPEN testing and setting is not atomic. The function should check and set the flag holding the spin lock continuously in and between the following lines: if (chans[unit]->flags & ZT_FLAG_OPEN) chans[unit]->flags |= ZT_FLAG_OPEN;
Comments:	By: Serge Vecher (serge-v) 2007-03-30 09:22:09 don't you have a disclaimer on file? By: matti (matti) 2007-03-31 04:16:48 I have a disclaimer on file. By: matti (matti) 2007-04-02 02:58:35 zt_specchan_open races also with zt_specchan_release, zt_net_open, zt_net_stop and ioctls. I will attach a patch against Revision 2360. By: matti (matti) 2007-04-02 06:16:14 Function zt_chan_ioctl case ZT_HDLCPPP can race with itself. To fix that, I will attach a new patch to Revision 2360. By: matti (matti) 2007-04-05 01:39:32 My patch still had a race in function zt_chan_ioctl case ZT_HDLCPPP. To fix that, I will attach a new patch to Revision 2384. By: matti (matti) 2007-08-09 02:34:10 Functions zt_specchan_open and zt_specchan_release have races also because they call function close_channel and access chans[unit] without holding a lock. By: Mark Spencer (markster) 2008-01-11 14:09:35.000-0600 Why don't we just use a test_and_set function here...  I think requiring GFP_ATOMIC on resizing buffers is a really bad idea because it happens frequently and potentially with many channels at the same time. By: jmls (jmls) 2008-02-06 04:29:50.000-0600 bump - any further news on this ? By: Matthew Fredrickson (mattf) 2008-04-05 18:04:23 I think I agree with Mark.  Many of these issues would be much simpler if we used the atomic test_and_set and test_and_clear_bit routines.  I'm currently auditing a few of these code paths and will add some patches which use these routines to fix this issue.  I'm starting with the open/release routines.  An initial version of my patch for these routines is about to be attached, Matti, if you would like to see how I am doing it and critique. By: Digium Subversion (svnbot) 2008-04-19 16:48:49 Repository: zaptel Revision: 4183 _U  branches/1.4/ U   branches/1.4/kernel/zaptel-base.c U   branches/1.4/kernel/zaptel.h ------------------------------------------------------------------------ r4183 | mattf | 2008-04-19 16:48:47 -0500 (Sat, 19 Apr 2008) | 1 line Partial fix for DAHLIN-5.  Fixes potential race condition in open and close routines for zaptel devices ------------------------------------------------------------------------ http://svn.digium.com/view/zaptel?view=rev&revision=4183 By: Matthew Fredrickson (mattf) 2008-04-19 16:53:45 Initial problem reported fixed.  Further discussion moved to linked issue. By: Digium Subversion (svnbot) 2008-05-02 12:22:39 Repository: zaptel Revision: 4220 _U  team/mattf/zaptel-1.4-stackcleanup/ U   team/mattf/zaptel-1.4-stackcleanup/Makefile U   team/mattf/zaptel-1.4-stackcleanup/README A   team/mattf/zaptel-1.4-stackcleanup/build_tools/zaptel_svn_tarball U   team/mattf/zaptel-1.4-stackcleanup/kernel/pciradio.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/wctdm.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/.version U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/Kbuild U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/utils/zconf/Zaptel/Config/Defaults.pm U   team/mattf/zaptel-1.4-stackcleanup/kernel/zaptel-base.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/zaptel.h U   team/mattf/zaptel-1.4-stackcleanup/kernel/ztdynamic.c U   team/mattf/zaptel-1.4-stackcleanup/zaptel.init U   team/mattf/zaptel-1.4-stackcleanup/ztcfg.c ------------------------------------------------------------------------ r4220 | mattf | 2008-05-02 12:22:37 -0500 (Fri, 02 May 2008) | 143 lines Merged revisions 4165-4170,4173,4175-4176,4183-4185,4187,4190-4196,4213,4217 via svnmerge from https://origsvn.digium.com/svn/zaptel/branches/1.4 ........ r4165 | tzafrir | 2008-04-10 11:15:23 -0500 (Thu, 10 Apr 2008) | 5 lines Officially bumping xpp version (xpp/.version) to 5566 . Merged revisions 4164 via svnmerge from http://svn.digium.com/svn/zaptel/branches/1.2 ........ r4166 | tzafrir | 2008-04-10 13:11:51 -0500 (Thu, 10 Apr 2008) | 2 lines zaptel_svn_tarball - get a tarball fron an SVN snapshot. ........ r4167 | sruffell | 2008-04-10 16:01:59 -0500 (Thu, 10 Apr 2008) | 2 lines Fixed typo in makefile which was preventing make from installing the udev rules. ........ r4168 | dbailey | 2008-04-10 17:25:30 -0500 (Thu, 10 Apr 2008) | 3 lines Correct problem where drivers implementing echocan_with_params was not getting called to disable the echo canceller ........ r4169 | mattf | 2008-04-11 16:16:47 -0500 (Fri, 11 Apr 2008) | 1 line Make sure that we disable the echo canceller on the old style echocan() function even if parameters are passed in. ........ r4170 | mattf | 2008-04-11 16:17:55 -0500 (Fri, 11 Apr 2008) | 1 line Take out part of a commit to the wct4xxp driver that wasn't supposed to go in ........ r4173 | kpfleming | 2008-04-17 07:20:26 -0500 (Thu, 17 Apr 2008) | 6 lines when processing ZT_SET_DIALPARAMS, don't return an error for out-of-range tone durations, just ignore them (so that applications that don't initialize the entire zt_dialparams structure won't fail to set the ones they do initialize) (closes issue ASTERISK-11861) Reported by: fnordian Tested by: fnordian ........ r4175 | tzafrir | 2008-04-17 09:27:50 -0500 (Thu, 17 Apr 2008) | 5 lines Avoid a double-free in ztdynamic.c ztdynamic_chanfree.diff from bug ZAP-308 . Merged revisions 4174 via svnmerge from http://svn.digium.com/svn/zaptel/branches/1.2 ........ r4176 | jdixon | 2008-04-18 10:19:00 -0500 (Fri, 18 Apr 2008) | 2 lines Fixed monitoring of zap pseudo channels ........ r4183 | mattf | 2008-04-19 16:54:03 -0500 (Sat, 19 Apr 2008) | 1 line Partial fix for DAHLIN-5.  Fixes potential race condition in open and close routines for zaptel devices ........ r4184 | mattf | 2008-04-19 16:56:06 -0500 (Sat, 19 Apr 2008) | 1 line Remove unintentional svn property addition ........ r4185 | mattf | 2008-04-19 17:05:42 -0500 (Sat, 19 Apr 2008) | 1 line Fix compiler from complaining ........ r4187 | tzafrir | 2008-04-21 16:27:59 -0500 (Mon, 21 Apr 2008) | 4 lines Zaptel::Config::Default - don't die if no config file. (Accidentally left out of the merge from 1.2) ........ r4190 | jdixon | 2008-04-22 17:07:08 -0500 (Tue, 22 Apr 2008) | 2 lines Fixed problem preventing multiple channels from using multiple CTCSS ........ r4191 | jdixon | 2008-04-22 23:36:22 -0500 (Tue, 22 Apr 2008) | 2 lines Updated to latest code, fixed bug in serial i/o ........ r4192 | tzafrir | 2008-04-23 04:48:56 -0500 (Wed, 23 Apr 2008) | 2 lines Bumped into yet another incompatibility. ........ r4193 | tzafrir | 2008-04-23 14:20:53 -0500 (Wed, 23 Apr 2008) | 10 lines A test for a specific #define in zconfig.h was done by invoking cpp . Sadly the change of CFLAGS handling in kernel 2.6.24 meant that it is no longer as easy to invoke cpp on our own. Impact: On kernel >= 2.6.24, xpd_bri never got built, even if the bri_dchan patch was applied. Fix: use a simpler grep instead. Hopefully noone passes it through other means. This should hopfully fix issue ZAP-312 . ........ r4194 | sruffell | 2008-04-24 13:17:12 -0500 (Thu, 24 Apr 2008) | 5 lines Fixes a regression in versions 1.2.25 and 1.4.19 identified by korihor where the wctdm driver was no longer properly recognizing polarity reversals. (closes issue ZAP-309) ........ r4195 | qwell | 2008-04-25 13:10:06 -0500 (Fri, 25 Apr 2008) | 1 line Based on a comment made by moy in #asterisk-dev, and the surrounding code in these functions, I believe that this is what was actually intended in r4130. ........ r4196 | qwell | 2008-04-25 15:47:07 -0500 (Fri, 25 Apr 2008) | 7 lines Since MF R2 tones are generated continuously, we need to not set the dialing status. (closes issue ZAP-317) Reported by: moy Patches:      zaptel-mf-dialing-rev4195.patch uploaded by moy (license 222) ........ r4213 | twilson | 2008-05-01 11:46:44 -0500 (Thu, 01 May 2008) | 2 lines Allow $(ARCH) to be overridden and pass on through $(KMAKE) ........ r4217 | qwell | 2008-05-01 14:37:07 -0500 (Thu, 01 May 2008) | 7 lines Clarify a message.  Some hardware doesn't provide timing, so this could be confusing. (closes issue ZAP-313) Reported by: panderson Patches:      zaptel.init.patch uploaded by panderson (license 469) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/zaptel?view=rev&revision=4220 By: matti (matti) 2008-05-04 11:23:41 zt_specchan_open still has a race with at least zt_ctl_ioctl case ZT_CHANCONFIG: setting ZT_FLAG_NETDEV. By: matti (matti) 2008-05-08 01:18:20 At least the current Linux does not have a race between zt_specchan_open and zt_ctl_ioctl case ZT_CHANCONFIG: setting ZT_FLAG_NETDEV because the big kernel lock is taken in both cases. By: matti (matti) 2008-05-16 13:27:40 Actually the big kernel lock does not prevent the race because the threads can sleep. By: Tzafrir Cohen (tzafrir) 2008-06-03 15:13:53 As of r4813 there seems to be a bad regression with disconnecting an Astribank device while a channel is in use: 1. load modules; ztcfg 2. start asterisk 3. rmmod xpp_usb (or disconnect device from USB) 4. stop asterisk. (4) causes a panic at zt_specchan_release+0x74/0x7d , called from the process Asterisk. By: Tzafrir Cohen (tzafrir) 2008-06-04 02:51:47 Uploaded clear_before_close.diff: a workaround for the panic mentioned in my previous comment. Further details: The close() span method decreases a reference count for the ammount of (zaptel) channels that are held open by userspace. If we want to disconnect the device (e.g: the USB device was unplugged) we remove the USB parts and leave a stub connected. That stub waits for the reference count of all the spans of this device to get to 0. When this happens, it cleans up everything and also deregisters all the spans of the device. Thus in this case the last close() for a channel in the device triggered removing it and invalidating all of its channels. And the channel probably got removed even before there was a chance to set its flags in the last line. By: Tzafrir Cohen (tzafrir) 2008-06-05 12:52:25 ok, just check that we don't look at an invalid channel after the close: clear_check.diff By: Tzafrir Cohen (tzafrir) 2008-06-05 15:06:19 Commited into http://svn.digium.com/view/zaptel?view=rev&rev=4348 . Now back to the original topic... By: Digium Subversion (svnbot) 2008-06-07 13:59:53 Repository: dahdi Revision: 4183 _U  branches/1.4/ U   branches/1.4/kernel/zaptel-base.c U   branches/1.4/kernel/zaptel.h ------------------------------------------------------------------------ r4183 | mattf | 2008-06-07 13:59:53 -0500 (Sat, 07 Jun 2008) | 1 line Partial fix for DAHLIN-5.  Fixes potential race condition in open and close routines for zaptel devices ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=4183 By: Digium Subversion (svnbot) 2008-06-07 14:00:32 Repository: dahdi Revision: 4220 _U  team/mattf/zaptel-1.4-stackcleanup/ U   team/mattf/zaptel-1.4-stackcleanup/Makefile U   team/mattf/zaptel-1.4-stackcleanup/README A   team/mattf/zaptel-1.4-stackcleanup/build_tools/zaptel_svn_tarball U   team/mattf/zaptel-1.4-stackcleanup/kernel/pciradio.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/wctdm.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/.version U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/Kbuild U   team/mattf/zaptel-1.4-stackcleanup/kernel/xpp/utils/zconf/Zaptel/Config/Defaults.pm U   team/mattf/zaptel-1.4-stackcleanup/kernel/zaptel-base.c U   team/mattf/zaptel-1.4-stackcleanup/kernel/zaptel.h U   team/mattf/zaptel-1.4-stackcleanup/kernel/ztdynamic.c U   team/mattf/zaptel-1.4-stackcleanup/zaptel.init U   team/mattf/zaptel-1.4-stackcleanup/ztcfg.c ------------------------------------------------------------------------ r4220 | mattf | 2008-06-07 14:00:30 -0500 (Sat, 07 Jun 2008) | 143 lines Merged revisions 4165-4170,4173,4175-4176,4183-4185,4187,4190-4196,4213,4217 via svnmerge from https://origsvn.digium.com/svn/zaptel/branches/1.4 ........ r4165 | tzafrir | 2008-04-10 11:15:23 -0500 (Thu, 10 Apr 2008) | 5 lines Officially bumping xpp version (xpp/.version) to 5566 . Merged revisions 4164 via svnmerge from http://svn.digium.com/svn/zaptel/branches/1.2 ........ r4166 | tzafrir | 2008-04-10 13:11:51 -0500 (Thu, 10 Apr 2008) | 2 lines zaptel_svn_tarball - get a tarball fron an SVN snapshot. ........ r4167 | sruffell | 2008-04-10 16:01:59 -0500 (Thu, 10 Apr 2008) | 2 lines Fixed typo in makefile which was preventing make from installing the udev rules. ........ r4168 | dbailey | 2008-04-10 17:25:30 -0500 (Thu, 10 Apr 2008) | 3 lines Correct problem where drivers implementing echocan_with_params was not getting called to disable the echo canceller ........ r4169 | mattf | 2008-04-11 16:16:47 -0500 (Fri, 11 Apr 2008) | 1 line Make sure that we disable the echo canceller on the old style echocan() function even if parameters are passed in. ........ r4170 | mattf | 2008-04-11 16:17:55 -0500 (Fri, 11 Apr 2008) | 1 line Take out part of a commit to the wct4xxp driver that wasn't supposed to go in ........ r4173 | kpfleming | 2008-04-17 07:20:26 -0500 (Thu, 17 Apr 2008) | 6 lines when processing ZT_SET_DIALPARAMS, don't return an error for out-of-range tone durations, just ignore them (so that applications that don't initialize the entire zt_dialparams structure won't fail to set the ones they do initialize) (closes issue ASTERISK-11861) Reported by: fnordian Tested by: fnordian ........ r4175 | tzafrir | 2008-04-17 09:27:50 -0500 (Thu, 17 Apr 2008) | 5 lines Avoid a double-free in ztdynamic.c ztdynamic_chanfree.diff from bug ZAP-308 . Merged revisions 4174 via svnmerge from http://svn.digium.com/svn/zaptel/branches/1.2 ........ r4176 | jdixon | 2008-04-18 10:19:00 -0500 (Fri, 18 Apr 2008) | 2 lines Fixed monitoring of zap pseudo channels ........ r4183 | mattf | 2008-04-19 16:54:03 -0500 (Sat, 19 Apr 2008) | 1 line Partial fix for DAHLIN-5.  Fixes potential race condition in open and close routines for zaptel devices ........ r4184 | mattf | 2008-04-19 16:56:06 -0500 (Sat, 19 Apr 2008) | 1 line Remove unintentional svn property addition ........ r4185 | mattf | 2008-04-19 17:05:42 -0500 (Sat, 19 Apr 2008) | 1 line Fix compiler from complaining ........ r4187 | tzafrir | 2008-04-21 16:27:59 -0500 (Mon, 21 Apr 2008) | 4 lines Zaptel::Config::Default - don't die if no config file. (Accidentally left out of the merge from 1.2) ........ r4190 | jdixon | 2008-04-22 17:07:08 -0500 (Tue, 22 Apr 2008) | 2 lines Fixed problem preventing multiple channels from using multiple CTCSS ........ r4191 | jdixon | 2008-04-22 23:36:22 -0500 (Tue, 22 Apr 2008) | 2 lines Updated to latest code, fixed bug in serial i/o ........ r4192 | tzafrir | 2008-04-23 04:48:56 -0500 (Wed, 23 Apr 2008) | 2 lines Bumped into yet another incompatibility. ........ r4193 | tzafrir | 2008-04-23 14:20:53 -0500 (Wed, 23 Apr 2008) | 10 lines A test for a specific #define in zconfig.h was done by invoking cpp . Sadly the change of CFLAGS handling in kernel 2.6.24 meant that it is no longer as easy to invoke cpp on our own. Impact: On kernel >= 2.6.24, xpd_bri never got built, even if the bri_dchan patch was applied. Fix: use a simpler grep instead. Hopefully noone passes it through other means. This should hopfully fix issue ZAP-312 . ........ r4194 | sruffell | 2008-04-24 13:17:12 -0500 (Thu, 24 Apr 2008) | 5 lines Fixes a regression in versions 1.2.25 and 1.4.19 identified by korihor where the wctdm driver was no longer properly recognizing polarity reversals. (closes issue ZAP-309) ........ r4195 | qwell | 2008-04-25 13:10:06 -0500 (Fri, 25 Apr 2008) | 1 line Based on a comment made by moy in #asterisk-dev, and the surrounding code in these functions, I believe that this is what was actually intended in r4130. ........ r4196 | qwell | 2008-04-25 15:47:07 -0500 (Fri, 25 Apr 2008) | 7 lines Since MF R2 tones are generated continuously, we need to not set the dialing status. (closes issue ZAP-317) Reported by: moy Patches:      zaptel-mf-dialing-rev4195.patch uploaded by moy (license 222) ........ r4213 | twilson | 2008-05-01 11:46:44 -0500 (Thu, 01 May 2008) | 2 lines Allow $(ARCH) to be overridden and pass on through $(KMAKE) ........ r4217 | qwell | 2008-05-01 14:37:07 -0500 (Thu, 01 May 2008) | 7 lines Clarify a message.  Some hardware doesn't provide timing, so this could be confusing. (closes issue ZAP-313) Reported by: panderson Patches:      zaptel.init.patch uploaded by panderson (license 469) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=4220 By: Digium Subversion (svnbot) 2010-10-20 07:23:18 Repository: dahdi Revision: 9444 U   linux/trunk/drivers/dahdi/dahdi-base.c U   linux/trunk/drivers/dahdi/tor2.c U   linux/trunk/drivers/dahdi/wct1xxp.c U   linux/trunk/drivers/dahdi/wct4xxp/base.c U   linux/trunk/drivers/dahdi/wcte11xp.c U   linux/trunk/drivers/dahdi/wcte12xp/base.c U   linux/trunk/include/dahdi/kernel.h ------------------------------------------------------------------------ r9444 | sruffell | 2010-10-20 07:23:18 -0500 (Wed, 20 Oct 2010) | 12 lines dahdi: Atomically set/test if channel has associated network device. Push all tests for the DAHDI_FLAGBIT_NETDEV flag behind a 'dahdi_have_netdev' function so if CONFIG_DAHDI_NET is not defined the compiler can just remove all the flag tests.  Also, makes sure that the bit is checked / set atomically. (closes issue DAHLIN-5) Signed-off-by: Shaun Ruffell <sruffell@digium.com> Acked-by: Kinsey Moore <kmoore@digium.com> Acked-by: Russ Meyerriecks <rmeyerriecks@digium.com> ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9444