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, 21 Jul 2015 23:43:34 +0200
From:	Sven Brauch <mail@...nbrauch.de>
To:	Oliver Neukum <oneukum@...e.com>
Cc:	Johan Hovold <johan@...nel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Peter Hurley <peter@...leysoftware.com>,
	Toby Gray <toby.gray@...lvnc.com>, linux-usb@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] Fix data loss in cdc-acm

Hi,

Thank you for your comments.

On 21/07/15 15:43, Oliver Neukum wrote:
> But others won't and we'd preserve stale data in preference over fresh
> data.
If that is important for your device, you should be using an isochronous
endpoint, not bulk, no?
Also note that the driver currently does this anyways. It loses a few kB
of data, and _then_ it throttles the producer and forces it to wait.

On 21/07/15 11:18, Johan Hovold wrote:
> In general if data isn't being consumed fast enough we eventually need
> to drop it (unless we can throttle the producer).
Yes, maybe this is the first thing which should be cleared up: Is
"throttle the producer" always preferable over "lose data"? I'd say yes
for bulk transfers, no for isochronous. It is in principle easy enough
to throttle the producer, that is what e.g. my patch does. Whether a
different approach may be more appropriate than the "don't resubmit the
urbs" thing is then of course open to debate.

As far as I can see, throttling the producer is the only way to
guarantee data delivery. So if we want that (and I certainly want it for
my application, I don't know about the general case), I think all
changes to the tty buffers or throttling mechanisms are "just"
performance optimization, since no such modification will ever guarantee
delivery if the producer is not throttled in time.
And, this I want to mention again, if your producer is timing-sensitive
you would not be using bulk anyways. The USB controller could just
decide that your device cannot send data for the next five seconds, and
it will have to handle that case as well. Thus I see no reason to not
throttle the producer if necessary.

On 21/07/15 18:45, Peter Hurley wrote:
> 1. Instrument acm_read_bulk_callback with tty_buffer_space_avail()
> to track the (approx) fill level of the tty buffers. I would
> use ftrace/trace_printk() to record this.
I already did this while debugging. For a while, the buffer is almost
empty (fluctuates by a few kB), then it rapidly drops to zero bytes
free. Only after a few urbs where submitted (or rather, not submitted)
into the full buffer, the throttle flag gets set.

Thank you for your offer to help figuring out what exactly goes wrong
with the throttling mechanism. If it turns out we should actually look
for a fix there, I'll be happy to make use of it.

Best,
Sven

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)