svn commit: r286880 - head/sys/kern

[Hans Petter Selasky]

On 08/18/15 12:15, Julien Charbon wrote:
> Author: jch
> Date: Tue Aug 18 10:15:09 2015
> New Revision: 286880
> URL: https://svnweb.freebsd.org/changeset/base/286880
>
> Log:
>    callout_stop() should return 0 (fail) when the callout is currently
>    being serviced and indeed unstoppable.
>
>    A scenario to reproduce this case is:
>
>    - the callout is being serviced and at same time,
>    - callout_reset() is called on this callout that sets
>      the CALLOUT_PENDING flag and at same time,
>    - callout_stop() is called on this callout and returns 1 (success)
>      even if the callout is indeed currently running and unstoppable.
>
>    This issue was caught up while making r284245 (D2763) workaround, and
>    was discussed at BSDCan 2015.  Once applied the r284245 workaround
>    is not needed anymore and will be reverted.
>
>    Differential Revision:	https://reviews.freebsd.org/D3078
>    Reviewed by:		jhb
>    Sponsored by:		Verisign, Inc.
>
> Modified:
>    head/sys/kern/kern_timeout.c
>
> Modified: head/sys/kern/kern_timeout.c
> ==============================================================================
> --- head/sys/kern/kern_timeout.c	Tue Aug 18 10:07:03 2015	(r286879)
> +++ head/sys/kern/kern_timeout.c	Tue Aug 18 10:15:09 2015	(r286880)
> @@ -1150,7 +1150,7 @@ _callout_stop_safe(struct callout *c, in
>   	struct callout_cpu *cc, *old_cc;
>   	struct lock_class *class;
>   	int direct, sq_locked, use_lock;
> -	int not_on_a_list;
> +	int not_on_a_list, not_running;
>
>   	if (safe)
>   		WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock,
> @@ -1378,8 +1378,15 @@ again:
>   		}
>   	}
>   	callout_cc_del(c, cc);
> +
> +	/*
> +	 * If we are asked to stop a callout which is currently in progress
> +	 * and indeed impossible to stop then return 0.
> +	 */
> +	not_running = !(cc_exec_curr(cc, direct) == c);
> +
>   	CC_UNLOCK(cc);
> -	return (1);
> +	return (not_running);
>   }
>
>   void
>
>

Hi,

I think this patch is incomplete and can break the return value for 
non-MPSAFE callouts. I think the correct statement should check the 
value of "use_lock" too:

	not_running = !(cc_exec_curr(cc, direct) == c && use_lock == 0);

Because if the callback process waits for lock "c_lock" in the callback 
process then we have above "cc_exec_curr(cc, direct) == c" is satisfied 
too, and non-MPSAFE callouts are always cancelable, via 
cc_exec_cancel(cc, direct) = true;

>                 class->lc_lock(c_lock, lock_status);
>                 /*
>                  * The callout may have been cancelled
>                  * while we switched locks.
>                  */
>                 if (cc_exec_cancel(cc, direct)) {
>                         class->lc_unlock(c_lock);
>                         goto skip;
>                 }
>                 /* The callout cannot be stopped now. */
>                 cc_exec_cancel(cc, direct) = true;

--HPS