[PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

The wakeup condition needs to be checked after adding the task to the
waitqueue and updating the task state to avoid missing a racing modem
status update or disconnect.

Fixes: 5a6a62bdb925 ("cdc-acm: add TIOCMIWAIT")
Cc: stable@vger.kernel.org	# 3.14
Cc: Oliver Neukum <oneukum@suse.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/class/cdc-acm.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7bc5329fa3ed..00c250515e83 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1027,7 +1027,10 @@ static int wait_serial_change(struct acm *acm, unsigned long arg)
 	DECLARE_WAITQUEUE(wait, current);
 	struct async_icount old, new;
 
-	do {
+	add_wait_queue(&acm->wioctl, &wait);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
 		spin_lock_irq(&acm->read_lock);
 		old = acm->oldcount;
 		new = acm->iocount;
@@ -1044,22 +1047,20 @@ static int wait_serial_change(struct acm *acm, unsigned long arg)
 			old.rng != new.rng)
 			break;
 
-		add_wait_queue(&acm->wioctl, &wait);
-		set_current_state(TASK_INTERRUPTIBLE);
-		schedule();
-		remove_wait_queue(&acm->wioctl, &wait);
 		if (acm->disconnected) {
-			if (arg & TIOCM_CD)
-				break;
-			else
-				rv = -ENODEV;
-		} else {
-			if (signal_pending(current))
-				rv = -ERESTARTSYS;
+			rv = -ENODEV;
+			break;
 		}
-	} while (!rv);
 
-	
+		schedule();
+
+		if (signal_pending(current)) {
+			rv = -ERESTARTSYS;
+			break;
+		}
+	}
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(&acm->wioctl, &wait);
 
 	return rv;
 }
-- 
2.55.0

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 07.09.26 08:55, Johan Hovold wrote:
> The wakeup condition needs to be checked after adding the task to the
> waitqueue and updating the task state to avoid missing a racing modem
> status update or disconnect.

I am sorry, but in this case I have to ask:
Isn't TIOCMIWAIT inherently racy against the hardware?

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
> On 07.09.26 08:55, Johan Hovold wrote:
> > The wakeup condition needs to be checked after adding the task to the
> > waitqueue and updating the task state to avoid missing a racing modem
> > status update or disconnect.
> 
> I am sorry, but in this case I have to ask:
> Isn't TIOCMIWAIT inherently racy against the hardware?

What do you mean?

Looking at the implementation again now, it seems further changes are
needed to fix the implementation, though. The completion handler should
not be updating old_count. That's for each TIOCMIWAIT to do.

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Mon, Sep 07, 2026 at 10:47:49AM +0200, Johan Hovold wrote:
> On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
> > On 07.09.26 08:55, Johan Hovold wrote:
> > > The wakeup condition needs to be checked after adding the task to the
> > > waitqueue and updating the task state to avoid missing a racing modem
> > > status update or disconnect.
> > 
> > I am sorry, but in this case I have to ask:
> > Isn't TIOCMIWAIT inherently racy against the hardware?
> 
> What do you mean?

Ok, I think I see what you're getting at. Nothing prevents a modem
status change just before calling TIOCMIWAIT from being missed. That is
indeed a general problem with the interface.

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 07.09.26 10:47, Johan Hovold wrote:
> On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
>> On 07.09.26 08:55, Johan Hovold wrote:
>>> The wakeup condition needs to be checked after adding the task to the
>>> waitqueue and updating the task state to avoid missing a racing modem
>>> status update or disconnect.
>>
>> I am sorry, but in this case I have to ask:
>> Isn't TIOCMIWAIT inherently racy against the hardware?
> 
> What do you mean?

The interface does not state from or to which state it changed.
Now you could say that it is implicitly the last time TIOCMGET
was called, but that is

1. not clearly stated
2. not enforceable
  
> Looking at the implementation again now, it seems further changes are
> needed to fix the implementation, though. The completion handler should
> not be updating old_count. That's for each TIOCMIWAIT to do.

I am sorry to be obnoxious about that, but I am afraid that would
break TIOCGICOUNT

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Mon, Sep 07, 2026 at 11:50:41AM +0200, Oliver Neukum wrote:
> On 07.09.26 10:47, Johan Hovold wrote:
> > On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
> >> On 07.09.26 08:55, Johan Hovold wrote:
> >>> The wakeup condition needs to be checked after adding the task to the
> >>> waitqueue and updating the task state to avoid missing a racing modem
> >>> status update or disconnect.
> >>
> >> I am sorry, but in this case I have to ask:
> >> Isn't TIOCMIWAIT inherently racy against the hardware?
> > 
> > What do you mean?
> 
> The interface does not state from or to which state it changed.
> Now you could say that it is implicitly the last time TIOCMGET
> was called, but that is
> 
> 1. not clearly stated
> 2. not enforceable

