1. Patchew
  2. linux
  3. [PATCH 0/4] Fix type confusion in page_table_check
  4. View patch

[PATCH 2/4] usb: usbfs: Use consistent mmap functions

Ruihan Li posted 4 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH 2/4] usb: usbfs: Use consistent mmap functions
Posted by Ruihan Li 2 years, 9 months ago 
When hcd->localmem_pool is non-null, it is used to allocate DMA memory.
In this case, the dma address will be properly returned (in dma_handle),
and dma_mmap_coherent should be used to map this memory into the user
space. However, the current implementation uses pfn_remap_range, which
is supposed to map normal pages (instead of DMA pages).

Instead of repeating the logic in the memory allocation function, this
patch introduces a more robust solution. To address the previous issue,
this patch checks the type of allocated memory by testing whether
dma_handle is properly set. If dma_handle is properly returned, it means
some DMA pages are allocated and dma_mmap_coherent should be used to map
them. Otherwise, normal pages are allocated and pfn_remap_range should
be called. This ensures that the correct mmap functions are used
consistently, independently with logic details that determine which type
of memory gets allocated.

Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch")
Cc: stable@vger.kernel.org
Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
---
 drivers/usb/core/devio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index b4cf9e860..5067030b7 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	size_t size = vma->vm_end - vma->vm_start;
 	void *mem;
 	unsigned long flags;
-	dma_addr_t dma_handle;
+	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
 	int ret;
 
 	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
@@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
 	usbm->vma_use_count = 1;
 	INIT_LIST_HEAD(&usbm->memlist);
 
