Messages in this thread | | | From | Ard Biesheuvel <> | Date | Sun, 31 Jul 2022 14:00:16 +0200 | Subject | Re: [PATCH] Add a read memory barrier to wait_on_buffer |
| |
Hi Mikulas,
On Sun, 31 Jul 2022 at 13:43, Mikulas Patocka <mpatocka@redhat.com> 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@redhat.com> > Cc: stable@vger.kernel.org > > 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?
| |