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:   Sun, 31 Jul 2022 09:41:02 -0400 (EDT)
From:   Mikulas Patocka <mpatocka@redhat.com>
To:     Ard Biesheuvel <ardb@kernel.org>
cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Alan Stern <stern@rowland.harvard.edu>,
        Andrea Parri <parri.andrea@gmail.com>,
        Will Deacon <will@kernel.org>,
        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>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        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] Add a read memory barrier to wait_on_buffer
In-Reply-To: <CAMj1kXFYRNrP2k8yppgfdKg+CxWeYfHTbzLBuyBqJ9UVAR_vaQ@mail.gmail.com>
Message-ID: <alpine.LRH.2.02.2207310920390.6506@file01.intranet.prod.int.rdu2.redhat.com>
References: <alpine.LRH.2.02.2207310703170.14394@file01.intranet.prod.int.rdu2.redhat.com> <CAMj1kXFYRNrP2k8yppgfdKg+CxWeYfHTbzLBuyBqJ9UVAR_vaQ@mail.gmail.com>
User-Agent: Alpine 2.02 (LRH 1266 2009-07-14)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Precedence: bulk


On Sun, 31 Jul 2022, Ard Biesheuvel wrote:

> This has little to do with speculation, so better to drop this S bomb
> from your commit message. This is about concurrency and weak memory
> ordering.

Yes.

> This doesn't seem like a very robust fix to me, tbh - I suppose this
> makes the symptom you encountered go away, but the underlying issue
> remains afaict.
> 
> Given that the lock and uptodate fields etc are just bits in a
> bitfield, wouldn't it be better to use cmpxchg() with acquire/release
> semantics (as appropriate) to manage these bits?

The kernel already uses clear_bit_unlock, test_and_set_bit_lock and 
wait_on_bit_lock_io to manage the BH_Lock bit - and they have 
acquire/release semantics.

The only problem is that test_bit doesn't provide any memory barriers. 
Should we add the barrier to buffer_locked() instead of wait_on_buffer()? 
Perhaps it would fix more bugs - in reiserfs, there's this piece of code:

                if (buffer_locked(bh)) {
                        spin_unlock(lock);
                        wait_on_buffer(bh);
                        spin_lock(lock);
                }
                if (!buffer_uptodate(bh)) {
                        ret = -EIO;
                }

or this:
                if (buffer_locked(bh)) {
                        int depth;
                        PROC_INFO_INC(sb, scan_bitmap.wait);
                        depth = reiserfs_write_unlock_nested(sb);
                        __wait_on_buffer(bh);
                        reiserfs_write_lock_nested(sb, depth);
                }
                BUG_ON(!buffer_uptodate(bh));
                BUG_ON(atomic_read(&bh->b_count) == 0);

That assumes that buffer_locked provides a barrier.

Mikulas