linux-kernel - Re: [PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private data

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] [day] [month] [year] [list]

Message-ID: <45b6468c.9346.196f76c5461.Coremail.00107082@163.com>
Date: Thu, 22 May 2025 17:56:34 +0800 (CST)
From: "David Wang" <00107082@....com>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: mathias.nyman@...el.com, oneukum@...e.com, stern@...land.harvard.edu,
	hminas@...opsys.com, rui.silva@...aro.org, jgross@...e.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] USB: xhci: use urb hcpriv mempool for private
 data

At 2025-05-22 16:32:40, "Greg KH" <gregkh@...uxfoundation.org> wrote:
>On Thu, May 22, 2025 at 03:10:10PM +0800, David Wang wrote:
>> xhci keeps alloc/free private data for each enqueue/dequeue cycles,
>> when using a USB webcam, allocation rate is ~250/s;
>> when using a USB mic, allocation rate reaches ~1k/s;
>> The more usb device in use, the higher allocation rate.
>> 
>> URB objects have longer lifespan than private data, hand over ownership
>> of private data to urb can save lots of memory allocations over time.
>> With this change, no extra memory allocation is needed during usages of
>> USB webcam/mic.
>> 
>> Signed-off-by: David Wang <00107082@....com>
>> ---
>>  drivers/usb/host/xhci-mem.c  | 1 +
>>  drivers/usb/host/xhci-ring.c | 3 +--
>>  drivers/usb/host/xhci.c      | 8 +++-----
>>  3 files changed, 5 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index d698095fc88d..b19e41cf1c4c 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1745,6 +1745,7 @@ struct xhci_command *xhci_alloc_command_with_ctx(struct xhci_hcd *xhci,
>>  
>>  void xhci_urb_free_priv(struct urb_priv *urb_priv)
>>  {
>> +	WARN_ONCE(1, "xhci private data should be managed by urb");
>
>You just crashed the kernel if this ever gets hit.  As you are saying
>this should never be called, why is this function even present anymore?
>
>This makes no sense :(

I meant to warn further changes to xhci:  better not manage  private data .
I don't think it would crash,   xhci_urb_free_priv should not  be paired with 
urb_hcpriv_mempool_zalloc.  (But nothing prevent it though, same as nothing 
prevents urb_hcpriv_mempool_zalloc being paired with kfree....)
It would be better to remove the whole function.


>
>Again, NEVER add a WARN*() call to the kernel for something that it
>should be handling properly on its own.  Otherwise you just lost all the
>user's data when the box got rebooted (and if userspace can trigger
>this, you just created a new CVE...)

Copy that~!

Thanks~
David
>
>thanks,
>
>greg k-h