linux-kernel - Re: + kthread-fix-kthread_mod_delayed_work-vs-kthread_cancel_delayed_work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 May 2021 19:06:06 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     akpm@...ux-foundation.org, bp@...e.de, davidchao@...gle.com,
        jenhaochen@...gle.com, jkosina@...e.cz, josh@...htriplett.org,
        liumartin@...gle.com, mhocko@...e.cz, mingo@...hat.com,
        mm-commits@...r.kernel.org, nathan@...nel.org,
        ndesaulniers@...gle.com, paulmck@...ux.vnet.ibm.com,
        peterz@...radead.org, rostedt@...dmis.org, stable@...r.kernel.org,
        tglx@...utronix.de, tj@...nel.org, vbabka@...e.cz,
        linux-kernel@...r.kernel.org
Subject: Re: +
 kthread-fix-kthread_mod_delayed_work-vs-kthread_cancel_delayed_work_sync-race.patch
 added to -mm tree

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.

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.

Thanks,

Oleg.