* usbfs zerocopy broken on ARM/ARM64 @ 2018-11-06 22:32 Steve Markgraf 2018-11-07 11:35 ` Mark Rutland 2018-11-07 12:31 ` Russell King - ARM Linux 0 siblings, 2 replies; 5+ messages in thread From: Steve Markgraf @ 2018-11-06 22:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* usbfs zerocopy broken on ARM/ARM64 2018-11-06 22:32 usbfs zerocopy broken on ARM/ARM64 Steve Markgraf @ 2018-11-07 11:35 ` Mark Rutland 2018-11-07 12:23 ` Robin Murphy 2018-11-07 12:31 ` Russell King - ARM Linux 1 sibling, 1 reply; 5+ messages in thread From: Mark Rutland @ 2018-11-07 11:35 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* usbfs zerocopy broken on ARM/ARM64 2018-11-07 11:35 ` Mark Rutland @ 2018-11-07 12:23 ` Robin Murphy 0 siblings, 0 replies; 5+ messages in thread From: Robin Murphy @ 2018-11-07 12:23 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* usbfs zerocopy broken on ARM/ARM64 2018-11-06 22:32 usbfs zerocopy broken on ARM/ARM64 Steve Markgraf 2018-11-07 11:35 ` Mark Rutland @ 2018-11-07 12:31 ` Russell King - ARM Linux 2018-11-09 20:58 ` Steve Markgraf 1 sibling, 1 reply; 5+ messages in thread From: Russell King - ARM Linux @ 2018-11-07 12:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* usbfs zerocopy broken on ARM/ARM64 2018-11-07 12:31 ` Russell King - ARM Linux @ 2018-11-09 20:58 ` Steve Markgraf 0 siblings, 0 replies; 5+ messages in thread From: Steve Markgraf @ 2018-11-09 20:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-09 20:58 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-06 22:32 usbfs zerocopy broken on ARM/ARM64 Steve Markgraf 2018-11-07 11:35 ` Mark Rutland 2018-11-07 12:23 ` Robin Murphy 2018-11-07 12:31 ` Russell King - ARM Linux 2018-11-09 20:58 ` Steve Markgraf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.