linux-kernel - Re: [PATCH 1/2] usb: gadget: f_tcm: Use auto-cleanup for usb

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]

Message-ID: <2025103010-awkward-gentleman-150d@gregkh>
Date: Thu, 30 Oct 2025 16:33:33 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Kuen-Han Tsai <khtsai@...gle.com>
Cc: Thinh Nguyen <Thinh.Nguyen@...opsys.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] usb: gadget: f_tcm: Use auto-cleanup for usb_request

On Thu, Oct 30, 2025 at 11:14:19PM +0800, Kuen-Han Tsai wrote:
> Refactor f_tcm.c to use auto-cleanup mechanism for usb_request
> allocations in bot_prepare_reqs(), uasp_alloc_stream_res(), and
> uasp_alloc_cmd().

Using guards are great for new code, or for bug fixes, but please don't
just refactor code to use them for the sake of using them.  It makes it
hard to review and justify the churn, especially when there is almost no
code savings here at all.

> The explicit nullification of fu->..._req and stream->..._req pointers
> on error is no longer needed. This is safe because these pointers are
> only updated after all allocations within the function have succeeded.
> If an error occurs, the fu structure members retain their previous
> value, and the existing cleanup functions like bot_cleanup_old_alt() and
> uasp_cleanup_old_alt() already handle stale pointers in the fu
> structure.

This seems to imply this is really fragile, and tricky, and maybe not
worth it?

The comment you added kind of enforces that feeling:

> +	fu->bot_req_in = no_free_ptr(bot_req_in);
> +	fu->bot_req_out = no_free_ptr(bot_req_out);
> +
> +	/* This line is placed here because free_usb_request also frees its
> +	 * buffer, which in this case points to the static fu->bot_status.csw.
> +	 */

Which is "this line"?

> +	status_req->buf = &fu->bot_status.csw;

This one?

> +	status_req->length = US_BULK_CS_WRAP_LEN;

Or that one?

Using guards for buffers for other structures is rough, as you have seen
here, I don't really see the benefit at all, do you?

thanks,

greg k-h