Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 5 Aug 2022 04:22:59 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Mikulas Patocka <mpatocka@redhat.com>
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
Message-ID: <YuyNE5c06WStxQ2z@casper.infradead.org>
References: <alpine.LRH.2.02.2207310920390.6506@file01.intranet.prod.int.rdu2.redhat.com>
 <alpine.LRH.2.02.2207311104020.16444@file01.intranet.prod.int.rdu2.redhat.com>
 <CAHk-=wiC_oidYZeMD7p0E-=TAuLgrNQ86-sB99=hRqFM8fVLDQ@mail.gmail.com>
 <alpine.LRH.2.02.2207311542280.21273@file01.intranet.prod.int.rdu2.redhat.com>
 <alpine.LRH.2.02.2207311639360.21350@file01.intranet.prod.int.rdu2.redhat.com>
 <CAHk-=wjA8HBrVqAqAetUvwNr=hcvhfnO7oMrOAd4V8bbSqokNA@mail.gmail.com>
 <alpine.LRH.2.02.2208010628510.22006@file01.intranet.prod.int.rdu2.redhat.com>
 <alpine.LRH.2.02.2208010642220.22006@file01.intranet.prod.int.rdu2.redhat.com>
 <YuflGG60pHiXp2z/@casper.infradead.org>
 <alpine.LRH.2.02.2208011040190.27101@file01.intranet.prod.int.rdu2.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LRH.2.02.2208011040190.27101@file01.intranet.prod.int.rdu2.redhat.com>
Precedence: bulk

On Mon, Aug 01, 2022 at 11:01:40AM -0400, Mikulas Patocka wrote:
> 
> 
> 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?

I think you misunderstand the purpose of locked/uptodate.  At least
for pages, the lock flag does not order access to the data in the page.
Indeed, the contents of the page can be changed while you hold the lock.
But the uptodate flag does order access to the data.  At the point where
you can observe the uptodate flag set, you know the contents of the page
have been completely read from storage.  And you don't need to hold the
lock to check the uptodate flag.  So this is wrong:

	buffer_lock()
	*data = 0x12345678;
	buffer_set_uptodate_not_ordered()
	buffer_unlock_ordered()

because a reader can do:

	while (!buffer_test_uptodate()) {
		buffer_lock();
		buffer_unlock();
	}
	x = *data;

and get x != 0x12345678 because the compiler can move the
buffer_set_uptodate_not_ordered() before the store to *data.