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
