* usb zero copy dma handling @ 2019-08-08 8:46 yvahkhfo.1df7f8c2 2019-08-08 8:58 ` Greg KH 2019-08-08 16:10 ` Christoph Hellwig 0 siblings, 2 replies; 11+ messages in thread From: yvahkhfo.1df7f8c2 @ 2019-08-08 8:46 UTC (permalink / raw) To: linux-usb, linux-arm-kernel; +Cc: security [-- Attachment #1: Type: text/plain, Size: 1882 bytes --] Hello linux-usb and linux-arm. Ccing security@ because "the kernel dma code is mapping randomish kernel/user mem to a user process" seems to have security implications even though i didnt research that aspect past "its a 100% reliable way to crash a raspi from userspace". tried submitting this through linux-arm-kernel ~2 weeks ago but the only "response" i got was phishing-spam. tried to follow up through raspi-internals chat, they suggested i try linux-usb instead, but otoh the original reporter was deflected from -usb to "try some other mls, they might care". https://www.spinics.net/lists/linux-usb/msg173277.html if i am not following some arcane ritual or indenting convention required by regular users of these lists i apologize in advance, but i am not a kernel developer, i am just here as a user with a bug and a patch. (and the vger FAQ link 404s...) i rediffed against HEAD even though the two weeks old patch still applied cleanly with +2 offset. # stepping off soap box # actual technical content starts here # this is a followup to that thread from 2018-11: https://www.spinics.net/lists/arm-kernel/msg685598.html the issue was discussed in more detail than i can claim to fully understand back then, but no fix ever merged. but i would really like to use rtl_433 on a raspi without having to build a custom-patched kernel first. the attached patch is my stripdown/cleanup of a devel-diff provided to me by the original reporter Steve Markgraf. credits to him for the good parts, blame to me for the bad parts. this does not cover the additional case of "PIO-based usb controllers" mainly because i dont understand what that means (or how to handle it) and if its broken right now (as the thread indicates) it might as well stay broken until someone who understands cares enough. could you please get this on track for merging? regards, x23 [-- Attachment #2: arm-usb-dma-v2.diff --] [-- Type: text/plain, Size: 678 bytes --] diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index b265ab5405f9..69594c2169ea 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) usbm->vma_use_count = 1; INIT_LIST_HEAD(&usbm->memlist); +#ifdef CONFIG_X86 if (remap_pfn_range(vma, vma->vm_start, virt_to_phys(usbm->mem) >> PAGE_SHIFT, size, vma->vm_page_prot) < 0) { +#else /* !CONFIG_X86 */ + if (dma_mmap_coherent(ps->dev->bus->sysdev, + vma, mem, dma_handle, size) < 0) { +#endif /* !CONFIG_X86 */ dec_usb_memory_use_count(usbm, &usbm->vma_use_count); return -EAGAIN; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 8:46 usb zero copy dma handling yvahkhfo.1df7f8c2 @ 2019-08-08 8:58 ` Greg KH 2019-08-08 9:46 ` Robin Murphy 2019-08-08 9:59 ` Russell King - ARM Linux admin 2019-08-08 16:10 ` Christoph Hellwig 1 sibling, 2 replies; 11+ messages in thread From: Greg KH @ 2019-08-08 8:58 UTC (permalink / raw) To: yvahkhfo.1df7f8c2; +Cc: linux-usb, linux-arm-kernel, security On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > Hello linux-usb and linux-arm. > > Ccing security@ because "the kernel dma code is mapping randomish > kernel/user mem to a user process" seems to have security implications > even though i didnt research that aspect past "its a 100% reliable way > to crash a raspi from userspace". > > tried submitting this through linux-arm-kernel ~2 weeks ago but > the only "response" i got was phishing-spam. > tried to follow up through raspi-internals chat, they suggested > i try linux-usb instead, but otoh the original reporter was > deflected from -usb to "try some other mls, they might care". > https://www.spinics.net/lists/linux-usb/msg173277.html > > if i am not following some arcane ritual or indenting convention required > by regular users of these lists i apologize in advance, but i am not a > kernel developer, i am just here as a user with a bug and a patch. > (and the vger FAQ link 404s...) The "arcane ritual" should be really well documented by now, it's in Documentation/SubmittingPatches in your kernel tree, and you can read it online at: https://www.kernel.org/doc/html/latest/process/submitting-patches.html > i rediffed against HEAD even though the two weeks old patch still applied > cleanly with +2 offset. > > # stepping off soap box # actual technical content starts here # > > this is a followup to that thread from 2018-11: > https://www.spinics.net/lists/arm-kernel/msg685598.html > > the issue was discussed in more detail than i can claim > to fully understand back then, but no fix ever merged. > but i would really like to use rtl_433 on a raspi without > having to build a custom-patched kernel first. > > the attached patch is my stripdown/cleanup of a devel-diff > provided to me by the original reporter Steve Markgraf. > credits to him for the good parts, blame to me for the bad parts. > > this does not cover the additional case of "PIO-based usb controllers" > mainly because i dont understand what that means (or how to handle it) > and if its broken right now (as the thread indicates) it might > as well stay broken until someone who understands cares enough. > > could you please get this on track for merging? > > regards, > x23 > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index b265ab5405f9..69594c2169ea 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > usbm->vma_use_count = 1; > INIT_LIST_HEAD(&usbm->memlist); > > +#ifdef CONFIG_X86 > if (remap_pfn_range(vma, vma->vm_start, > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > size, vma->vm_page_prot) < 0) { > +#else /* !CONFIG_X86 */ > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > + vma, mem, dma_handle, size) < 0) { > +#endif /* !CONFIG_X86 */ > dec_usb_memory_use_count(usbm, &usbm->vma_use_count); > return -EAGAIN; > } First off, we need this in a format we could apply it in (hint, read the above links). But the main issue here is what exactly is this "fixing"? What is wrong with the existing code that non-x86 systems have such a problem with? Shouldn't all of these dma issues be handled by the platform with the remap_pfn_range() call itself? What is the problem that you are having? thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 8:58 ` Greg KH @ 2019-08-08 9:46 ` Robin Murphy 2019-08-08 10:07 ` Greg KH 2019-08-08 9:59 ` Russell King - ARM Linux admin 1 sibling, 1 reply; 11+ messages in thread From: Robin Murphy @ 2019-08-08 9:46 UTC (permalink / raw) To: Greg KH, yvahkhfo.1df7f8c2; +Cc: security, linux-usb, linux-arm-kernel On 2019-08-08 9:58 am, Greg KH wrote: > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: >> Hello linux-usb and linux-arm. >> >> Ccing security@ because "the kernel dma code is mapping randomish >> kernel/user mem to a user process" seems to have security implications >> even though i didnt research that aspect past "its a 100% reliable way >> to crash a raspi from userspace". >> >> tried submitting this through linux-arm-kernel ~2 weeks ago but >> the only "response" i got was phishing-spam. >> tried to follow up through raspi-internals chat, they suggested >> i try linux-usb instead, but otoh the original reporter was >> deflected from -usb to "try some other mls, they might care". >> https://www.spinics.net/lists/linux-usb/msg173277.html >> >> if i am not following some arcane ritual or indenting convention required >> by regular users of these lists i apologize in advance, but i am not a >> kernel developer, i am just here as a user with a bug and a patch. >> (and the vger FAQ link 404s...) > > The "arcane ritual" should be really well documented by now, it's in > Documentation/SubmittingPatches in your kernel tree, and you can read it > online at: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html > > >> i rediffed against HEAD even though the two weeks old patch still applied >> cleanly with +2 offset. >> >> # stepping off soap box # actual technical content starts here # >> >> this is a followup to that thread from 2018-11: >> https://www.spinics.net/lists/arm-kernel/msg685598.html >> >> the issue was discussed in more detail than i can claim >> to fully understand back then, but no fix ever merged. >> but i would really like to use rtl_433 on a raspi without >> having to build a custom-patched kernel first. >> >> the attached patch is my stripdown/cleanup of a devel-diff >> provided to me by the original reporter Steve Markgraf. >> credits to him for the good parts, blame to me for the bad parts. >> >> this does not cover the additional case of "PIO-based usb controllers" >> mainly because i dont understand what that means (or how to handle it) >> and if its broken right now (as the thread indicates) it might >> as well stay broken until someone who understands cares enough. >> >> could you please get this on track for merging? > > >> >> regards, >> x23 >> >> >> > >> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c >> index b265ab5405f9..69594c2169ea 100644 >> --- a/drivers/usb/core/devio.c >> +++ b/drivers/usb/core/devio.c >> @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) >> usbm->vma_use_count = 1; >> INIT_LIST_HEAD(&usbm->memlist); >> >> +#ifdef CONFIG_X86 >> if (remap_pfn_range(vma, vma->vm_start, >> virt_to_phys(usbm->mem) >> PAGE_SHIFT, >> size, vma->vm_page_prot) < 0) { >> +#else /* !CONFIG_X86 */ >> + if (dma_mmap_coherent(ps->dev->bus->sysdev, >> + vma, mem, dma_handle, size) < 0) { >> +#endif /* !CONFIG_X86 */ >> dec_usb_memory_use_count(usbm, &usbm->vma_use_count); >> return -EAGAIN; >> } > > First off, we need this in a format we could apply it in (hint, read the > above links). > > But the main issue here is what exactly is this "fixing"? What is wrong > with the existing code that non-x86 systems have such a problem with? > Shouldn't all of these dma issues be handled by the platform with the > remap_pfn_range() call itself? If usbm->mem is (or ever can be) a CPU address returned by dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield a nonsense 'PFN' to begin with. However, it it can can ever come from a regular page allocation/kmalloc/vmalloc then unconditionally passing it to dma_mmap_coherent wouldn't be right either. Robin. > > What is the problem that you are having? > > thanks, > > greg k-h > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 9:46 ` Robin Murphy @ 2019-08-08 10:07 ` Greg KH 2019-08-08 10:43 ` Robin Murphy 2019-08-08 13:05 ` Greg KH 0 siblings, 2 replies; 11+ messages in thread From: Greg KH @ 2019-08-08 10:07 UTC (permalink / raw) To: Robin Murphy; +Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote: > On 2019-08-08 9:58 am, Greg KH wrote: > > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > > > Hello linux-usb and linux-arm. > > > > > > Ccing security@ because "the kernel dma code is mapping randomish > > > kernel/user mem to a user process" seems to have security implications > > > even though i didnt research that aspect past "its a 100% reliable way > > > to crash a raspi from userspace". > > > > > > tried submitting this through linux-arm-kernel ~2 weeks ago but > > > the only "response" i got was phishing-spam. > > > tried to follow up through raspi-internals chat, they suggested > > > i try linux-usb instead, but otoh the original reporter was > > > deflected from -usb to "try some other mls, they might care". > > > https://www.spinics.net/lists/linux-usb/msg173277.html > > > > > > if i am not following some arcane ritual or indenting convention required > > > by regular users of these lists i apologize in advance, but i am not a > > > kernel developer, i am just here as a user with a bug and a patch. > > > (and the vger FAQ link 404s...) > > > > The "arcane ritual" should be really well documented by now, it's in > > Documentation/SubmittingPatches in your kernel tree, and you can read it > > online at: > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html > > > > > > > i rediffed against HEAD even though the two weeks old patch still applied > > > cleanly with +2 offset. > > > > > > # stepping off soap box # actual technical content starts here # > > > > > > this is a followup to that thread from 2018-11: > > > https://www.spinics.net/lists/arm-kernel/msg685598.html > > > > > > the issue was discussed in more detail than i can claim > > > to fully understand back then, but no fix ever merged. > > > but i would really like to use rtl_433 on a raspi without > > > having to build a custom-patched kernel first. > > > > > > the attached patch is my stripdown/cleanup of a devel-diff > > > provided to me by the original reporter Steve Markgraf. > > > credits to him for the good parts, blame to me for the bad parts. > > > > > > this does not cover the additional case of "PIO-based usb controllers" > > > mainly because i dont understand what that means (or how to handle it) > > > and if its broken right now (as the thread indicates) it might > > > as well stay broken until someone who understands cares enough. > > > > > > could you please get this on track for merging? > > > > > > > > > > regards, > > > x23 > > > > > > > > > > > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > > index b265ab5405f9..69594c2169ea 100644 > > > --- a/drivers/usb/core/devio.c > > > +++ b/drivers/usb/core/devio.c > > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > > usbm->vma_use_count = 1; > > > INIT_LIST_HEAD(&usbm->memlist); > > > +#ifdef CONFIG_X86 > > > if (remap_pfn_range(vma, vma->vm_start, > > > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > > > size, vma->vm_page_prot) < 0) { > > > +#else /* !CONFIG_X86 */ > > > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > > > + vma, mem, dma_handle, size) < 0) { > > > +#endif /* !CONFIG_X86 */ > > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count); > > > return -EAGAIN; > > > } > > > > First off, we need this in a format we could apply it in (hint, read the > > above links). > > > > But the main issue here is what exactly is this "fixing"? What is wrong > > with the existing code that non-x86 systems have such a problem with? > > Shouldn't all of these dma issues be handled by the platform with the > > remap_pfn_range() call itself? > > If usbm->mem is (or ever can be) a CPU address returned by > dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield > a nonsense 'PFN' to begin with. However, it it can can ever come from a > regular page allocation/kmalloc/vmalloc then unconditionally passing it to > dma_mmap_coherent wouldn't be right either. usbm->mem comes from a call to usb_alloc_coherent() which calls hcd_buffer_alloc() which tries to allocate memory in the best possible way for that specific host controller. If the host controller has a pool of memory, it uses that, if the host controller has PIO it uses kmalloc(), if there are some "pools" of host controller memory it uses dma_pool_alloc() and as a total last resort, calls dma_alloc_coherent(). So yes, this could happen. So how to fix this properly? What host controller driver is being used here that ends up defaulting to dma_alloc_coherent()? Shouldn't that be fixed up no matter what? And then, if what you say is correct then a real fix for devio.c could be made, but that is NOT going to just depend on the arch the system is running on, as all of this depends on the host controller being accessed at that moment for that device. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 10:07 ` Greg KH @ 2019-08-08 10:43 ` Robin Murphy 2019-08-08 13:05 ` Greg KH 1 sibling, 0 replies; 11+ messages in thread From: Robin Murphy @ 2019-08-08 10:43 UTC (permalink / raw) To: Greg KH; +Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel On 2019-08-08 11:07 am, Greg KH wrote: > On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote: >> On 2019-08-08 9:58 am, Greg KH wrote: >>> On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: >>>> Hello linux-usb and linux-arm. >>>> >>>> Ccing security@ because "the kernel dma code is mapping randomish >>>> kernel/user mem to a user process" seems to have security implications >>>> even though i didnt research that aspect past "its a 100% reliable way >>>> to crash a raspi from userspace". >>>> >>>> tried submitting this through linux-arm-kernel ~2 weeks ago but >>>> the only "response" i got was phishing-spam. >>>> tried to follow up through raspi-internals chat, they suggested >>>> i try linux-usb instead, but otoh the original reporter was >>>> deflected from -usb to "try some other mls, they might care". >>>> https://www.spinics.net/lists/linux-usb/msg173277.html >>>> >>>> if i am not following some arcane ritual or indenting convention required >>>> by regular users of these lists i apologize in advance, but i am not a >>>> kernel developer, i am just here as a user with a bug and a patch. >>>> (and the vger FAQ link 404s...) >>> >>> The "arcane ritual" should be really well documented by now, it's in >>> Documentation/SubmittingPatches in your kernel tree, and you can read it >>> online at: >>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html >>> >>> >>>> i rediffed against HEAD even though the two weeks old patch still applied >>>> cleanly with +2 offset. >>>> >>>> # stepping off soap box # actual technical content starts here # >>>> >>>> this is a followup to that thread from 2018-11: >>>> https://www.spinics.net/lists/arm-kernel/msg685598.html >>>> >>>> the issue was discussed in more detail than i can claim >>>> to fully understand back then, but no fix ever merged. >>>> but i would really like to use rtl_433 on a raspi without >>>> having to build a custom-patched kernel first. >>>> >>>> the attached patch is my stripdown/cleanup of a devel-diff >>>> provided to me by the original reporter Steve Markgraf. >>>> credits to him for the good parts, blame to me for the bad parts. >>>> >>>> this does not cover the additional case of "PIO-based usb controllers" >>>> mainly because i dont understand what that means (or how to handle it) >>>> and if its broken right now (as the thread indicates) it might >>>> as well stay broken until someone who understands cares enough. >>>> >>>> could you please get this on track for merging? >>> >>> >>>> >>>> regards, >>>> x23 >>>> >>>> >>>> >>> >>>> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c >>>> index b265ab5405f9..69594c2169ea 100644 >>>> --- a/drivers/usb/core/devio.c >>>> +++ b/drivers/usb/core/devio.c >>>> @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) >>>> usbm->vma_use_count = 1; >>>> INIT_LIST_HEAD(&usbm->memlist); >>>> +#ifdef CONFIG_X86 >>>> if (remap_pfn_range(vma, vma->vm_start, >>>> virt_to_phys(usbm->mem) >> PAGE_SHIFT, >>>> size, vma->vm_page_prot) < 0) { >>>> +#else /* !CONFIG_X86 */ >>>> + if (dma_mmap_coherent(ps->dev->bus->sysdev, >>>> + vma, mem, dma_handle, size) < 0) { >>>> +#endif /* !CONFIG_X86 */ >>>> dec_usb_memory_use_count(usbm, &usbm->vma_use_count); >>>> return -EAGAIN; >>>> } >>> >>> First off, we need this in a format we could apply it in (hint, read the >>> above links). >>> >>> But the main issue here is what exactly is this "fixing"? What is wrong >>> with the existing code that non-x86 systems have such a problem with? >>> Shouldn't all of these dma issues be handled by the platform with the >>> remap_pfn_range() call itself? >> >> If usbm->mem is (or ever can be) a CPU address returned by >> dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield >> a nonsense 'PFN' to begin with. However, it it can can ever come from a >> regular page allocation/kmalloc/vmalloc then unconditionally passing it to >> dma_mmap_coherent wouldn't be right either. > > usbm->mem comes from a call to usb_alloc_coherent() which calls > hcd_buffer_alloc() which tries to allocate memory in the best possible > way for that specific host controller. If the host controller has a > pool of memory, it uses that, if the host controller has PIO it uses > kmalloc(), if there are some "pools" of host controller memory it uses > dma_pool_alloc() and as a total last resort, calls dma_alloc_coherent(). > > So yes, this could happen. > > So how to fix this properly? What host controller driver is being used > here that ends up defaulting to dma_alloc_coherent()? Shouldn't that be > fixed up no matter what? > > And then, if what you say is correct then a real fix for devio.c could > be made, but that is NOT going to just depend on the arch the system is > running on, as all of this depends on the host controller being accessed > at that moment for that device. Right, in that case we'd probably want some kind of usb_mmap_coherent() helper to encapsulate equivalent logic to usb_{alloc,free}_coherent() to figure out which remap operation is appropriate. It's absolutely not an arch-specific thing. Robin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 10:07 ` Greg KH 2019-08-08 10:43 ` Robin Murphy @ 2019-08-08 13:05 ` Greg KH 1 sibling, 0 replies; 11+ messages in thread From: Greg KH @ 2019-08-08 13:05 UTC (permalink / raw) To: Robin Murphy; +Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel On Thu, Aug 08, 2019 at 12:07:26PM +0200, Greg KH wrote: > On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote: > > On 2019-08-08 9:58 am, Greg KH wrote: > > > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > > > > Hello linux-usb and linux-arm. > > > > > > > > Ccing security@ because "the kernel dma code is mapping randomish > > > > kernel/user mem to a user process" seems to have security implications > > > > even though i didnt research that aspect past "its a 100% reliable way > > > > to crash a raspi from userspace". > > > > > > > > tried submitting this through linux-arm-kernel ~2 weeks ago but > > > > the only "response" i got was phishing-spam. > > > > tried to follow up through raspi-internals chat, they suggested > > > > i try linux-usb instead, but otoh the original reporter was > > > > deflected from -usb to "try some other mls, they might care". > > > > https://www.spinics.net/lists/linux-usb/msg173277.html > > > > > > > > if i am not following some arcane ritual or indenting convention required > > > > by regular users of these lists i apologize in advance, but i am not a > > > > kernel developer, i am just here as a user with a bug and a patch. > > > > (and the vger FAQ link 404s...) > > > > > > The "arcane ritual" should be really well documented by now, it's in > > > Documentation/SubmittingPatches in your kernel tree, and you can read it > > > online at: > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html > > > > > > > > > > i rediffed against HEAD even though the two weeks old patch still applied > > > > cleanly with +2 offset. > > > > > > > > # stepping off soap box # actual technical content starts here # > > > > > > > > this is a followup to that thread from 2018-11: > > > > https://www.spinics.net/lists/arm-kernel/msg685598.html > > > > > > > > the issue was discussed in more detail than i can claim > > > > to fully understand back then, but no fix ever merged. > > > > but i would really like to use rtl_433 on a raspi without > > > > having to build a custom-patched kernel first. > > > > > > > > the attached patch is my stripdown/cleanup of a devel-diff > > > > provided to me by the original reporter Steve Markgraf. > > > > credits to him for the good parts, blame to me for the bad parts. > > > > > > > > this does not cover the additional case of "PIO-based usb controllers" > > > > mainly because i dont understand what that means (or how to handle it) > > > > and if its broken right now (as the thread indicates) it might > > > > as well stay broken until someone who understands cares enough. > > > > > > > > could you please get this on track for merging? > > > > > > > > > > > > > > regards, > > > > x23 > > > > > > > > > > > > > > > > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > > > index b265ab5405f9..69594c2169ea 100644 > > > > --- a/drivers/usb/core/devio.c > > > > +++ b/drivers/usb/core/devio.c > > > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > > > usbm->vma_use_count = 1; > > > > INIT_LIST_HEAD(&usbm->memlist); > > > > +#ifdef CONFIG_X86 > > > > if (remap_pfn_range(vma, vma->vm_start, > > > > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > > > > size, vma->vm_page_prot) < 0) { > > > > +#else /* !CONFIG_X86 */ > > > > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > > > > + vma, mem, dma_handle, size) < 0) { > > > > +#endif /* !CONFIG_X86 */ > > > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count); > > > > return -EAGAIN; > > > > } > > > > > > First off, we need this in a format we could apply it in (hint, read the > > > above links). > > > > > > But the main issue here is what exactly is this "fixing"? What is wrong > > > with the existing code that non-x86 systems have such a problem with? > > > Shouldn't all of these dma issues be handled by the platform with the > > > remap_pfn_range() call itself? > > > > If usbm->mem is (or ever can be) a CPU address returned by > > dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield > > a nonsense 'PFN' to begin with. However, it it can can ever come from a > > regular page allocation/kmalloc/vmalloc then unconditionally passing it to > > dma_mmap_coherent wouldn't be right either. > > usbm->mem comes from a call to usb_alloc_coherent() which calls > hcd_buffer_alloc() which tries to allocate memory in the best possible > way for that specific host controller. If the host controller has a > pool of memory, it uses that, if the host controller has PIO it uses > kmalloc(), if there are some "pools" of host controller memory it uses > dma_pool_alloc() and as a total last resort, calls dma_alloc_coherent(). > > So yes, this could happen. > > So how to fix this properly? What host controller driver is being used > here that ends up defaulting to dma_alloc_coherent()? Shouldn't that be > fixed up no matter what? > > And then, if what you say is correct then a real fix for devio.c could > be made, but that is NOT going to just depend on the arch the system is > running on, as all of this depends on the host controller being accessed > at that moment for that device. Also see this thread: https://lore.kernel.org/linux-usb/20190801220134.3295-1-gavinli@thegavinli.com/ where this just came up and how the proposed patch here would cause warnings to occur in the kernel log of users for no good reason. That issue is supposed to be fixed "soon"... thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 8:58 ` Greg KH 2019-08-08 9:46 ` Robin Murphy @ 2019-08-08 9:59 ` Russell King - ARM Linux admin 2019-08-08 10:02 ` Oliver Neukum 1 sibling, 1 reply; 11+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-08 9:59 UTC (permalink / raw) To: Greg KH; +Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel On Thu, Aug 08, 2019 at 10:58:11AM +0200, Greg KH wrote: > But the main issue here is what exactly is this "fixing"? What is wrong > with the existing code that non-x86 systems have such a problem with? > Shouldn't all of these dma issues be handled by the platform with the > remap_pfn_range() call itself? remap_pfn_range() takes a PFN. virt_to_phys() converts a kernel *direct mapped* virtual address to a physical address. That much is fine. The question is - what is usbm->mem? If that is anything other than an address returned by kmalloc() or from the normal page allocator, then virt_to_phys() will return garbage. In other words, if it comes from dma_alloc_coherent(), vmalloc() or ioremap(), using virt_to_phys() on it results in garbage. This aspect of virt_to_phys() has been well known about for ages; it's one of the fundamentals of kernel programming. -- RMK's Patch system: https://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] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 9:59 ` Russell King - ARM Linux admin @ 2019-08-08 10:02 ` Oliver Neukum 0 siblings, 0 replies; 11+ messages in thread From: Oliver Neukum @ 2019-08-08 10:02 UTC (permalink / raw) To: Russell King - ARM Linux admin, Greg KH Cc: yvahkhfo.1df7f8c2, security, linux-arm-kernel, linux-usb Am Donnerstag, den 08.08.2019, 10:59 +0100 schrieb Russell King - ARM Linux admin: > On Thu, Aug 08, 2019 at 10:58:11AM +0200, Greg KH wrote: > > But the main issue here is what exactly is this "fixing"? What is wrong > > with the existing code that non-x86 systems have such a problem with? > > Shouldn't all of these dma issues be handled by the platform with the > > remap_pfn_range() call itself? > > remap_pfn_range() takes a PFN. virt_to_phys() converts a kernel *direct > mapped* virtual address to a physical address. That much is fine. > > The question is - what is usbm->mem? If that is anything other than an > address returned by kmalloc() or from the normal page allocator, then > virt_to_phys() will return garbage. > > In other words, if it comes from dma_alloc_coherent(), vmalloc() or > ioremap(), using virt_to_phys() on it results in garbage. It comes from usb_alloc_coherent() -> hcd_buffer_alloc() -> hcd_buffer_alloc() That function is a bit complicated. so I rather quote than explain: if (hcd->localmem_pool) return gen_pool_dma_alloc(hcd->localmem_pool, size, dma) /* some USB hosts just use PIO */ if (!IS_ENABLED(CONFIG_HAS_DMA) || !is_device_dma_capable(bus->sysdev)) { *dma = ~(dma_addr_t) 0; return kmalloc(size, mem_flags); } for (i = 0; i < HCD_BUFFER_POOLS; i++) { if (size <= pool_max[i]) return dma_pool_alloc(hcd->pool[i], mem_flags, dma); } return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags); Regards Oliver ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 8:46 usb zero copy dma handling yvahkhfo.1df7f8c2 2019-08-08 8:58 ` Greg KH @ 2019-08-08 16:10 ` Christoph Hellwig 2019-08-08 16:12 ` Christoph Hellwig 2019-08-08 16:57 ` Russell King - ARM Linux admin 1 sibling, 2 replies; 11+ messages in thread From: Christoph Hellwig @ 2019-08-08 16:10 UTC (permalink / raw) To: yvahkhfo.1df7f8c2; +Cc: linux-usb, linux-arm-kernel, security On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > usbm->vma_use_count = 1; > INIT_LIST_HEAD(&usbm->memlist); > > +#ifdef CONFIG_X86 > if (remap_pfn_range(vma, vma->vm_start, > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > size, vma->vm_page_prot) < 0) { > +#else /* !CONFIG_X86 */ > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > + vma, mem, dma_handle, size) < 0) { > +#endif /* !CONFIG_X86 */ Doing the dma_mmap_coherent unconditionally is the right thing here. Gavin who is on Cc has been looking into that. Note that you'll also need this patch which I'm going to send to Linus this week before it properly works on x86: http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/197b3e665b82c6027be5c68a143233df7ce5224f ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 16:10 ` Christoph Hellwig @ 2019-08-08 16:12 ` Christoph Hellwig 2019-08-08 16:57 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2019-08-08 16:12 UTC (permalink / raw) To: yvahkhfo.1df7f8c2; +Cc: security, linux-usb, linux-arm-kernel On Thu, Aug 08, 2019 at 09:10:15AM -0700, Christoph Hellwig wrote: > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > > --- a/drivers/usb/core/devio.c > > +++ b/drivers/usb/core/devio.c > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > usbm->vma_use_count = 1; > > INIT_LIST_HEAD(&usbm->memlist); > > > > +#ifdef CONFIG_X86 > > if (remap_pfn_range(vma, vma->vm_start, > > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > > size, vma->vm_page_prot) < 0) { > > +#else /* !CONFIG_X86 */ > > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > > + vma, mem, dma_handle, size) < 0) { > > +#endif /* !CONFIG_X86 */ > > Doing the dma_mmap_coherent unconditionally is the right thing here. > Gavin who is on Cc has been looking into that. Ok, tht is assuming it always is dma_alloc_* memory which apparently it isn't. But the arch ifdef for sure is wrong. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: usb zero copy dma handling 2019-08-08 16:10 ` Christoph Hellwig 2019-08-08 16:12 ` Christoph Hellwig @ 2019-08-08 16:57 ` Russell King - ARM Linux admin 1 sibling, 0 replies; 11+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-08 16:57 UTC (permalink / raw) To: Christoph Hellwig Cc: yvahkhfo.1df7f8c2, security, linux-usb, linux-arm-kernel On Thu, Aug 08, 2019 at 09:10:15AM -0700, Christoph Hellwig wrote: > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote: > > --- a/drivers/usb/core/devio.c > > +++ b/drivers/usb/core/devio.c > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma) > > usbm->vma_use_count = 1; > > INIT_LIST_HEAD(&usbm->memlist); > > > > +#ifdef CONFIG_X86 > > if (remap_pfn_range(vma, vma->vm_start, > > virt_to_phys(usbm->mem) >> PAGE_SHIFT, > > size, vma->vm_page_prot) < 0) { > > +#else /* !CONFIG_X86 */ > > + if (dma_mmap_coherent(ps->dev->bus->sysdev, > > + vma, mem, dma_handle, size) < 0) { > > +#endif /* !CONFIG_X86 */ > > Doing the dma_mmap_coherent unconditionally is the right thing here. So what if usbm->mem is from kmalloc because the host doesn't support DMA? -- RMK's Patch system: https://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] 11+ messages in thread
end of thread, other threads:[~2019-08-08 16:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-08 8:46 usb zero copy dma handling yvahkhfo.1df7f8c2 2019-08-08 8:58 ` Greg KH 2019-08-08 9:46 ` Robin Murphy 2019-08-08 10:07 ` Greg KH 2019-08-08 10:43 ` Robin Murphy 2019-08-08 13:05 ` Greg KH 2019-08-08 9:59 ` Russell King - ARM Linux admin 2019-08-08 10:02 ` Oliver Neukum 2019-08-08 16:10 ` Christoph Hellwig 2019-08-08 16:12 ` Christoph Hellwig 2019-08-08 16:57 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).