Hi Mikulas, On Sun, 31 Jul 2022 at 13:43, Mikulas Patocka <mpatocka@xxxxxxxxxx> 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 a memory barrier. > Consequently, if someone calls sb_bread and then reads the buffer data, > the read of buffer data may be speculatively executed before > wait_on_buffer(bh) and it may return invalid data. > 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. > Also, there is this pattern present several times: > wait_on_buffer(bh); > if (!buffer_uptodate(bh)) > err = -EIO; > It may be possible that buffer_uptodate is executed before wait_on_buffer > and it may return spurious error. > > Fix these bugs by adding a read memory barrier to wait_on_buffer(). > > Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> > Cc: stable@xxxxxxxxxxxxxxx > > Index: linux-2.6/include/linux/buffer_head.h > =================================================================== > --- linux-2.6.orig/include/linux/buffer_head.h > +++ linux-2.6/include/linux/buffer_head.h > @@ -353,6 +353,11 @@ static inline void wait_on_buffer(struct > might_sleep(); > if (buffer_locked(bh)) > __wait_on_buffer(bh); > + /* > + * Make sure that the following accesses to buffer state or buffer data > + * are not reordered with buffer_locked(bh). > + */ > + smp_rmb(); > } > > static inline int trylock_buffer(struct buffer_head *bh) > 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?