-	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
+	/* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates
+	 * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check
+	 * whether we are in such cases, and then use remap_pfn_range (or
+	 * dma_mmap_coherent) to map normal (or DMA) pages into the user
+	 * space, respectively.
+	 */
+	if (dma_handle == DMA_MAPPING_ERROR) {
 		if (remap_pfn_range(vma, vma->vm_start,
 				    virt_to_phys(usbm->mem) >> PAGE_SHIFT,
 				    size, vma->vm_page_prot) < 0) {
-- 
2.40.1
Re: [PATCH 2/4] usb: usbfs: Use consistent mmap functions
Posted by Alan Stern 2 years, 9 months ago 
On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote:
> When hcd->localmem_pool is non-null, it is used to allocate DMA memory.
> In this case, the dma address will be properly returned (in dma_handle),
> and dma_mmap_coherent should be used to map this memory into the user
> space. However, the current implementation uses pfn_remap_range, which
> is supposed to map normal pages (instead of DMA pages).
> 
> Instead of repeating the logic in the memory allocation function, this
> patch introduces a more robust solution. To address the previous issue,
> this patch checks the type of allocated memory by testing whether
> dma_handle is properly set. If dma_handle is properly returned, it means
> some DMA pages are allocated and dma_mmap_coherent should be used to map
> them. Otherwise, normal pages are allocated and pfn_remap_range should
> be called. This ensures that the correct mmap functions are used
> consistently, independently with logic details that determine which type
> of memory gets allocated.
> 
> Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> ---
>  drivers/usb/core/devio.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index b4cf9e860..5067030b7 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	size_t size = vma->vm_end - vma->vm_start;
>  	void *mem;
>  	unsigned long flags;
> -	dma_addr_t dma_handle;
> +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
>  	int ret;
>  
>  	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>  	usbm->vma_use_count = 1;
>  	INIT_LIST_HEAD(&usbm->memlist);
>  
> -	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> +	/* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates
> +	 * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check
> +	 * whether we are in such cases, and then use remap_pfn_range (or
> +	 * dma_mmap_coherent) to map normal (or DMA) pages into the user
> +	 * space, respectively.
> +	 */

Another stylistic issue.  For multi-line comments, the format we use is:

	/*
	 * Blah, blah, blah
	 * Blah, blah, blah
	 */

Alan Stern
Re: [PATCH 2/4] usb: usbfs: Use consistent mmap functions
Posted by Ruihan Li 2 years, 9 months ago 
Hi Alan,

On Wed, May 10, 2023 at 10:38:48AM -0400, Alan Stern wrote:
> On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote:
> > When hcd->localmem_pool is non-null, it is used to allocate DMA memory.
> > In this case, the dma address will be properly returned (in dma_handle),
> > and dma_mmap_coherent should be used to map this memory into the user
> > space. However, the current implementation uses pfn_remap_range, which
> > is supposed to map normal pages (instead of DMA pages).
> > 
> > Instead of repeating the logic in the memory allocation function, this
> > patch introduces a more robust solution. To address the previous issue,
> > this patch checks the type of allocated memory by testing whether
> > dma_handle is properly set. If dma_handle is properly returned, it means
> > some DMA pages are allocated and dma_mmap_coherent should be used to map
> > them. Otherwise, normal pages are allocated and pfn_remap_range should
> > be called. This ensures that the correct mmap functions are used
> > consistently, independently with logic details that determine which type
> > of memory gets allocated.
> > 
> > Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
> > ---
> >  drivers/usb/core/devio.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index b4cf9e860..5067030b7 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> >  	size_t size = vma->vm_end - vma->vm_start;
> >  	void *mem;
> >  	unsigned long flags;
> > -	dma_addr_t dma_handle;
> > +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
> >  	int ret;
> >  
> >  	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> > @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> >  	usbm->vma_use_count = 1;
> >  	INIT_LIST_HEAD(&usbm->memlist);
> >  
> > -	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
> > +	/* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates
> > +	 * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check
> > +	 * whether we are in such cases, and then use remap_pfn_range (or
> > +	 * dma_mmap_coherent) to map normal (or DMA) pages into the user
> > +	 * space, respectively.
> > +	 */
> 
> Another stylistic issue.  For multi-line comments, the format we use is:
> 
> 	/*
> 	 * Blah, blah, blah
> 	 * Blah, blah, blah
> 	 */
> 
> Alan Stern

Yeah, I am pretty sure it is another style difference with the net
subsystem. Again, in the next version, I'll follow the coding style that
you have pointed out.

Thanks,
Ruihan Li
Re: [PATCH 2/4] usb: usbfs: Use consistent mmap functions
Posted by David Hildenbrand 2 years, 9 months ago 
On 10.05.23 17:41, Ruihan Li wrote:
> Hi Alan,
> 
> On Wed, May 10, 2023 at 10:38:48AM -0400, Alan Stern wrote:
>> On Wed, May 10, 2023 at 04:55:25PM +0800, Ruihan Li wrote:
>>> When hcd->localmem_pool is non-null, it is used to allocate DMA memory.
>>> In this case, the dma address will be properly returned (in dma_handle),
>>> and dma_mmap_coherent should be used to map this memory into the user
>>> space. However, the current implementation uses pfn_remap_range, which
>>> is supposed to map normal pages (instead of DMA pages).
>>>
>>> Instead of repeating the logic in the memory allocation function, this
>>> patch introduces a more robust solution. To address the previous issue,
>>> this patch checks the type of allocated memory by testing whether
>>> dma_handle is properly set. If dma_handle is properly returned, it means
>>> some DMA pages are allocated and dma_mmap_coherent should be used to map
>>> them. Otherwise, normal pages are allocated and pfn_remap_range should
>>> be called. This ensures that the correct mmap functions are used
>>> consistently, independently with logic details that determine which type
>>> of memory gets allocated.
>>>
>>> Fixes: a0e710a7def4 ("USB: usbfs: fix mmap dma mismatch")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn>
>>> ---
>>>   drivers/usb/core/devio.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>>> index b4cf9e860..5067030b7 100644
>>> --- a/drivers/usb/core/devio.c
>>> +++ b/drivers/usb/core/devio.c
>>> @@ -235,7 +235,7 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>>>   	size_t size = vma->vm_end - vma->vm_start;
>>>   	void *mem;
>>>   	unsigned long flags;
>>> -	dma_addr_t dma_handle;
>>> +	dma_addr_t dma_handle = DMA_MAPPING_ERROR;
>>>   	int ret;
>>>   
>>>   	ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
>>> @@ -265,7 +265,13 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
>>>   	usbm->vma_use_count = 1;
>>>   	INIT_LIST_HEAD(&usbm->memlist);
>>>   
>>> -	if (hcd->localmem_pool || !hcd_uses_dma(hcd)) {
>>> +	/* In DMA-unavailable cases, hcd_buffer_alloc_pages allocates
>>> +	 * normal pages and assigns DMA_MAPPING_ERROR to dma_handle. Check
>>> +	 * whether we are in such cases, and then use remap_pfn_range (or
>>> +	 * dma_mmap_coherent) to map normal (or DMA) pages into the user
>>> +	 * space, respectively.
>>> +	 */
>>
>> Another stylistic issue.  For multi-line comments, the format we use is:
>>
>> 	/*
>> 	 * Blah, blah, blah
>> 	 * Blah, blah, blah
>> 	 */
>>
>> Alan Stern
> 
> Yeah, I am pretty sure it is another style difference with the net
> subsystem. Again, in the next version, I'll follow the coding style that
> you have pointed out.

Documentation/process/coding-style.rst

spells out that net/ and drivers/net/ are "special".

Regarding breaking long lines, it's just an inconsistent, undocumented 
mess IIRC ...

-- 
Thanks,

David / dhildenb

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.