Right. The original implementation (and serial core) have always stored
the counters on entry and compared to that. So that is the defacto
semantics.

> > Looking at the implementation again now, it seems further changes are
> > needed to fix the implementation, though. The completion handler should
> > not be updating old_count. That's for each TIOCMIWAIT to do.
> 
> I am sorry to be obnoxious about that, but I am afraid that would
> break TIOCGICOUNT

Why do you think so? TIOCGICOUNT just returns the current counters.

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 07.09.26 12:01, Johan Hovold wrote:
> On Mon, Sep 07, 2026 at 11:50:41AM +0200, Oliver Neukum wrote:
>> On 07.09.26 10:47, Johan Hovold wrote:
>>> On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
>>>> On 07.09.26 08:55, Johan Hovold wrote:

[..]
>>> Looking at the implementation again now, it seems further changes are
>>> needed to fix the implementation, though. The completion handler should
>>> not be updating old_count. That's for each TIOCMIWAIT to do.
>>
>> I am sorry to be obnoxious about that, but I am afraid that would
>> break TIOCGICOUNT
> 
> Why do you think so? TIOCGICOUNT just returns the current counters.

Yes, but user space can call TIOCGICOUNT multiple times in sequence
_without_ calling TIOCMIWAIT in between.
In that case the counters have to increase if a line changes, don't they?
For that to happen you need to handle a status change in the completion
handler.

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Mon, Sep 07, 2026 at 12:28:46PM +0200, Oliver Neukum wrote:
> 
> 
> On 07.09.26 12:01, Johan Hovold wrote:
> > On Mon, Sep 07, 2026 at 11:50:41AM +0200, Oliver Neukum wrote:
> >> On 07.09.26 10:47, Johan Hovold wrote:
> >>> On Mon, Sep 07, 2026 at 10:17:38AM +0200, Oliver Neukum wrote:
> >>>> On 07.09.26 08:55, Johan Hovold wrote:
> 
> [..]
> >>> Looking at the implementation again now, it seems further changes are
> >>> needed to fix the implementation, though. The completion handler should
> >>> not be updating old_count. That's for each TIOCMIWAIT to do.
> >>
> >> I am sorry to be obnoxious about that, but I am afraid that would
> >> break TIOCGICOUNT
> > 
> > Why do you think so? TIOCGICOUNT just returns the current counters.
> 
> Yes, but user space can call TIOCGICOUNT multiple times in sequence
> _without_ calling TIOCMIWAIT in between.
> In that case the counters have to increase if a line changes, don't they?
> For that to happen you need to handle a status change in the completion
> handler.

Yes, the completion handler needs to handle the status change (i.e.
compare new and previous status and increment the counters), but it
shouldn't update any old *icount* structure.

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 07.09.26 13:38, Johan Hovold wrote:
> On Mon, Sep 07, 2026 at 12:28:46PM +0200, Oliver Neukum wrote:
>>
>>
>> On 07.09.26 12:01, Johan Hovold wrote:
>>> On Mon, Sep 07, 2026 at 11:50:41AM +0200, Oliver Neukum wrote:

>> Yes, but user space can call TIOCGICOUNT multiple times in sequence
>> _without_ calling TIOCMIWAIT in between.
>> In that case the counters have to increase if a line changes, don't they?
>> For that to happen you need to handle a status change in the completion
>> handler.
> 
> Yes, the completion handler needs to handle the status change (i.e.
> compare new and previous status and increment the counters), but it
> shouldn't update any old *icount* structure.

We could do that and it would indeed be simpler.

But it would change the semantics so that past status changes
no longer show up in TIOCMIWAIT. Is that really a good idea?

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Mon, Sep 07, 2026 at 02:46:16PM +0200, Oliver Neukum wrote:
> 
> 
> On 07.09.26 13:38, Johan Hovold wrote:
> > On Mon, Sep 07, 2026 at 12:28:46PM +0200, Oliver Neukum wrote:
> >>
> >>
> >> On 07.09.26 12:01, Johan Hovold wrote:
> >>> On Mon, Sep 07, 2026 at 11:50:41AM +0200, Oliver Neukum wrote:
> 
> >> Yes, but user space can call TIOCGICOUNT multiple times in sequence
> >> _without_ calling TIOCMIWAIT in between.
> >> In that case the counters have to increase if a line changes, don't they?
> >> For that to happen you need to handle a status change in the completion
> >> handler.
> > 
> > Yes, the completion handler needs to handle the status change (i.e.
> > compare new and previous status and increment the counters), but it
> > shouldn't update any old *icount* structure.
> 
> We could do that and it would indeed be simpler.
> 
> But it would change the semantics so that past status changes
> no longer show up in TIOCMIWAIT. Is that really a good idea?

