2021-09-29 10:44:33

by Johan Hovold

[permalink] [raw]
Subject: [PATCH 0/2] USB: cdc-acm: fix break reporting

This fixes a couple of bugs in the recently added support for reporting
break events.

Note that the offending commit was backported to stable.

Johan


Johan Hovold (2):
USB: cdc-acm: fix racy tty buffer accesses
USB: cdc-acm: fix break detection

drivers/usb/class/cdc-acm.c | 8 ++++++++
1 file changed, 8 insertions(+)

--
2.32.0


2021-09-29 11:45:12

by Johan Hovold

[permalink] [raw]
Subject: [PATCH 1/2] USB: cdc-acm: fix racy tty buffer accesses

A recent change that started reporting break events to the line
discipline caused the tty-buffer insertions to no longer be serialised
by inserting events also from the completion handler for the interrupt
endpoint.

Completion calls for distinct endpoints are not guaranteed to be
serialised. For example, in case a host-controller driver uses
bottom-half completion, the interrupt and bulk-in completion handlers
can end up running in parallel on two CPUs (high-and low-prio tasklets,
respectively) thereby breaking the tty layer's single producer
assumption.

Fix this by holding the read lock also when inserting characters from
the bulk endpoint.

Fixes: 08dff274edda ("cdc-acm: fix BREAK rx code path adding necessary calls")
Cc: [email protected]
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/class/cdc-acm.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 4e2f1552f4b7..c7a1736720e7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -475,11 +475,16 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t mem_flags)

static void acm_process_read_urb(struct acm *acm, struct urb *urb)
{
+ unsigned long flags;
+
if (!urb->actual_length)
return;

+ spin_lock_irqsave(&acm->read_lock, flags);
tty_insert_flip_string(&acm->port, urb->transfer_buffer,
urb->actual_length);
+ spin_unlock_irqrestore(&acm->read_lock, flags);
+
tty_flip_buffer_push(&acm->port);
}

--
2.32.0

2021-09-30 09:07:07

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH 1/2] USB: cdc-acm: fix racy tty buffer accesses


On 29.09.21 11:09, Johan Hovold wrote:
> A recent change that started reporting break events to the line
> discipline caused the tty-buffer insertions to no longer be serialised
> by inserting events also from the completion handler for the interrupt
> endpoint.
>
> Completion calls for distinct endpoints are not guaranteed to be
> serialised. For example, in case a host-controller driver uses
> bottom-half completion, the interrupt and bulk-in completion handlers
> can end up running in parallel on two CPUs (high-and low-prio tasklets,
> respectively) thereby breaking the tty layer's single producer
> assumption.
>
> Fix this by holding the read lock also when inserting characters from
> the bulk endpoint.
>
> Fixes: 08dff274edda ("cdc-acm: fix BREAK rx code path adding necessary calls")
> Cc: [email protected]
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Oliver Neukum <[email protected]>