Re: [PATCH v2] buffer: Fix I/O error due to ARM read-after-read hazard

[Will Deacon]

Hi Russell,

On Wed, Nov 13, 2019 at 10:31:51AM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Nov 13, 2019 at 10:23:58AM +0000, Will Deacon wrote:
> > On Tue, Nov 12, 2019 at 10:39:01AM -0800, Linus Torvalds wrote:
> > > On Tue, Nov 12, 2019 at 10:22 AM Catalin Marinas
> > > <catalin.marinas@xxxxxxx> wrote:
> > > >
> > > > OK, so this includes changing test_bit() to perform a READ_ONCE.
> > > 
> > > That's not going to happen.
> > 
> > Ok, I'll stick my neck out here, but if test_bit() is being used to read
> > a bitmap that is being concurrently modified (e.g. by set_bit() which boils
> > down to atomic_long_or()), then why isn't READ_ONCE() required? Right now,
> > test_bit takes a 'const volatile unsigned long *addr' argument, so I don't
> > see that you'll get a change in codegen except on alpha and, with this
> > erratum, arm32.
> 
> I'm not entirely clear what you're suggesting, so I'll just pick the
> scenario that I think you're talking about - but I'm not sure it's the
> one you're intending.
> 
> Using test_bit() in one thread and set_bit() on the same bit in another
> thread without locking is going to be racy by definition.  It's entirely
> possible for:
> 
> 	Thread 1			Thread 2
> 	bit = test_bit(...);
> 					set_bit(...);
> 	/* use bit */
> 
> and here, bit == 0 but the bit has been set by thread 2.  Use of the
> result from test_bit() is inherently a non-atomic operation.

I think it's atomic in the same way that atomic_read() is atomic (which is
typically defined using READ_ONCE()).

> This is why we have test_and_set_bit() and friends that atomically test
> that a bit is clear before setting it.  Where this is especially
> important is for some filesystems, as they use test_and_xxx_bit() to
> manage their allocation bitmaps.

Agreed, but what we don't want is something like:

	Thread 1			Thread 2
					set_bit(...); // bit is now 1
	test_bit(...); // returns 1
	test_bit(...); // returns 0

which is what can happen due to this erratum. It's generally good practice
to use READ_ONCE() when reading something which can be updated concurrently
because:

	* It ensures that the value is (re-)loaded from memory

	* It prevents the compiler from performing harmful optimisations,
	  such as merging or tearing (although in this case I suspect
	  these are ok because we're dealing with a single bit)

	* On Alpha, it gives you a barrier so that dependency ordering
	  can be relied upon from the load

	* It keeps KCSAN happy

I think the current definition of test_bit() gives you the first two by
casting to volatile explicitly, but not the second two.

So I'm mainly curious as to the disconnect between my thinking and Linus's
"That's not going to happen" comment, given that it shouldn't have an
impact on architectures that don't need magic here.

Will