linux-kernel - Re: [PATCH] poll: Ignore benign race on pwd->triggered

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] [day] [month] [year] [list]

Message-ID: <YJBE/3nzZV3pazGx@elver.google.com>
Date:   Mon, 3 May 2021 20:46:23 +0200
From:   Marco Elver <elver@...gle.com>
To:     Baptiste Lepers <baptiste.lepers@...il.com>
Cc:     trivial@...nel.org, Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Miklos Szeredi <mszeredi@...e.cz>,
        Tejun Heo <htejun@...il.com>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, paulmck@...nel.org
Subject: Re: [PATCH] poll: Ignore benign race on pwd->triggered

Thanks for the patch -- any progress on this front is much appreciated.

However, the subject is not really doing this patch justice. We're not
ignoring the race -- and if by "ignore" you mean let KCSAN ignore the
race, then it's more like saying "we'll make the sanitizer not report
this problem anymore... by fixing the problem".

I'd suggest, similar to wording below:

	"poll: mark racy accesses on pwq->triggered"

Data races are real, and even though at a high-level (where we pretend
all accesses are atomic) these races are safe and intentional, it it not
at all clear to the reader why the data race would be "benign" -- that
is if the compiler optimized or miscompiled the code in such a way that
we end up with cases unintended by the programmer.

In this case, we can probably argue that the data race would be safe
(benign), given there's a simple 0->1 transition on triggered, and the
reader only doing a compare-against-0.

Nevertheless, a READ_ONCE()/WRITE_ONCE() pair is preferred if there are
no objections, and generated code almost always is the same, and it
saves us reasoning about another use of data_race().

Paul recently summarized some of these strategies:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt

On Thu, Apr 29, 2021 at 01:44PM +1000, Baptiste Lepers wrote:
> Mark data races to pwd->triggered as benign using READ_ONCE and
> WRITE_ONCE. These accesses are expected to be racy per comment.

This sounds fine, given there's already a comment.

> This patch is part of a series of patches aiming at reducing the number
> of benign races reported by KCSAN in order to focus future debugging
> effort on harmful races.

Looking forward to the rest.

> Reported-by: syzbot+9b3fb64bcc8c1d807595@...kaller.appspotmail.com
> Fixes: 5f820f648c92a ("poll: allow f_op->poll to sleep")
> Signed-off-by: Baptiste Lepers <baptiste.lepers@...il.com>

Acked-by: Marco Elver <elver@...gle.com>

> ---
>  fs/select.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/select.c b/fs/select.c
> index 945896d0ac9e..e71b4d1a2606 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -194,7 +194,7 @@ static int __pollwake(wait_queue_entry_t *wait, unsigned mode, int sync, void *k
>  	 * and is paired with smp_store_mb() in poll_schedule_timeout.
>  	 */
>  	smp_wmb();
> -	pwq->triggered = 1;
> +	WRITE_ONCE(pwq->triggered, 1);
>  
>  	/*
>  	 * Perform the default wake up operation using a dummy
> @@ -239,7 +239,7 @@ static int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
>  	int rc = -EINTR;
>  
>  	set_current_state(state);
> -	if (!pwq->triggered)
> +	if (!READ_ONCE(pwq->triggered))
>  		rc = schedule_hrtimeout_range(expires, slack, HRTIMER_MODE_ABS);
>  	__set_current_state(TASK_RUNNING);
>  
> -- 
> 2.17.1