Re: [PATCH v4 2/2] change buffer_locked, so that it has acquire semantics

From: Mikulas Patocka <mpatocka@redhat.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Will Deacon <will@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Alan Stern <stern@rowland.harvard.edu>,
	Andrea Parri <parri.andrea@gmail.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Nicholas Piggin <npiggin@gmail.com>,
	David Howells <dhowells@redhat.com>,
	Jade Alglave <j.alglave@ucl.ac.uk>,
	Luc Maranget <luc.maranget@inria.fr>,
	Akira Yokosawa <akiyks@gmail.com>,
	Daniel Lustig <dlustig@nvidia.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] change buffer_locked, so that it has acquire semantics
Date: Mon, 1 Aug 2022 11:01:40 -0400 (EDT)	[thread overview]
Message-ID: <alpine.LRH.2.02.2208011040190.27101@file01.intranet.prod.int.rdu2.redhat.com> (raw)
In-Reply-To: <YuflGG60pHiXp2z/@casper.infradead.org>

On Mon, 1 Aug 2022, Matthew Wilcox wrote:

> On Mon, Aug 01, 2022 at 06:43:55AM -0400, Mikulas Patocka wrote:
> > Let's have a look at this piece of code in __bread_slow:
> > 	get_bh(bh);
> > 	bh->b_end_io = end_buffer_read_sync;
> > 	submit_bh(REQ_OP_READ, 0, bh);
> > 	wait_on_buffer(bh);
> > 	if (buffer_uptodate(bh))
> > 		return bh;
> > Neither wait_on_buffer nor buffer_uptodate contain any memory barrier.
> > Consequently, if someone calls sb_bread and then reads the buffer data,
> > the read of buffer data may be executed before wait_on_buffer(bh) on
> > architectures with weak memory ordering and it may return invalid data.
> > 
> > Fix this bug by changing the function buffer_locked to have the acquire
> > semantics - so that code that follows buffer_locked cannot be moved before
> > it.
> 
> I think this is the wrong approach.  Instead, buffer_set_uptodate()
> should have the smp_wmb() and buffer_uptodate should have the smp_rmb().
> Just like the page flags.  As I said last night.

Linus said that he prefers acquire/release to smp_rmb/smp_wmb. So, sort it 
out with him :)

In most cases, the buffer is set uptodate while it is locked, so that 
there is no race on the uptodate flag (the race exists on the locked 
flag). Are there any cases where the uptodate flag is modified on unlocked 
buffer, so that it needs special treatment too?

Mikulas