linux-kernel - Re: [PATCH 07/10] netfs: Fix missing barriers by using clear_and_wake_up

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]

Message-ID: <3332016.1734183881@warthog.procyon.org.uk>
Date: Sat, 14 Dec 2024 13:44:41 +0000
From: David Howells <dhowells@...hat.com>
To: Akira Yokosawa <akiyks@...il.com>,
    "Paul E. McKenney" <paulmck@...nel.org>
Cc: dhowells@...hat.com, Christian Brauner <christian@...uner.io>,
    Max Kellermann <max.kellermann@...os.com>,
    Ilya Dryomov <idryomov@...il.com>, Xiubo Li <xiubli@...hat.com>,
    Trond Myklebust <trondmy@...nel.org>,
    Jeff Layton <jlayton@...nel.org>,
    Matthew Wilcox <willy@...radead.org>, netfs@...ts.linux.dev,
    linux-afs@...ts.infradead.org, linux-cifs@...r.kernel.org,
    linux-nfs@...r.kernel.org, ceph-devel@...r.kernel.org,
    v9fs@...ts.linux.dev, linux-erofs@...ts.ozlabs.org,
    linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
    linux-kernel@...r.kernel.org, Zilin Guan <zilin@....edu.cn>
Subject: Re: [PATCH 07/10] netfs: Fix missing barriers by using clear_and_wake_up_bit()

[Adding Paul McKenney as he's the expert.]

Akira Yokosawa <akiyks@...il.com> wrote:

> David Howells wrote:
> > Use clear_and_wake_up_bit() rather than something like:
> > 
> > 	clear_bit_unlock(NETFS_RREQ_IN_PROGRESS, &rreq->flags);
> > 	wake_up_bit(&rreq->flags, NETFS_RREQ_IN_PROGRESS);
> > 
> > as there needs to be a barrier inserted between which is present in
> > clear_and_wake_up_bit().
> 
> If I am reading the kernel-doc comment of clear_bit_unlock() [1, 2]:
> 
>     This operation is atomic and provides release barrier semantics.
> 
> correctly, there already seems to be a barrier which should be
> good enough.
> 
> [1]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.clear_bit_unlock
> [2]: include/asm-generic/bitops/instrumented-lock.h
> 
> > 
> > Fixes: 288ace2f57c9 ("netfs: New writeback implementation")
> > Fixes: ee4cdf7ba857 ("netfs: Speed up buffered reading")
> 
> So I'm not sure this fixes anything.
> 
> What am I missing?

We may need two barriers.  You have three things to synchronise:

 (1) The stuff you did before unlocking.

 (2) The lock bit.

 (3) The task state.

clear_bit_unlock() interposes a release barrier between (1) and (2).

Neither clear_bit_unlock() nor wake_up_bit(), however, necessarily interpose a
barrier between (2) and (3).  I'm not sure it entirely matters, but it seems
that since we have a function that combines the two, we should probably use
it - though, granted, it might not actually be a fix.

David