linux-kernel - Re: [PATCH] Fix data loss in cdc-acm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 04 Aug 2015 20:26:57 -0400
From:	Peter Hurley <peter@...leysoftware.com>
To:	Oliver Neukum <oneukum@...e.com>
CC:	Johan Hovold <johan@...nel.org>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Toby Gray <toby.gray@...lvnc.com>,
	Sven Brauch <mail@...nbrauch.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-serial@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] Fix data loss in cdc-acm

On 07/22/2015 11:01 AM, Oliver Neukum wrote:
> On Wed, 2015-07-22 at 10:30 -0400, Peter Hurley wrote:
>> 3. Pre-allocate space _before_ the data arrives (with
>> tty_buffer_request_room());
>>    this is applicable to subsystems which know how much data can be
>> in-flight
>>    at any one time. This guarantees that when rx data arrives buffer
>> space is
>>    available (since it has already been allocated).
>>
>> Drivers that use method 2 typically attempt to recopy the buffered
>> data
>> when either new data arrives or @ unthrottle. I've seen others use
>> deferred
>> work as well.
>>
>> AFAIK no driver/subsystem is using method 3 for guaranteed delivery
>> of in-flight data, but it seems ideally suited to usb serial.
> 
> Indeed. But flow control is still done by throttle/unthrottle, isn't it?

Oliver, somehow I never saw this. Please accept my apologies.

Well, input flow control from the driver would be implicitly limited
by a failure to allocate new buffer space after the old buffer was
inserted and pushed. In the case of urbs, this would simply mean that
the urb could not be resubmitted yet, and would have to be deferred.
So preventing buffer overrun wouldn't rely on throttle.

WRT flow control, method 3 only needs to know when to resume.
As with method 2, this could be @ unthrottle or by retry in deferred work
(essentially polling). At least that's what's currently available.

Regardless, tty drivers should still respond to throttle/unthrottle
requests so as to limit input data when there is no active reader.

All that being said, unthrottle behaves degenerately at very high line
rates and throughput, which needs to be resolved.

The central issue is that the wrong thread is evaluating the wrong
conditions for restart. A further complication is that unthrottle
is guaranteed race-free reads of struct termios which limit the contexts
from which unthrottle can be called.

So to recap; the tty buffer api already provides the necessary interface
for preventing buffer overrun and drivers/subsystems with very high line
rates should be utilizing that interface instead of relying on throttle.

In addition, a more reliable/appropriate method of restart is required
(preferably by fixing when/why unthrottle is invoked). I'll look into that.

Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/