Re: + kthread-fix-kthread_mod_delayed_work-vs-kthread_cancel_delayed_work_sync-race.patch added to -mm tree

[Petr Mladek]

Added Tejun into CC because of the workqueue API related question
at the end of the mail.

On Wed 2021-05-26 19:06:06, Oleg Nesterov wrote:
> On 05/24, Petr Mladek wrote:
> >
> > Your patch changes the semantic. The current semantic is the same for
> > the workqueue's counter-part mod_delayed_work_on().
> 
> OK, I see, thanks. I was confused by the comment.
> 
> > We should actually keep the "ret" value as is to stay compatible with
> > workqueue API:
> >
> > 	/*
> > 	 * Canceling could run in parallel from kthread_cancel_delayed_work_sync
> > 	 * and change work's canceling count as the spinlock is released and regain
> > 	 * in __kthread_cancel_work so we need to check the count again. Otherwise,
> > 	 * we might incorrectly queue the dwork and further cause
> > 	 * cancel_delayed_work_sync thread waiting for flush dwork endlessly.
> > 	 *
> > 	 * Keep the ret code. The API primary informs the caller
> > 	 * whether some pending work has been canceled (not proceed).
> > 	 */
> > 	if (work->canceling)
> > 		goto out;
> 
> Agreed, we should keep the "ret" value.

Martin Liu, could you please resend the patch without the "ret =
false" line? See above.

Andrew, could you please remove this patch from the -mm tree for now?

> but unless I am confused again this doesn't match mod_delayed_work_on()
> which always returns true if it races with cancel(). Nevermind, I think
> this doesn't matter.

Good point. I think that it is actually a bug. Most callers ignore
the return code but there is the following user:

static void addrconf_del_dad_work(struct inet6_ifaddr *ifp)
{
	if (cancel_delayed_work(&ifp->dad_work))
		__in6_ifa_put(ifp);
}
static void addrconf_mod_dad_work(struct inet6_ifaddr *ifp,
				   unsigned long delay)
{
	in6_ifa_hold(ifp);
	if (mod_delayed_work(addrconf_wq, &ifp->dad_work, delay))
		in6_ifa_put(ifp);
}

If mod_delayed_work() races with cancel_delayed_work() then both might
return true and call in6_ifa_put(ifp).

I thought that they were serialized by ifp->lock. But, for example,
addrconf_dad_start() calls addrconf_mod_dad_work() after releasing
this lock.

It is possible that they are serialized another way. But I think that
in principle only the one that really cancelled a pending work
should return "true".

Tejun, any opinion?  Feel free to ask for more context.

Best Regards,
Petr