USB: serial: fix unthrottle races

PATCH In review Johan Hovold 4 participants 12 messages Apr 25, 2019 – May 14, 2019

Subsystems (3)

Stable branch USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2019-04-25 16:05 UTC #0 lore raw

PATCH 1/5 USB: serial: fix unthrottle races

Fix two long-standing bugs which could potentially lead to memory corruption or leave the port throttled until it is reopened (on weakly ordered systems), respectively, when read-URB completion races with unthrottle().

First, the URB must not be marked as free before processing is complete to prevent it from being submitted by unthrottle() on another CPU.

	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  process_urb();
	  smp_mb__before_atomic();
	  set_bit(i, free);		  if (test_and_clear_bit(i, free))
	  					  submit_urb();

Second, the URB must be marked as free before checking the throttled flag to prevent unthrottle() on another CPU from failing to observe that the URB needs to be submitted if complete() sees that the throttled flag is set.

	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  set_bit(i, free);		  throttled = 0;
	  smp_mb__after_atomic();	  smp_mb();
	  if (throttled)		  if (test_and_clear_bit(i, free))
	  	  return;			  submit_urb();

Note that test_and_clear_bit() only implies barriers when the test is successful. To handle the case where the URB is still in use an explicit barrier needs to be added to unthrottle() for the second race condition.

Fixes d83b405383c9 ("USB: serial: add support for multiple read urbs")

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/usb/serial/generic.c | 39 +++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

drivers/usb/serial/generic.c +32 -7

index 2274d9625f63..0fff4968ea1b 100644
@@ -376,6 +376,7 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
 	struct usb_serial_port *port = urb->context;
 	unsigned char *data = urb->transfer_buffer;
 	unsigned long flags;
+	bool stopped = false;
 	int status = urb->status;
 	int i;

@@ -383,33 +384,51 @@ void usb_serial_generic_read_bulk_callback(struct urb *urb)
 		if (urb == port->read_urbs[i])
 			break;
 	}
-	set_bit(i, &port->read_urbs_free);
 
 	dev_dbg(&port->dev, "%s - urb %d, len %d\n", __func__, i,
 							urb->actual_length);
 	switch (status) {
 	case 0:
+		usb_serial_debug_data(&port->dev, __func__, urb->actual_length,
+							data);
+		port->serial->type->process_read_urb(urb);
 		break;
 	case -ENOENT:
 	case -ECONNRESET:
 	case -ESHUTDOWN:
 		dev_dbg(&port->dev, "%s - urb stopped: %d\n",
 							__func__, status);
-		return;
+		stopped = true;
+		break;
 	case -EPIPE:
 		dev_err(&port->dev, "%s - urb stopped: %d\n",
 							__func__, status);
-		return;
+		stopped = true;
+		break;
 	default:
 		dev_dbg(&port->dev, "%s - nonzero urb status: %d\n",
 							__func__, status);
-		goto resubmit;
+		break;
 	}
 
-	usb_serial_debug_data(&port->dev, __func__, urb->actual_length, data);
-	port->serial->type->process_read_urb(urb);
+	/*
+	 * Make sure URB processing is done before marking as free to avoid
+	 * racing with unthrottle() on another CPU. Matches the barriers
+	 * implied by the test_and_clear_bit() in
+	 * usb_serial_generic_submit_read_urb().
+	 */
+	smp_mb__before_atomic();
+	set_bit(i, &port->read_urbs_free);
+	/*
+	 * Make sure URB is marked as free before checking the throttled flag
+	 * to avoid racing with unthrottle() on another CPU. Matches the
+	 * smp_mb() in unthrottle().
+	 */
+	smp_mb__after_atomic();
+
+	if (stopped)
+		return;
 
-resubmit:
 	/* Throttle the device if requested by tty */
 	spin_lock_irqsave(&port->lock, flags);
 	port->throttled = port->throttle_req;

@@ -484,6 +503,12 @@ void usb_serial_generic_unthrottle(struct tty_struct *tty)
 	port->throttled = port->throttle_req = 0;
 	spin_unlock_irq(&port->lock);
 
+	/*
+	 * Matches the smp_mb__after_atomic() in
+	 * usb_serial_generic_read_bulk_callback().
+	 */
+	smp_mb();
+
 	if (was_throttled)
 		usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
 }

