Race in expire code

Race in expire code

[raven]

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;

Re: Race in expire code

[Steinar H. Gunderson]

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/

Re: Race in expire code

[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?

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

Re: Race in expire code

[Jeff Moyer]

==> 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

Re: Race in expire code

[Chris Feist]

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