| Message ID | alpine.LRH.2.02.2207311639360.21350@file01.intranet.prod.int.rdu2.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] wait_bit: do read barrier after testing a bit | expand |
On Sun, Jul 31, 2022 at 1:41 PM Mikulas Patocka <mpatocka@redhat.com> wrote: > > - if (!test_bit(bit, word)) > + if (!test_bit(bit, word)) { > + smp_rmb(); Logically, I don't think that makes sense. Maybe you're checking the buffer being up-to-date before you *write* to it? So smp_rmb() seems entirely wrong. I think it should consistently aim for just doing unsigned long state = smp_read_acquire(word); if (!(state & (1 << bit))) return 0; or whatever. We should strive to *not* add new uses of the legacy memory barriers. They are garbage from last century when people didn't know better. Then people learnt to use acquire and release, and things improved. Let's live in that improved world. Linus
On Sun, Jul 31, 2022 at 04:40:59PM -0400, Mikulas Patocka wrote: > wait_on_bit tests the bit without any memory barriers, consequently the > code that follows wait_on_bit may be moved before testing the bit on > architectures with weak memory ordering. When the code tests for some > event using wait_on_bit and then performs a load operation, the load may > be unexpectedly moved before wait_on_bit and it may return data that > existed before the event occurred. > > Such bugs exist in fs/buffer.c:__wait_on_buffer, > drivers/md/dm-bufio.c:new_read, > drivers/media/usb/dvb-usb-v2/dvb_usb_core.c:dvb_usb_start_feed, > drivers/bluetooth/btusb.c:btusb_mtk_hci_wmt_sync > and perhaps in other places. > > We fix this class of bugs by adding a read barrier after test_bit(). > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Cc: stable@vger.kernel.org > > Index: linux-2.6/include/linux/wait_bit.h > =================================================================== > --- linux-2.6.orig/include/linux/wait_bit.h > +++ linux-2.6/include/linux/wait_bit.h > @@ -71,8 +71,10 @@ static inline int > wait_on_bit(unsigned long *word, int bit, unsigned mode) > { > might_sleep(); > - if (!test_bit(bit, word)) > + if (!test_bit(bit, word)) { > + smp_rmb(); Any new code using smp_rmb or an acquire access should always include a comment that explains where the matching smp_wmb or release access is. Alan Stern > return 0; > + } > return out_of_line_wait_on_bit(word, bit, > bit_wait, > mode); > @@ -96,8 +98,10 @@ static inline int > wait_on_bit_io(unsigned long *word, int bit, unsigned mode) > { > might_sleep(); > - if (!test_bit(bit, word)) > + if (!test_bit(bit, word)) { > + smp_rmb(); > return 0; > + } > return out_of_line_wait_on_bit(word, bit, > bit_wait_io, > mode); > @@ -123,8 +127,10 @@ wait_on_bit_timeout(unsigned long *word, > unsigned long timeout) > { > might_sleep(); > - if (!test_bit(bit, word)) > + if (!test_bit(bit, word)) { > + smp_rmb(); > return 0; > + } > return out_of_line_wait_on_bit_timeout(word, bit, > bit_wait_timeout, > mode, timeout); > @@ -151,8 +157,10 @@ wait_on_bit_action(unsigned long *word, > unsigned mode) > { > might_sleep(); > - if (!test_bit(bit, word)) > + if (!test_bit(bit, word)) { > + smp_rmb(); > return 0; > + } > return out_of_line_wait_on_bit(word, bit, action, mode); > } > > Index: linux-2.6/kernel/sched/wait_bit.c > =================================================================== > --- linux-2.6.orig/kernel/sched/wait_bit.c > +++ linux-2.6/kernel/sched/wait_bit.c > @@ -51,6 +51,8 @@ __wait_on_bit(struct wait_queue_head *wq > > finish_wait(wq_head, &wbq_entry->wq_entry); > > + smp_rmb(); > + > return ret; > } > EXPORT_SYMBOL(__wait_on_bit); >
On Sun, 31 Jul 2022, Linus Torvalds wrote: > On Sun, Jul 31, 2022 at 1:41 PM Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > - if (!test_bit(bit, word)) > > + if (!test_bit(bit, word)) { > > + smp_rmb(); > > Logically, I don't think that makes sense. > > Maybe you're checking the buffer being up-to-date before you *write* to it? None of the CPUs have speculative writes - so the write can't be moved before the "test_bit" function. So, we are only concerned about reads. > So smp_rmb() seems entirely wrong. > > I think it should consistently aim for just doing > > unsigned long state = smp_read_acquire(word); > if (!(state & (1 << bit))) > return 0; > > or whatever. > > We should strive to *not* add new uses of the legacy memory barriers. > They are garbage from last century when people didn't know better. > > Then people learnt to use acquire and release, and things improved. > Let's live in that improved world. > > Linus OK - I'm sending new patches that introduce the function test_bit_acquire. Mikulas
Index: linux-2.6/include/linux/wait_bit.h =================================================================== --- linux-2.6.orig/include/linux/wait_bit.h +++ linux-2.6/include/linux/wait_bit.h @@ -71,8 +71,10 @@ static inline int wait_on_bit(unsigned long *word, int bit, unsigned mode) { might_sleep(); - if (!test_bit(bit, word)) + if (!test_bit(bit, word)) { + smp_rmb(); return 0; + } return out_of_line_wait_on_bit(word, bit, bit_wait, mode); @@ -96,8 +98,10 @@ static inline int wait_on_bit_io(unsigned long *word, int bit, unsigned mode) { might_sleep(); - if (!test_bit(bit, word)) + if (!test_bit(bit, word)) { + smp_rmb(); return 0; + } return out_of_line_wait_on_bit(word, bit, bit_wait_io, mode); @@ -123,8 +127,10 @@ wait_on_bit_timeout(unsigned long *word, unsigned long timeout) { might_sleep(); - if (!test_bit(bit, word)) + if (!test_bit(bit, word)) { + smp_rmb(); return 0; + } return out_of_line_wait_on_bit_timeout(word, bit, bit_wait_timeout, mode, timeout); @@ -151,8 +157,10 @@ wait_on_bit_action(unsigned long *word, unsigned mode) { might_sleep(); - if (!test_bit(bit, word)) + if (!test_bit(bit, word)) { + smp_rmb(); return 0; + } return out_of_line_wait_on_bit(word, bit, action, mode); } Index: linux-2.6/kernel/sched/wait_bit.c =================================================================== --- linux-2.6.orig/kernel/sched/wait_bit.c +++ linux-2.6/kernel/sched/wait_bit.c @@ -51,6 +51,8 @@ __wait_on_bit(struct wait_queue_head *wq finish_wait(wq_head, &wbq_entry->wq_entry); + smp_rmb(); + return ret; } EXPORT_SYMBOL(__wait_on_bit);
wait_on_bit tests the bit without any memory barriers, consequently the code that follows wait_on_bit may be moved before testing the bit on architectures with weak memory ordering. When the code tests for some event using wait_on_bit and then performs a load operation, the load may be unexpectedly moved before wait_on_bit and it may return data that existed before the event occurred. Such bugs exist in fs/buffer.c:__wait_on_buffer, drivers/md/dm-bufio.c:new_read, drivers/media/usb/dvb-usb-v2/dvb_usb_core.c:dvb_usb_start_feed, drivers/bluetooth/btusb.c:btusb_mtk_hci_wmt_sync and perhaps in other places. We fix this class of bugs by adding a read barrier after test_bit(). Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> Cc: stable@vger.kernel.org