Re: [PATCH v2] usb: core: warn if a GFP zone flag is passed to hcd_buffer_alloc()

[Petr Tesarik <ptesarik@suse.com>]

On Mon, 14 Apr 2025 09:12:09 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Mon, Apr 14, 2025 at 09:02:16AM +0200, Petr Tesarik wrote:
> > On Fri, 11 Apr 2025 15:57:19 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.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@suse.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).  
> 
> There are billions of Linux systems out there with panic-on-warn enabled :(
> 
> > > 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.  
> 
> A warning is fine, but don't reboot a box please.  dev_warn() with a
> ratelimit and then return an error perhaps?

If we're concerned about breaking existing systems in the wild, then we
should merely issue a warning that the flag is ignored. So, probably a
ratelimited dev_warn() and continue operation.

> > 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.  
> 
> I agree, they should be removed as they don't do what people think they
> do.  So why not just remove them entirely, otherwise are you going to go
> and add this type of checking to all bus subsystems?

I'm kind of testing grounds here. But yes, I'm browsing all in-tree
occurrences of GFP_DMA and GFP_DMA32, looking for corner cases that
may break. So far, I have found exactly one user of the DMA zone who
appears to be quite right: s390 I/O channels to cope with the legacy
31-bit addressing of the CCW instruction. JFYI.

Petr T