linux-kernel - Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YBGg132/qWaPgjsI@hovoldconsulting.com>
Date:   Wed, 27 Jan 2021 18:20:23 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Anant Thazhemadam <anant.thazhemadam@...il.com>
Cc:     Johan Hovold <johan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Xu Wang <vulab@...as.ac.cn>,
        Liu Shixin <liushixin2@...wei.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the
 usb_control_msg_{send|recv}() API

On Wed, Jan 27, 2021 at 08:12:21PM +0530, Anant Thazhemadam wrote:
> 
> On 27/01/21 7:28 pm, Johan Hovold wrote:
> > On Wed, Jan 27, 2021 at 12:03:52AM +0530, Anant Thazhemadam wrote:
> >> The newer usb_control_msg_{send|recv}() API are an improvement on the
> >> existing usb_control_msg() as it ensures that a short read/write is treated
> > As I mentioned in my comments on v2, a short write has always been
> > treated as an error so you shouldn't imply that it wasn't here (and in
> > the other commit messages).
> 
> The newer API ensures that a short send/recv is an error, as they
> either complete the complete translation, or return an error, and
> remove the need for explicit return value checking that was previously
> expected to detect the short read/write (which wasn't present in a lot
> of places).

But my point is that this claim isn't factually correct; there has never
been a need to check for short *writes* (even if a few drivers have such
redundant checks).

> That's what I was trying to say. But if the commit message isn't
> representative of that, I'll try and modify it.

Just drop the bit about "short writes".

> Does this sound like a better commit message?
> 
> "The newer usb_control_msg_{send|recv}() API are an improvement on the
> existing usb_control_msg().

Even this is disputable; in some situations the usb_control_msg() is
still preferred as I hope my comments have shown.

Perhaps they are better described as "convenience wrappers" or similar
as the real gain from using these helpers is to replace a pattern like:

	f(data, ...) {
		buf = malloc(data);
		usb_control_msg(..., buf, ...);
		memcpy(data, buf, ...);
		kfree(buf);
	}

for when data is on the stack *and* you do not expect variable-length IN
transfers.

But as soon as a driver is able to reuse a single buffer for multiple
transfers or the data buffer is already DMA-able, usb_control_msg() may
still be a better choice.

> The new API ensures either the full translation is completed,
> or an error is returned. This ensures that all short send/recv are detected as

recv only

> errors even if there is no explicit return value checking performed.
> 
> The new API also allows us to use data off the stack, and don't require raw usb
> pipes to be created in the calling functions."

Johan