Johan Hovold

author maintainer

2019-05-13 10:43 UTC #1 lore raw

Quoted 29 lines · Johan Hovold

Fix two long-standing bugs which could potentially lead to memory corruption or leave the port throttled until it is reopened (on weakly ordered systems), respectively, when read-URB completion races with unthrottle().
First, the URB must not be marked as free before processing is complete to prevent it from being submitted by unthrottle() on another CPU.
	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  process_urb();
	  smp_mb__before_atomic();
	  set_bit(i, free);		  if (test_and_clear_bit(i, free))
	  					  submit_urb();
Second, the URB must be marked as free before checking the throttled flag to prevent unthrottle() on another CPU from failing to observe that the URB needs to be submitted if complete() sees that the throttled flag is set.
	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  set_bit(i, free);		  throttled = 0;
	  smp_mb__after_atomic();	  smp_mb();
	  if (throttled)		  if (test_and_clear_bit(i, free))
	  	  return;			  submit_urb();

Quoted 6 lines

Note that test_and_clear_bit() only implies barriers when the test is successful. To handle the case where the URB is still in use an explicit barrier needs to be added to unthrottle() for the second race condition.
Fixes d83b405383c9 ("USB: serial: add support for multiple read urbs")
Signed-off-by Johan Hovold <[email protected]>

Greg, I noticed you added a stable tag to the corresponding cdc-acm fix and think I should have added on one from the start to this one as well.

Would you mind queuing this one up for stable?

Upstream commit 3f5edd58d040bfa4b74fb89bc02f0bc6b9cd06ab.

Thanks,
Johan

Greg Kroah-Hartman

maintainer

2019-05-13 10:56 UTC #2 lore raw

Quoted 37 lines · Johan Hovold

Quoted 29 lines · Johan Hovold
Fix two long-standing bugs which could potentially lead to memory corruption or leave the port throttled until it is reopened (on weakly ordered systems), respectively, when read-URB completion races with unthrottle().
First, the URB must not be marked as free before processing is complete to prevent it from being submitted by unthrottle() on another CPU.
	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  process_urb();
	  smp_mb__before_atomic();
	  set_bit(i, free);		  if (test_and_clear_bit(i, free))
	  					  submit_urb();
Second, the URB must be marked as free before checking the throttled flag to prevent unthrottle() on another CPU from failing to observe that the URB needs to be submitted if complete() sees that the throttled flag is set.
	CPU 1				CPU 2
	================		================
	complete()			unthrottle()
	  set_bit(i, free);		  throttled = 0;
	  smp_mb__after_atomic();	  smp_mb();
	  if (throttled)		  if (test_and_clear_bit(i, free))
	  	  return;			  submit_urb();
Quoted 6 lines
Note that test_and_clear_bit() only implies barriers when the test is successful. To handle the case where the URB is still in use an explicit barrier needs to be added to unthrottle() for the second race condition.
Fixes d83b405383c9 ("USB: serial: add support for multiple read urbs")
Signed-off-by Johan Hovold <[email protected]>

Quoted 6 lines

Greg, I noticed you added a stable tag to the corresponding cdc-acm fix and think I should have added on one from the start to this one as well.
Would you mind queuing this one up for stable?
Upstream commit 3f5edd58d040bfa4b74fb89bc02f0bc6b9cd06ab.

Sure, now queued up for 4.9+

thanks,

greg k-h

Johan Hovold

author maintainer

2019-05-13 11:46 UTC #3 lore raw

Quoted 10 lines · Greg Kroah-Hartman

Quoted 9 lines · Johan Hovold
Quoted 7 lines · Johan Hovold
Fix two long-standing bugs which could potentially lead to memory corruption or leave the port throttled until it is reopened (on weakly ordered systems), respectively, when read-URB completion races with unthrottle().
Fixes d83b405383c9 ("USB: serial: add support for multiple read urbs")
Signed-off-by Johan Hovold <[email protected]>

Quoted 8 lines

Quoted 6 lines
Greg, I noticed you added a stable tag to the corresponding cdc-acm fix and think I should have added on one from the start to this one as well.
Would you mind queuing this one up for stable?
Upstream commit 3f5edd58d040bfa4b74fb89bc02f0bc6b9cd06ab.
Sure, now queued up for 4.9+