Yes, as the current behaviour is both non-standard and inconsistent.

If you have multiple tasks waiting waiting on status change, then only
one of them will detect it, for example.

And if there are multiple changes (before or during TIOCMIWAIT) events
may be lost too.

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 07.09.26 15:03, Johan Hovold wrote:
> On Mon, Sep 07, 2026 at 02:46:16PM +0200, Oliver Neukum wrote:
>>

>> We could do that and it would indeed be simpler.
>>
>> But it would change the semantics so that past status changes
>> no longer show up in TIOCMIWAIT. Is that really a good idea?
> 
> Yes, as the current behaviour is both non-standard and inconsistent.
> 
> If you have multiple tasks waiting waiting on status change, then only
> one of them will detect it, for example.
> 
> And if there are multiple changes (before or during TIOCMIWAIT) events
> may be lost too.

Very well. Ample reasons. Do you have a patch or do you want me to fix it?

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Tue, Sep 08, 2026 at 08:54:36AM +0200, Oliver Neukum wrote:
> 
> 
> On 07.09.26 15:03, Johan Hovold wrote:
> > On Mon, Sep 07, 2026 at 02:46:16PM +0200, Oliver Neukum wrote:
> >>
> 
> >> We could do that and it would indeed be simpler.
> >>
> >> But it would change the semantics so that past status changes
> >> no longer show up in TIOCMIWAIT. Is that really a good idea?
> > 
> > Yes, as the current behaviour is both non-standard and inconsistent.
> > 
> > If you have multiple tasks waiting waiting on status change, then only
> > one of them will detect it, for example.
> > 
> > And if there are multiple changes (before or during TIOCMIWAIT) events
> > may be lost too.
> 
> Very well. Ample reasons. Do you have a patch or do you want me to fix it?

I sent a v2 yesterday that should take care of it all:

	https://lore.kernel.org/all/20260907095130.130636-1-johan@kernel.org/

Johan

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Oliver Neukum]

On 08.09.26 09:12, Johan Hovold wrote:
> On Tue, Sep 08, 2026 at 08:54:36AM +0200, Oliver Neukum wrote:
>>
>>

>> Very well. Ample reasons. Do you have a patch or do you want me to fix it?
> 
> I sent a v2 yesterday that should take care of it all:
> 
> 	https://lore.kernel.org/all/20260907095130.130636-1-johan@kernel.org/

Thank you. I misinterpreted your intent. Sorry.

I am afraid that patch has a few weaknesses.

1. It mixes changes to measuring the old count and to how the sleeping works
2. If you want to change the logic at all, then we have a first check for changes
that is almost sure to fail.

It seems to me that if you are going to touch the logic there at all, the
sensible order of actions in the loop would be

A - check for signals
B - schedule
C - check for changes

That way we

i   - avoid calling into the scheduler if signals are pending
ii  - avoid returning -ERESTARTSYS if we have a result we can report
iii - avoid rechecking for changes after only a few dozen cycles
We have an inherent race against the hardware. The chance that we don't
have to sleep is essentially zero.

	Regards
		Oliver

Re: [PATCH] USB: cdc-acm: fix racy TIOCMIWAIT implementation

[Johan Hovold]

On Tue, Sep 08, 2026 at 11:23:38AM +0200, Oliver Neukum wrote:
> On 08.09.26 09:12, Johan Hovold wrote:

> > I sent a v2 yesterday that should take care of it all:
> > 
> > 	https://lore.kernel.org/all/20260907095130.130636-1-johan@kernel.org/
> 
> Thank you. I misinterpreted your intent. Sorry.
> 
> I am afraid that patch has a few weaknesses.
> 
> 1. It mixes changes to measuring the old count and to how the sleeping works

It could possibly be split in two (i.e. my v1 + a separate change to
drop the old count), but the commit fixes the "racy tiocmiwait
implementation" and all of these races were there from the start. So it
does not seem necessary to break it up.

> 2. If you want to change the logic at all, then we have a first check for changes
> that is almost sure to fail.
>
> It seems to me that if you are going to touch the logic there at all, the
> sensible order of actions in the loop would be
> 
> A - check for signals
> B - schedule
> C - check for changes
> 
> That way we
> 
> i   - avoid calling into the scheduler if signals are pending
> ii  - avoid returning -ERESTARTSYS if we have a result we can report

This would be further changes to the current behaviour, which is also
reflected in serial core. Can possibly be done on top.

> iii - avoid rechecking for changes after only a few dozen cycles
> We have an inherent race against the hardware. The chance that we don't
> have to sleep is essentially zero.

Sure, but the code is more readable this way with a standard wait loop
(and it highlights the limitations of the interface).

Johan