* Race in expire code @ 2005-01-29 13:36 raven 2005-01-29 13:51 ` Steinar H. Gunderson 0 siblings, 1 reply; 5+ messages in thread From: raven @ 2005-01-29 13:36 UTC (permalink / raw) To: autofs mailing list Hi all, I've had three reports now, of mounts not expiring. This appears to be caused by a race in the function st_expire in automount.c. Can everyone who is maintaining downstream packages please chaeck that the code in their package ends up looking like what the patch below acheives. And for those that have reported the problem, please try this patch. Ian --- autofs-4.1.3-michael/daemon/automount.c.sig-fix 2005-01-19 20:11:09.000000000 +0800 +++ autofs-4.1.3-michael/daemon/automount.c 2005-01-19 20:12:02.000000000 +0800 @@ -920,8 +920,8 @@ return 1; case EXP_STARTED: - sigprocmask(SIG_SETMASK, &ready_sigs, NULL); ap.state = ST_EXPIRE; + sigprocmask(SIG_SETMASK, &ready_sigs, NULL); return 0; } return 1; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Race in expire code 2005-01-29 13:36 Race in expire code raven @ 2005-01-29 13:51 ` Steinar H. Gunderson 2005-01-29 14:13 ` raven 0 siblings, 1 reply; 5+ messages in thread From: Steinar H. Gunderson @ 2005-01-29 13:51 UTC (permalink / raw) To: autofs On Sat, Jan 29, 2005 at 09:36:31PM +0800, raven@themaw.net wrote: > Can everyone who is maintaining downstream packages please chaeck that > the code in their package ends up looking like what the patch below > acheives. > > [...] > > case EXP_STARTED: > - sigprocmask(SIG_SETMASK, &ready_sigs, NULL); > ap.state = ST_EXPIRE; > + sigprocmask(SIG_SETMASK, &ready_sigs, NULL); > return 0; > } > return 1; The Debian packages are close, but not quite: case EXP_STARTED: sigprocmask(SIG_SETMASK, &lock_sigs, NULL); ap.state = ST_EXPIRE; sigprocmask(SIG_SETMASK, &ready_sigs, NULL); return 0; I have no idea what lock_sigs does, but this should be OK, no? /* Steinar */ -- Homepage: http://www.sesse.net/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Race in expire code 2005-01-29 13:51 ` Steinar H. Gunderson @ 2005-01-29 14:13 ` raven 2005-01-31 15:34 ` Jeff Moyer 0 siblings, 1 reply; 5+ messages in thread From: raven @ 2005-01-29 14:13 UTC (permalink / raw) To: Steinar H. Gunderson; +Cc: autofs On Sat, 29 Jan 2005, Steinar H. Gunderson wrote: > On Sat, Jan 29, 2005 at 09:36:31PM +0800, raven@themaw.net wrote: >> Can everyone who is maintaining downstream packages please chaeck that >> the code in their package ends up looking like what the patch below >> acheives. >> >> [...] >> >> case EXP_STARTED: >> - sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >> ap.state = ST_EXPIRE; >> + sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >> return 0; >> } >> return 1; > > The Debian packages are close, but not quite: > > case EXP_STARTED: > sigprocmask(SIG_SETMASK, &lock_sigs, NULL); > ap.state = ST_EXPIRE; > sigprocmask(SIG_SETMASK, &ready_sigs, NULL); > return 0; > > I have no idea what lock_sigs does, but this should be OK, no? It's just the list of signals that are blocked while we do "stuff" that we don`t want interrupted by something else. Similarly ready_sigs contains the signals we expect to be received in normal operation. The first sigprocmask call above does nothing as signals are already blocked at that point. Ian ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Race in expire code 2005-01-29 14:13 ` raven @ 2005-01-31 15:34 ` Jeff Moyer 2005-01-31 16:47 ` Chris Feist 0 siblings, 1 reply; 5+ messages in thread From: Jeff Moyer @ 2005-01-31 15:34 UTC (permalink / raw) To: raven; +Cc: autofs, cfeist ==> Regarding Re: [autofs] Race in expire code; raven@themaw.net adds: raven> On Sat, 29 Jan 2005, Steinar H. Gunderson wrote: >> On Sat, Jan 29, 2005 at 09:36:31PM +0800, raven@themaw.net wrote: >>> Can everyone who is maintaining downstream packages please chaeck that >>> the code in their package ends up looking like what the patch below >>> acheives. >>> >>> [...] >>> >>> case EXP_STARTED: - sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >>> ap.state = ST_EXPIRE; + sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >>> return 0; } return 1; >> The Debian packages are close, but not quite: >> >> case EXP_STARTED: sigprocmask(SIG_SETMASK, &lock_sigs, NULL); ap.state = >> ST_EXPIRE; sigprocmask(SIG_SETMASK, &ready_sigs, NULL); return 0; >> >> I have no idea what lock_sigs does, but this should be OK, no? raven> It's just the list of signals that are blocked while we do "stuff" raven> that we don`t want interrupted by something else. raven> Similarly ready_sigs contains the signals we expect to be received raven> in normal operation. raven> The first sigprocmask call above does nothing as signals are already raven> blocked at that point. The Red Hat package also has the two calls to sigprocmask. When "fixing" signal races, I definitely managed to introduce a similar bug. Chris Feist fixed it. Chris, do you agree with Ian on this matter, that the first sigprocmask is superfluous? -Jeff ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Race in expire code 2005-01-31 15:34 ` Jeff Moyer @ 2005-01-31 16:47 ` Chris Feist 0 siblings, 0 replies; 5+ messages in thread From: Chris Feist @ 2005-01-31 16:47 UTC (permalink / raw) To: jmoyer, autofs Jeff Moyer wrote: > ==> Regarding Re: [autofs] Race in expire code; raven@themaw.net adds: > > raven> On Sat, 29 Jan 2005, Steinar H. Gunderson wrote: > >>>On Sat, Jan 29, 2005 at 09:36:31PM +0800, raven@themaw.net wrote: >>> >>>>Can everyone who is maintaining downstream packages please chaeck that >>>>the code in their package ends up looking like what the patch below >>>>acheives. >>>> >>>>[...] >>>> >>>>case EXP_STARTED: - sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >>>>ap.state = ST_EXPIRE; + sigprocmask(SIG_SETMASK, &ready_sigs, NULL); >>>>return 0; } return 1; >>> >>>The Debian packages are close, but not quite: >>> >>>case EXP_STARTED: sigprocmask(SIG_SETMASK, &lock_sigs, NULL); ap.state = >>>ST_EXPIRE; sigprocmask(SIG_SETMASK, &ready_sigs, NULL); return 0; >>> >>>I have no idea what lock_sigs does, but this should be OK, no? > > > raven> It's just the list of signals that are blocked while we do "stuff" > raven> that we don`t want interrupted by something else. > > raven> Similarly ready_sigs contains the signals we expect to be received > raven> in normal operation. > > raven> The first sigprocmask call above does nothing as signals are already > raven> blocked at that point. > > The Red Hat package also has the two calls to sigprocmask. When "fixing" > signal races, I definitely managed to introduce a similar bug. Chris Feist > fixed it. Chris, do you agree with Ian on this matter, that the first > sigprocmask is superfluous? Yes, I think I just included it to be safe, but it should be able to be safely removed. The reason that we needed to make the 'ap.state = ST_EXPIRE' run before enabling ready_sigs, is as soon as we enable sigprocmask the kernel sends down a queued up SIGCHLD which then runs the sigchld event handler which doesn't realize that we're in ST_EXPIRE (because it hasn't been set yet). Thanks, Chris ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-01-31 16:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-01-29 13:36 Race in expire code raven 2005-01-29 13:51 ` Steinar H. Gunderson 2005-01-29 14:13 ` raven 2005-01-31 15:34 ` Jeff Moyer 2005-01-31 16:47 ` Chris Feist
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.