usbfs zerocopy broken on ARM/ARM64

usbfs zerocopy broken on ARM/ARM64

[Steve Markgraf]

Hi,

Preface: I've been directed here after posting this to linux-
usb at vger.kernel.org.

I'm the author of a userspace library that makes use of the usbfs
zerocopy-feature via libusb-1.0, and recently received a bug report that
garbage data is received with bulk transfers on ARM-based systems.

When I debugged the issue, I found out that the Kernel maps seemingly
random memory to my transfer buffers, containing memory of other
processes or even the Kernel itself.

The code that does the mapping in drivers/usb/core/devio.c:
(line 243 in v4.20-rc1)

> if (remap_pfn_range(vma, vma->vm_start,
>                     virt_to_phys(usbm->mem) >> PAGE_SHIFT,
>                     size, vma->vm_page_prot) < 0) {

With the following change the issue is fixed for ARM systems, but it
breaks x86 systems:

-                      virt_to_phys(usbm->mem) >> PAGE_SHIFT,
+                      page_to_pfn(virt_to_page(dma_addr)),

Both usbm->mem and dma_addr are returned by the previous call to
usb_alloc_coherent().
Here's an example of the pointers generated by the two macros on an
ARM64 system for the same buffer:

virt_to_phys(usbm->mem) >> PAGE_SHIFT: 00000000808693ce
page_to_pfn(virt_to_page(dma_addr)):   000000009775a856

>From what I read so far I got the impression that the 'proper' way would
be to use dma_mmap_coherent() with dma_addr instead of
remap_pfn_range(), however, I did not get it to work. Can anyone help
out?

Regards,
Steve

usbfs zerocopy broken on ARM/ARM64

[Mark Rutland]

On Tue, Nov 06, 2018 at 11:32:01PM +0100, Steve Markgraf wrote:
> Hi,
> 
> Preface: I've been directed here after posting this to linux-
> usb at vger.kernel.org.
> 
> I'm the author of a userspace library that makes use of the usbfs
> zerocopy-feature via libusb-1.0, and recently received a bug report that
> garbage data is received with bulk transfers on ARM-based systems.
> 
> When I debugged the issue, I found out that the Kernel maps seemingly
> random memory to my transfer buffers, containing memory of other
> processes or even the Kernel itself.
> 
> The code that does the mapping in drivers/usb/core/devio.c:
> (line 243 in v4.20-rc1)
> 
> > if (remap_pfn_range(vma, vma->vm_start,
> >                     virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> >                     size, vma->vm_page_prot) < 0) {
> 
> With the following change the issue is fixed for ARM systems, but it
> breaks x86 systems:
> 
> -                      virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> +                      page_to_pfn(virt_to_page(dma_addr)),

Given that dma_addr is not a CPU virtual address, that change isn't
right.

For the existing case, is usbm->mem definitely always a linear map
address? What happens if you enable CONFIG_DEBUG_VIRTUAL?

> Both usbm->mem and dma_addr are returned by the previous call to
> usb_alloc_coherent().

It looks like this calls hcd_buffer_alloc(), which may call:

* kmalloc()
* dma_pool_alloc()
* dma_alloc_coherent()

I'm not familiar with the DMA API, but I suspect the latter two may
return vmalloc-space addresses.

> Here's an example of the pointers generated by the two macros on an
> ARM64 system for the same buffer:
> 
> virt_to_phys(usbm->mem) >> PAGE_SHIFT: 00000000808693ce
> page_to_pfn(virt_to_page(dma_addr)):   000000009775a856
> 
> From what I read so far I got the impression that the 'proper' way would
> be to use dma_mmap_coherent() with dma_addr instead of
> remap_pfn_range(), however, I did not get it to work. Can anyone help
> out?

Given that usb_alloc_coherent() *could* return a kmalloc address, I
don't think that's right either.

It sounds like there needs to be a corresponding hcd_buffer_mmap() that
does the right thing for whatever hcd_buffer_alloc() did, which might
mean not being able to expose an address to userspace in some cases.

Thanks,
Mark.

usbfs zerocopy broken on ARM/ARM64

[Robin Murphy]

On 2018-11-07 11:35 am, Mark Rutland wrote:
> On Tue, Nov 06, 2018 at 11:32:01PM +0100, Steve Markgraf wrote:
>> Hi,
>>
>> Preface: I've been directed here after posting this to linux-
>> usb at vger.kernel.org.
>>
>> I'm the author of a userspace library that makes use of the usbfs
>> zerocopy-feature via libusb-1.0, and recently received a bug report that
>> garbage data is received with bulk transfers on ARM-based systems.
>>
>> When I debugged the issue, I found out that the Kernel maps seemingly
>> random memory to my transfer buffers, containing memory of other
>> processes or even the Kernel itself.
>>
>> The code that does the mapping in drivers/usb/core/devio.c:
>> (line 243 in v4.20-rc1)
>>
>>> if (remap_pfn_range(vma, vma->vm_start,
>>>                      virt_to_phys(usbm->mem) >> PAGE_SHIFT,
>>>                      size, vma->vm_page_prot) < 0) {
>>
>> With the following change the issue is fixed for ARM systems, but it
>> breaks x86 systems:
>>
>> -                      virt_to_phys(usbm->mem) >> PAGE_SHIFT,
>> +                      page_to_pfn(virt_to_page(dma_addr)),
> 
> Given that dma_addr is not a CPU virtual address, that change isn't
> right.
> 
> For the existing case, is usbm->mem definitely always a linear map
> address? What happens if you enable CONFIG_DEBUG_VIRTUAL?
> 
>> Both usbm->mem and dma_addr are returned by the previous call to
>> usb_alloc_coherent().
> 
> It looks like this calls hcd_buffer_alloc(), which may call:
> 
> * kmalloc()
> * dma_pool_alloc()
> * dma_alloc_coherent()
> 
> I'm not familiar with the DMA API, but I suspect the latter two may
> return vmalloc-space addresses.

Indeed; they may be regular kernel addresses, they may be vmalloc 
addresses, or they may be something else entirely. The only thing you 
are guaranteed able to do with the address returned by 
dma_alloc_coherent() (and thus by extension dma_pool_alloc() as well) is 
dereference it - feeding it to virt_to_* is most definitely wrong.

>> Here's an example of the pointers generated by the two macros on an
>> ARM64 system for the same buffer:
>>
>> virt_to_phys(usbm->mem) >> PAGE_SHIFT: 00000000808693ce
>> page_to_pfn(virt_to_page(dma_addr)):   000000009775a856
>>
>>  From what I read so far I got the impression that the 'proper' way would
>> be to use dma_mmap_coherent() with dma_addr instead of
>> remap_pfn_range(), however, I did not get it to work. Can anyone help
>> out?
> 
> Given that usb_alloc_coherent() *could* return a kmalloc address, I
> don't think that's right either.
> 
> It sounds like there needs to be a corresponding hcd_buffer_mmap() that
> does the right thing for whatever hcd_buffer_alloc() did, which might
> mean not being able to expose an address to userspace in some cases.

Right - if the logic for figuring out the source of an allocation is 
good enough for hcd_buffer_free(), then it's also the perfect starting 
point for a corresponding mmap implementation. As it stands, that code 
in usbdev_mmap() has never been correct, and has simply been getting 
lucky on certain platforms.

In terms of actually implementing it, you can probably get away with 
just handling the case where the buffer did come from 
dma_alloc_coherent() and rejecting the other two outright - an mmap 
request is presumably going to be at least page-aligned and thus too 
large to hit the DMA pool path anyway, and using zero-copy with a 
PIO-mode device seems futile enough that I doubt anyone would mind it 
not being supported at all.

Robin.

usbfs zerocopy broken on ARM/ARM64

[Russell King - ARM Linux]

On Tue, Nov 06, 2018 at 11:32:01PM +0100, Steve Markgraf wrote:
> Hi,
> 
> Preface: I've been directed here after posting this to linux-
> usb at vger.kernel.org.
> 
> I'm the author of a userspace library that makes use of the usbfs
> zerocopy-feature via libusb-1.0, and recently received a bug report that
> garbage data is received with bulk transfers on ARM-based systems.
> 
> When I debugged the issue, I found out that the Kernel maps seemingly
> random memory to my transfer buffers, containing memory of other
> processes or even the Kernel itself.
> 
> The code that does the mapping in drivers/usb/core/devio.c:
> (line 243 in v4.20-rc1)
> 
> > if (remap_pfn_range(vma, vma->vm_start,
> >                     virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> >                     size, vma->vm_page_prot) < 0) {
> 
> With the following change the issue is fixed for ARM systems, but it
> breaks x86 systems:
> 
> -                      virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> +                      page_to_pfn(virt_to_page(dma_addr)),
> 
> Both usbm->mem and dma_addr are returned by the previous call to
> usb_alloc_coherent().
> Here's an example of the pointers generated by the two macros on an
> ARM64 system for the same buffer:
> 
> virt_to_phys(usbm->mem) >> PAGE_SHIFT: 00000000808693ce
> page_to_pfn(virt_to_page(dma_addr)):   000000009775a856
> 
> >From what I read so far I got the impression that the 'proper' way would
> be to use dma_mmap_coherent() with dma_addr instead of
> remap_pfn_range(), however, I did not get it to work. Can anyone help
> out?

That is correct.  Please show what you tried.  Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

usbfs zerocopy broken on ARM/ARM64

[Steve Markgraf]

On 07.11.18 13:31, Russell King - ARM Linux wrote:
>> From what I read so far I got the impression that the 'proper' way would
>> be to use dma_mmap_coherent() with dma_addr instead of
>> remap_pfn_range(), however, I did not get it to work. Can anyone help
>> out?
> 
> That is correct.  Please show what you tried.  Thanks.

Thanks for the clarification. I now debugged it further and it turned
out I was using the wrong device pointer. Now it is working for both ARM
and x86 with:

> dma_mmap_coherent(ps->dev->bus->sysdev, vma, mem, dma_handle, size);

However, on x86 I get warnings in the Kernel log:

> x86/PAT: rtl_test:4018 map pfn RAM range req uncached-minus for
> [mem >0x34bc0000-0x34bfffff], got write-back

Another user of dma_mmap_coherent(), sound/core/pcm_native.c, only uses
it for arch != x86 for that reason:

> #ifndef CONFIG_X86 /* for avoiding warnings arch/x86/mm/pat.c */

Since the original code is working fine on x86, probably that would be
the way to go for the usbfs code as well.
And as the previous posters pointed out, hcd_buffer_alloc() can use
kmalloc() for PIO-based USB controllers, but in this case sets the
dma_handle to ~0, so we could check for this and only use
dma_mmap_coherent() if that's not the case.

If that's correct, I'll prepare a patch.

Regards,
Steve