Re: [PATCH 1/4] usb: usbfs: Enforce page requirements for mmap

[Ruihan Li]

Hi Alan,

On Wed, May 10, 2023 at 10:37:45AM -0400, Alan Stern wrote:
> On Wed, May 10, 2023 at 04:55:24PM +0800, Ruihan Li wrote:
> > The current implementation of usbdev_mmap uses usb_alloc_coherent to
> > allocate memory pages that will later be mapped into the user space.
> > Meanwhile, usb_alloc_coherent employs three different methods to
> > allocate memory, as outlined below:
> >  * If hcd->localmem_pool is non-null, it uses gen_pool_dma_alloc to
> >    allocate memory.
> >  * If DMA is not available, it uses kmalloc to allocate memory.
> >  * Otherwise, it uses dma_alloc_coherent.
> > 
> > However, it should be noted that gen_pool_dma_alloc does not guarantee
> > that the resulting memory will be page-aligned. Furthermore, trying to
> > map slab pages (i.e., memory allocated by kmalloc) into the user space
> > is not resonable and can lead to problems, such as a type confusion bug
> > when PAGE_TABLE_CHECK=y [1].
> > 
> > To address these issues, this patch introduces hcd_alloc_coherent_pages,
> > which addresses the above two problems. Specifically,
> > hcd_alloc_coherent_pages uses gen_pool_dma_alloc_align instead of
> > gen_pool_dma_alloc to ensure that the memory is page-aligned. To replace
> > kmalloc, hcd_alloc_coherent_pages directly allocates pages by calling
> > __get_free_pages.
> > 
> > Reported-by: syzbot+fcf1a817ceb50935ce99@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/lkml/000000000000258e5e05fae79fc1@xxxxxxxxxx/ [1]
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Ruihan Li <lrh2000@xxxxxxxxxx>
> > ---
> 
> I'm never quite sure about when it makes sense to complain about 
> stylistic issues.  Nevertheless, I'm going to do so here...
> 
> >  drivers/usb/core/buffer.c | 41 +++++++++++++++++++++++++++++++++++++++
> >  drivers/usb/core/devio.c  |  9 +++++----
> >  include/linux/usb/hcd.h   |  5 +++++
> >  3 files changed, 51 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> > index fbb087b72..6010ef9f5 100644
> > --- a/drivers/usb/core/buffer.c
> > +++ b/drivers/usb/core/buffer.c
> > @@ -172,3 +172,44 @@ void hcd_buffer_free(
> >  	}
> >  	dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> >  }
> > +
> > +void *hcd_buffer_alloc_pages(struct usb_hcd *hcd, size_t size,
> > +			     gfp_t mem_flags, dma_addr_t *dma)
> > +{
> > +	if (size == 0)
> > +		return NULL;
> > +
> > +	if (hcd->localmem_pool)
> > +		return gen_pool_dma_alloc_align(hcd->localmem_pool,
> > +						size, dma, PAGE_SIZE);
> 
> C isn't Lisp.  Expressions in C are not based entirely around 
> parentheses, and it's not necessary to align our code based on the 
> parenthesized sub-expressions to avoid hopelessly confusing the reader.
> 
> The style used in this file (and many other places in the USB core) is 
> to indent continuation lines by two tab stops.  The same comment applies 
> to all the other continuation lines you added or changed in this patch 
> and in patch 2/4.
> 
> Alan Stern

I'm just a bit shocked to find out that different subsystems might
prefer different styles of coding. In the net subsystem, checkpatch.pl
will complain that:
	CHECK: Alignment should match open parenthesis

Nevertheless, in the next version, I'll follow the coding style that you
have pointed out.

Thanks,
Ruihan Li