Thanks. The issue has been there since v3.3 so I guess you could queue it for all stable trees.

Johan

Greg Kroah-Hartman

maintainer

2019-05-13 12:51 UTC #4 lore raw

Quoted 14 lines · Johan Hovold

Quoted 13 lines · Greg Kroah-Hartman
Quoted 12 lines · Johan Hovold
Quoted 7 lines · Johan Hovold
Fix two long-standing bugs which could potentially lead to memory corruption or leave the port throttled until it is reopened (on weakly ordered systems), respectively, when read-URB completion races with unthrottle().
Fixes d83b405383c9 ("USB: serial: add support for multiple read urbs")
Signed-off-by Johan Hovold <[email protected]>
Greg, I noticed you added a stable tag to the corresponding cdc-acm fix and think I should have added on one from the start to this one as well.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Would you mind queuing this one up for stable?
Upstream commit 3f5edd58d040bfa4b74fb89bc02f0bc6b9cd06ab.
Sure, now queued up for 4.9+
Thanks. The issue has been there since v3.3 so I guess you could queue it for all stable trees.

Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth adding there (and I kind of doubt it), I would need a backport :)

thanks,

greg k-h

Johan Hovold

author maintainer

2019-05-13 12:59 UTC #5 lore raw

Quoted 7 lines · Greg Kroah-Hartman

Quoted 2 lines · Johan Hovold
Thanks. The issue has been there since v3.3 so I guess you could queue it for all stable trees.
Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth adding there (and I kind of doubt it), I would need a backport :)

Ah ok, just wasn't sure why you chose 4.9+.

Johan

Sasha Levin

maintainer

2019-05-14 12:53 UTC #6 lore raw

Quoted 10 lines · Johan Hovold

Quoted 7 lines · Greg Kroah-Hartman
Quoted 2 lines · Johan Hovold
Thanks. The issue has been there since v3.3 so I guess you could queue it for all stable trees.
Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth adding there (and I kind of doubt it), I would need a backport :)
Ah ok, just wasn't sure why you chose 4.9+.

On 4.4 and 3.18 it just had a contextual conflict because of 3161da970d38c ("USB: serial: use variable for status"), I can queue both 3161da970d38c and 3f5edd58d040b for 4.4 and 3.18.

--
Thanks,
Sasha

Johan Hovold

author maintainer

2019-05-14 12:57 UTC #7 lore raw

Quoted 7 lines · Sasha Levin

Quoted 6 lines · Johan Hovold
Quoted 5 lines · Greg Kroah-Hartman
Quoted 2 lines · Johan Hovold
Thanks. The issue has been there since v3.3 so I guess you could queue it for all stable trees.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Doesn't apply cleanly for 4.4.y or 3.18.y, so if it's really worth adding there (and I kind of doubt it), I would need a backport :)
Ah ok, just wasn't sure why you chose 4.9+.
On 4.4 and 3.18 it just had a contextual conflict because of 3161da970d38c ("USB: serial: use variable for status"), I can queue both 3161da970d38c and 3f5edd58d040b for 4.4 and 3.18.

Sounds good, thanks!

Johan

Johan Hovold

author maintainer

2019-04-25 16:06 UTC #8 lore raw

cover USB: fix tty unthrottle races

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().

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(-)

Signature · 1 line

2.21.0

Alan Stern

2019-04-25 20:58 UTC #9 lore raw

Re: [PATCH 0/5] USB: fix tty unthrottle races

Quoted 13 lines · Johan Hovold

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.

Quoted 8 lines

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

Quoted 24 lines

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(-)

Johan Hovold

author maintainer

2019-04-26 04:55 UTC #10 lore raw

Re: [PATCH 0/5] USB: fix tty unthrottle races

Quoted 24 lines · Alan Stern

Quoted 13 lines · Johan Hovold
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.
Quoted 8 lines
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().

Quoted 4 lines

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

Johan Hovold

author maintainer

2019-04-29 09:30 UTC #11 lore raw

Re: [PATCH 0/5] USB: fix tty unthrottle races

In reply to Johan Hovold

Quoted 8 lines · Johan Hovold

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.

Quoted 6 lines

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