linux-kernel - Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer

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: <20250414090216.596ebd11@mordecai>
Date: Mon, 14 Apr 2025 09:02:16 +0200
From: Petr Tesarik <ptesarik@...e.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to
 hcd_buffer_alloc()

On Fri, 11 Apr 2025 15:57:19 +0200
Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:

> On Tue, Mar 25, 2025 at 02:40:00PM +0100, Petr Tesarik wrote:
> > Remove a misleading comment and issue a warning if a zone modifier is
> > specified when allocating a hcd buffer.
> > 
> > There is no valid use case for a GFP zone modifier in hcd_buffer_alloc():
> > - PIO mode can use any kernel-addressable memory
> > - dma_alloc_coherent() ignores memory zone bits
> > 
> > This function is called by usb_alloc_coherent() and indirectly by
> > usb_submit_urb(). Despite the comment, no in-tree users currently pass
> > GFP_DMA.
> > 
> > Signed-off-by: Petr Tesarik <ptesarik@...e.com>
> > ---
> >  drivers/usb/core/buffer.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> > index 87230869e1fa..10844cd42e66 100644
> > --- a/drivers/usb/core/buffer.c
> > +++ b/drivers/usb/core/buffer.c
> > @@ -108,10 +108,6 @@ void hcd_buffer_destroy(struct usb_hcd *hcd)
> >  }
> >  
> >  
> > -/* sometimes alloc/free could use kmalloc with GFP_DMA, for
> > - * better sharing and to leverage mm/slab.c intelligence.
> > - */
> > -
> >  void *hcd_buffer_alloc(
> >  	struct usb_bus		*bus,
> >  	size_t			size,
> > @@ -128,6 +124,12 @@ void *hcd_buffer_alloc(
> >  	if (hcd->localmem_pool)
> >  		return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> >  
> > +	/*
> > +	 * Zone modifiers are ignored by DMA API, and PIO should always use
> > +	 * GFP_KERNEL.
> > +	 */
> > +	WARN_ON_ONCE(mem_flags & GFP_ZONEMASK);  
> 
> You just rebooted the box if this happens, do you REALLY want to do
> that?  People generally do not like their data lost :(

FWIW my box does not reboot on a warning. But I admit there are people
who want to run their systems with panic_on_warn (although I suspect
they already experience some sudden reboots, so they had better be
prepared).

> Why not just fix the callers, OR if this really isn't going to be
> allowed, return an error and just fail the whole submission?  And stick
> around to fix up all of the drivers that end up triggering this...

That's the point. AFAICS there are _no_ in-tree callers that would pass
GFP_DMA or GFP_DMA32 to hcd_buffer_alloc(), directly or indirectly. But
nobody should be tempted to add the flag, because I cannot imagine how
that would ever be the right thing to do.

I can change it back to mem_flags &= ~GFP_ZONEMASK to fix it silently;
I simply thought that driver authors may appreciate a warning that
they're trying to do something silly.

Whatever works for you, but please keep in mind that there seems to be
agreement among mm people that DMA and DMA32 zones should be removed
from the kernel eventually.

Petr T