| Message ID | 20190425160540.10036-1-johan@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: fix tty unthrottle races | expand |
On Thu, 25 Apr 2019, Johan Hovold wrote: > This series fixes a couple of long-standing issues in USB serial and > cdc-acm which essentially share the same implementation. > > As noted by Oliver a few years back, read-urb completion can race with > unthrottle() running on another CPU and this can potentially lead to > memory corruption. This particular bug in cdc-acm was unfortunately > reintroduced a year later. > > There's also a second race due to missing memory barriers which could > theoretically lead to the port staying throttled until reopened on > weakly ordered systems. A second set of memory barriers should address > that. > > I would appreciate your keen eyes on this one to make sure I got the > barriers right. > > I noticed there's some on-going discussion about the atomic memory > barriers that Alan's involved in, and I'll try to catch up on his > data-race work as well. I'm still a little concerned about whether the > smp_mb__before_atomic() is sufficient to prevent the compiler from > messing things up without adding READ_ONCE(). I think your changes in patches 1 and 4 are correct. Regardless of the issues still undergoing discussion elsewhere, smp_mb__before_atomic() should cause both the compiler and the CPU to order every preceding operation (READ_ONCE or not) before the atomic op. Alan Stern > Note that none of these have stable tags as the issues have been there > for eight years or so without anyone noticing (besides Oliver). > > Still feels good to clean up your own mess. > > Note that the cdc-acm patches have so far only been compile tested. > > Johan > > > Johan Hovold (5): > USB: serial: fix unthrottle races > USB: serial: clean up throttle handling > USB: serial: generic: drop unnecessary goto > USB: cdc-acm: fix unthrottle races > USB: cdc-acm: clean up throttle handling > > drivers/usb/class/cdc-acm.c | 63 +++++++++++++++--------------- > drivers/usb/class/cdc-acm.h | 3 +- > drivers/usb/serial/generic.c | 76 +++++++++++++++++++----------------- > include/linux/usb/serial.h | 5 +-- > 4 files changed, 75 insertions(+), 72 deletions(-) > >
On Thu, Apr 25, 2019 at 04:58:20PM -0400, Alan Stern wrote: > On Thu, 25 Apr 2019, Johan Hovold wrote: > > > This series fixes a couple of long-standing issues in USB serial and > > cdc-acm which essentially share the same implementation. > > > > As noted by Oliver a few years back, read-urb completion can race with > > unthrottle() running on another CPU and this can potentially lead to > > memory corruption. This particular bug in cdc-acm was unfortunately > > reintroduced a year later. > > > > There's also a second race due to missing memory barriers which could > > theoretically lead to the port staying throttled until reopened on > > weakly ordered systems. A second set of memory barriers should address > > that. > > > > I would appreciate your keen eyes on this one to make sure I got the > > barriers right. > > > > I noticed there's some on-going discussion about the atomic memory > > barriers that Alan's involved in, and I'll try to catch up on his > > data-race work as well. I'm still a little concerned about whether the > > smp_mb__before_atomic() is sufficient to prevent the compiler from > > messing things up without adding READ_ONCE(). > > I think your changes in patches 1 and 4 are correct. Regardless of the > issues still undergoing discussion elsewhere, smp_mb__before_atomic() > should cause both the compiler and the CPU to order every preceding > operation (READ_ONCE or not) before the atomic op. Ok, thanks for taking a look! Johan
On Thu, Apr 25, 2019 at 06:05:35PM +0200, Johan Hovold wrote: > This series fixes a couple of long-standing issues in USB serial and > cdc-acm which essentially share the same implementation. > > As noted by Oliver a few years back, read-urb completion can race with > unthrottle() running on another CPU and this can potentially lead to > memory corruption. This particular bug in cdc-acm was unfortunately > reintroduced a year later. > > There's also a second race due to missing memory barriers which could > theoretically lead to the port staying throttled until reopened on > weakly ordered systems. A second set of memory barriers should address > that. > Note that the cdc-acm patches have so far only been compile tested. I've tested also the cdc-acm changes now. So unless anyone complains, I'll apply the USB-serial ones in a few days, and maybe Greg can pick up the cdc-acm patches. Johan