Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Wed, Feb 24, 2016 at 02:30:08PM -0500, Sasha Levin wrote:
> I'm seeing the following warning while fuzzing:
> [ 1595.188189] WARNING: CPU: 3 PID: 26063 at mm/page_alloc.c:3207 
> __alloc_pages_nodemask+0x960/0x29e0()
> [ 1595.188287] Modules linked in:
> [ 1595.188316] CPU: 3 PID: 26063 Comm: syz-executor Not tainted 
> 4.5.0-rc5-next-20160223-sasha-00022-g03b30f1-dirty #2982

I think it was already established that one could cause kernel warnings if
trying to allocate large amounts of memory, but that the usbfs memory limits
could curb truly dangerous amounts. Someone please correct me if I'm
misunderstanding?

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Sasha Levin]

On 11/25/2015 07:19 PM, Steinar H. Gunderson wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:
> 
>  - Zerocopy; data no longer needs to be copied between the userspace
>and the kernel, but can instead be read directly by the driver from
>userspace's buffers. This works for all kinds of transfers (even if
>nonsensical for control and interrupt transfers); isochronous also
>no longer need to memset() the buffer to zero to avoid leaking kernel data.
> 
>  - Once the buffers are allocated, USB transfers can no longer fail due to
>memory fragmentation; previously, long-running programs could run into
>problems finding a large enough contiguous memory chunk, especially on
>embedded systems or at high rates.
> 
> Memory is allocated by using mmap() against the usbfs file descriptor,
> and similarly deallocated by munmap(). Once memory has been allocated,
> using it as pointers to a bulk or isochronous operation means you will
> automatically get zerocopy behavior. Note that this also means you cannot
> modify outgoing data until the transfer is complete. The same holds for
> data on the same cache lines as incoming data; DMA modifying them at the
> same time could lead to your changes being overwritten.
> 
> There's a new capability USBDEVFS_CAP_MMAP that userspace can query to see
> if the running kernel supports this functionality, if just trying mmap() is
> not acceptable.
> 
> Largely based on a patch by Markus Rechberger with some updates. The original
> patch can be found at:
> 
>   http://sundtek.de/support/devio_mmap_v0.4.diff
> 
> Signed-off-by: Steinar H. Gunderson 
> Signed-off-by: Markus Rechberger 
> Acked-by: Alan Stern 

Hi,

I'm seeing the following warning while fuzzing:

[ 1595.188189] WARNING: CPU: 3 PID: 26063 at mm/page_alloc.c:3207 
__alloc_pages_nodemask+0x960/0x29e0()

[ 1595.188287] Modules linked in:

[ 1595.188316] CPU: 3 PID: 26063 Comm: syz-executor Not tainted 
4.5.0-rc5-next-20160223-sasha-00022-g03b30f1-dirty #2982

[ 1595.188362]  11001460ce89 8800a30674d0 a03cecad 
0003

[ 1595.188380]  fbfff5829420 41b58ab3 abb334e2 
a03ceb15

[ 1595.188395]  9e5964c0 41b58ab3 abb4e542 
9e6ee830

[ 1595.188401] Call Trace:

[ 1595.188445] dump_stack (lib/dump_stack.c:53)
[ 1595.188529] warn_slowpath_common (kernel/panic.c:483)
[ 1595.188552] warn_slowpath_null (kernel/panic.c:517)
[ 1595.188561] __alloc_pages_nodemask (mm/page_alloc.c:3207 
mm/page_alloc.c:3467)
[ 1595.188768] alloc_pages_current (mm/mempolicy.c:2088)
[ 1595.188784] alloc_kmem_pages (mm/page_alloc.c:3648)
[ 1595.188802] kmalloc_order (mm/slab_common.c:1014)
[ 1595.188819] __kmalloc (include/linux/slab.h:397 include/linux/slab.h:404 
mm/slub.c:3573)
[ 1595.188870] hcd_buffer_alloc (include/linux/slab.h:477 
drivers/usb/core/buffer.c:130)
[ 1595.188934] usb_alloc_coherent (drivers/usb/core/usb.c:736)
[ 1595.188941] usbdev_mmap (drivers/usb/core/devio.c:243)
[ 1595.189014] mmap_region (mm/mmap.c:1502)
[ 1595.189335] do_mmap (mm/mmap.c:1282)
[ 1595.189352] vm_mmap_pgoff (mm/util.c:335)
[ 1595.189384] SyS_mmap_pgoff (mm/mmap.c:1331 mm/mmap.c:1289)
[ 1595.189429] SyS_mmap (arch/x86/kernel/sys_x86_64.c:86)
[ 1595.189437] entry_SYSCALL_64_fastpath (arch/x86/entry/entry_64.S:200)


Thanks,
Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Wed, Jan 06, 2016 at 04:22:12PM +0100, Peter Stuge wrote:
>>> Our interface for zero copy reads/writes is O_DIRECT, and that requires
>>> not special memory allocation, just proper alignment.
>> But that assumes you are using I/O using read()/write(). There's no way you
>> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or 
>> not.
> How about aio?

I don't really see how; a USB device does not look much like a file. (Where
would you stick the endpoint, for one? And how would you ever submit an URB
with multiple packets in it, which is essential?) It feels a bit like
trying to use UDP sockets with only read() and write().

In any case, the usbfs interface already exists and is stable. This is about
extending it; replacing it with something new from scratch to get zerocopy
would seem overkill.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Wed, 6 Jan 2016, Peter Stuge wrote:

> Steinar H. Gunderson wrote:
> > > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > > not special memory allocation, just proper alignment.
> > 
> > But that assumes you are using I/O using read()/write(). There's no way you
> > can shoehorn USB isochronous reads into the read() interface, O_DIRECT or 
> > not.
> 
> How about aio?

aio is not zerocopy.  And it also doesn't solve the memory allocation 
problem that originally affected both Steiner and Markus.

(Were you thinking of "asynchronous" instead of "isochronous"?)

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Peter Stuge]

Steinar H. Gunderson wrote:
> > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > not special memory allocation, just proper alignment.
> 
> But that assumes you are using I/O using read()/write(). There's no way you
> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

How about aio?


//Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Tue, Jan 05, 2016 at 10:49:49PM -0800, Christoph Hellwig wrote:
> This is a completely broken usage of the mmap interface.  if you use
> mmap on a device file you must use the actual mmap for the data
> transfer.

Really? V4L does exactly the same thing, from what I can see. It's just a way
of allocating memory with specific properties, roughly similar to hugetlbfs.

> Our interface for zero copy reads/writes is O_DIRECT, and that requires
> not special memory allocation, just proper alignment.

But that assumes you are using I/O using read()/write(). There's no way you
can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Wed, 6 Jan 2016, Steinar H. Gunderson wrote:

> On Tue, Jan 05, 2016 at 10:49:49PM -0800, Christoph Hellwig wrote:
> > This is a completely broken usage of the mmap interface.  if you use
> > mmap on a device file you must use the actual mmap for the data
> > transfer.
> 
> Really? V4L does exactly the same thing, from what I can see. It's just a way
> of allocating memory with specific properties, roughly similar to hugetlbfs.
> 
> > Our interface for zero copy reads/writes is O_DIRECT, and that requires
> > not special memory allocation, just proper alignment.
> 
> But that assumes you are using I/O using read()/write(). There's no way you
> can shoehorn USB isochronous reads into the read() interface, O_DIRECT or not.

Indeed, the I/O operations we are using with mmap here are not reads or 
writes; they are ioctls.  As far as I know, the kernel doesn't have any 
defined interface for zerocopy ioctls.

Furthermore, this approach _does_ use the mmap for data transfers.  I'm 
not sure what Christoph was referring to.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[David Laight]

From: Steinar H. Gunderson [mailto:se...@google.com]
> Sent: 26 November 2015 00:19

There is still a problem with this code, not sure how to fix it.

...
> +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
> +{
> + struct usb_dev_state *ps = usbm->ps;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>lock, flags);
> + --*count;
> + if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
...
> + module_put(THIS_MODULE);
> + } else {
> + spin_unlock_irqrestore(>lock, flags);
> + }
> +}
...
> +static void usbdev_vm_close(struct vm_area_struct *vma)
> +{
> + struct usb_memory *usbm = vma->vm_private_data;
> +
> + dec_usb_memory_use_count(usbm, >vma_use_count);
> +}

If the last reference to the module is for an mmap request
(which is the only reason to reference count the module here)
then the module_put() in dec_usb_memory_use_count()) returns
back into freed memory.

There isn't much the module can do about it either apart
from using kthread_run() to call module_put_and_exit()
and even that is somewhat racy (a sleep in the kthread
is probably enough).

The only real solution is for mmap() itself to take the
reference on the module.

David


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Tue, 5 Jan 2016, David Laight wrote:

> From: Steinar H. Gunderson [mailto:se...@google.com]
> > Sent: 26 November 2015 00:19
> 
> There is still a problem with this code, not sure how to fix it.
> 
> ...
> > +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
> > +{
> > +   struct usb_dev_state *ps = usbm->ps;
> > +   unsigned long flags;
> > +
> > +   spin_lock_irqsave(>lock, flags);
> > +   --*count;
> > +   if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
> ...
> > +   module_put(THIS_MODULE);
> > +   } else {
> > +   spin_unlock_irqrestore(>lock, flags);
> > +   }
> > +}
> ...
> > +static void usbdev_vm_close(struct vm_area_struct *vma)
> > +{
> > +   struct usb_memory *usbm = vma->vm_private_data;
> > +
> > +   dec_usb_memory_use_count(usbm, >vma_use_count);
> > +}
> 
> If the last reference to the module is for an mmap request
> (which is the only reason to reference count the module here)
> then the module_put() in dec_usb_memory_use_count()) returns
> back into freed memory.

Ooh, you're right.

> There isn't much the module can do about it either apart
> from using kthread_run() to call module_put_and_exit()
> and even that is somewhat racy (a sleep in the kthread
> is probably enough).
> 
> The only real solution is for mmap() itself to take the
> reference on the module.

I agree; the vm_operations_struct structure ought to have a .owner 
member.  But I don't feel like going through and changing all of them.

How do other drivers handle this problem?  Perhaps they don't face it
because they don't use DMA mapping to implement mmap.  A normal MMIO
sort of page mapping doesn't cause difficulties if you leave it in
place after the device is unregistered.

Or maybe I just don't understand the problem very well and the core 
kernel handles all of this for us.  I'll try asking.

kthread_run plus sleep and module_put_and_exit might turn out to be the
way to go. If anyone has a better suggestion, I'd like to hear it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[David Laight]

From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 05 January 2016 15:58
> On Tue, 5 Jan 2016, David Laight wrote:
> 
> > From: Steinar H. Gunderson [mailto:se...@google.com]
> > > Sent: 26 November 2015 00:19
> >
> > There is still a problem with this code, not sure how to fix it.
> >
> > ...
> > > +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
> > > +{
> > > + struct usb_dev_state *ps = usbm->ps;
> > > + unsigned long flags;
> > > +
> > > + spin_lock_irqsave(>lock, flags);
> > > + --*count;
> > > + if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
> > ...
> > > + module_put(THIS_MODULE);
> > > + } else {
> > > + spin_unlock_irqrestore(>lock, flags);
> > > + }
> > > +}
> > ...
> > > +static void usbdev_vm_close(struct vm_area_struct *vma)
> > > +{
> > > + struct usb_memory *usbm = vma->vm_private_data;
> > > +
> > > + dec_usb_memory_use_count(usbm, >vma_use_count);
> > > +}
> >
> > If the last reference to the module is for an mmap request
> > (which is the only reason to reference count the module here)
> > then the module_put() in dec_usb_memory_use_count()) returns
> > back into freed memory.
> 
> Ooh, you're right.

In fact every call of module_put(THIS_MODULE) is probably either buggy
or unnecessary.

> > There isn't much the module can do about it either apart
> > from using kthread_run() to call module_put_and_exit()
> > and even that is somewhat racy (a sleep in the kthread
> > is probably enough).
> >
> > The only real solution is for mmap() itself to take the
> > reference on the module.
> 
> I agree; the vm_operations_struct structure ought to have a .owner
> member.  But I don't feel like going through and changing all of them.

The .owner from the device can be copied into the 'vm_area_struct',
vm_operations_struct doesn't need changing.

> How do other drivers handle this problem?  Perhaps they don't face it
> because they don't use DMA mapping to implement mmap.  A normal MMIO
> sort of page mapping doesn't cause difficulties if you leave it in
> place after the device is unregistered.

It can cause issues.
If you remove a PCIe device (echo 1 >/sys/devices/pci*/*/remove) and don't
invalidate any corresponding mmap vma then the same bus addresses could
be assigned to a different card if a rescan is done.

I think that zap_page_range() could be called with parameters taken
from the vma in order to cause the .fault function be called.
However zap_page_range() isn't exported in mainline kernels.

I'm trying to get the same effect by calling remap_pfn_range() to
replace the dma/io page with a vmalloc()ed one - but so far have
only have multiple oops and system lockups.
It might be because I'm trying to remap pages for the wrong process
(or I might just have the args to remap_pfn_range() wrong).

There is a further problem that it is basically impossible to remove
timing windows between an application unmap() and a card removal event
because the required locks can't be obtained in the same order in
both cases.

> Or maybe I just don't understand the problem very well and the core
> kernel handles all of this for us.  I'll try asking.
> 
> kthread_run plus sleep and module_put_and_exit might turn out to be the
> way to go. If anyone has a better suggestion, I'd like to hear it.

If you know the device is open you can directly call module_put()
(with a mutex/sema held to block the close) since you know it
can't be the last reference.

David


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Markus Rechberger]

On Tue, Jan 5, 2016 at 4:57 PM, Alan Stern  wrote:
> On Tue, 5 Jan 2016, David Laight wrote:
>
>> From: Steinar H. Gunderson [mailto:se...@google.com]
>> > Sent: 26 November 2015 00:19
>>
>> There is still a problem with this code, not sure how to fix it.
>>
>> ...
>> > +static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
>> > +{
>> > +   struct usb_dev_state *ps = usbm->ps;
>> > +   unsigned long flags;
>> > +
>> > +   spin_lock_irqsave(>lock, flags);
>> > +   --*count;
>> > +   if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
>> ...
>> > +   module_put(THIS_MODULE);
>> > +   } else {
>> > +   spin_unlock_irqrestore(>lock, flags);
>> > +   }
>> > +}
>> ...
>> > +static void usbdev_vm_close(struct vm_area_struct *vma)
>> > +{
>> > +   struct usb_memory *usbm = vma->vm_private_data;
>> > +
>> > +   dec_usb_memory_use_count(usbm, >vma_use_count);
>> > +}
>>
>> If the last reference to the module is for an mmap request
>> (which is the only reason to reference count the module here)
>> then the module_put() in dec_usb_memory_use_count()) returns
>> back into freed memory.
>
> Ooh, you're right.
>
>> There isn't much the module can do about it either apart
>> from using kthread_run() to call module_put_and_exit()
>> and even that is somewhat racy (a sleep in the kthread
>> is probably enough).
>>
>> The only real solution is for mmap() itself to take the
>> reference on the module.
>
> I agree; the vm_operations_struct structure ought to have a .owner
> member.  But I don't feel like going through and changing all of them.
>
> How do other drivers handle this problem?  Perhaps they don't face it
> because they don't use DMA mapping to implement mmap.  A normal MMIO
> sort of page mapping doesn't cause difficulties if you leave it in
> place after the device is unregistered.
>
> Or maybe I just don't understand the problem very well and the core
> kernel handles all of this for us.  I'll try asking.
>
> kthread_run plus sleep and module_put_and_exit might turn out to be the
> way to go. If anyone has a better suggestion, I'd like to hear it.
>

you might check the video4linux implementation, they do DMA mapping of
videoframes but not sure if they run into the
same problem somehow.

Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Christoph Hellwig]

This is a completely broken usage of the mmap interface.  if you use
mmap on a device file you must use the actual mmap for the data
transfer.

Our interface for zero copy reads/writes is O_DIRECT, and that requires
not special memory allocation, just proper alignment.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Greg Kroah-Hartman]

On Thu, Nov 26, 2015 at 01:19:13AM +0100, Steinar H. Gunderson wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:

Please 'version' your patches, so that I have a chance to figure out
what patch is what, and what changed between patches.

otherwise the odds of me picking the "wrong" one is _very_ high...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Tue, Jan 05, 2016 at 04:11:43PM -0800, Greg Kroah-Hartman wrote:
>> Add a new interface for userspace to preallocate memory that can be
>> used with usbfs. This gives two primary benefits:
> Please 'version' your patches, so that I have a chance to figure out
> what patch is what, and what changed between patches.
> 
> otherwise the odds of me picking the "wrong" one is _very_ high...

OK. I won't make any attempt at reconstructing the history, but I'll resend
the one I just sent you as v2, ie. --reroll-count=2.

Somehow it feels like there should be a way to integrate this better into my
MUA, but hopefully this is soon all done anyway. :-)

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Mon, Jan 04, 2016 at 01:42:06AM +0100, Markus Rechberger wrote:
> Don't expect anyone else seriously testing this interface aside of me
> and Steinar.

FWIW, I got an email from the OpenKinect people, who are (as far as I know)
currently testing the patch together with my libusb-1.0 patch. It seems
they've been having similar problems about large isochronous memory
allocations failing.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Markus Rechberger]

On Mon, Jan 4, 2016 at 1:06 AM, Greg Kroah-Hartman
 wrote:
> On Mon, Jan 04, 2016 at 12:54:57AM +0100, Markus Rechberger wrote:
>> I would appreciate if this thing could speed up a little bit. We have
>> implemented our previous mmap support into some embedded systems
>> however we need to update quite a few systems if we are going to
>> support the current mmap version.
>> It's taking 2 months already for this small change.
>
> This is not a small change.  It's an api that we, not you, have to
> support for the next 20+ years.  Combine that with the holliday break,
> and it ended up taking a bit longer than normal.
>
>> When I did the patch initially I gave up and went my own way because
>> all this just takes way too long.
>
> No one appreciates your unecessary comments, please stop.
>

just improve that sloppy situation then there will not be any
unnecessary comments, it already took another month back then when I
did that stuff. So we're at 3 months now. I remember it took you a few
days to add some faulty Suse patches to this interface some years ago.
I even pointed out that this decreases the cpu load on the older intel
atom systems and no one cared either.
Anyway I'm waiting for this thing to complete finally.
Don't expect anyone else seriously testing this interface aside of me
and Steinar.

Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Markus Rechberger]

I would appreciate if this thing could speed up a little bit. We have
implemented our previous mmap support into some embedded systems
however we need to update quite a few systems if we are going to
support the current mmap version.
It's taking 2 months already for this small change.

When I did the patch initially I gave up and went my own way because
all this just takes way too long.

Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Greg Kroah-Hartman]

On Mon, Jan 04, 2016 at 12:54:57AM +0100, Markus Rechberger wrote:
> I would appreciate if this thing could speed up a little bit. We have
> implemented our previous mmap support into some embedded systems
> however we need to update quite a few systems if we are going to
> support the current mmap version.
> It's taking 2 months already for this small change.

This is not a small change.  It's an api that we, not you, have to
support for the next 20+ years.  Combine that with the holliday break,
and it ended up taking a bit longer than normal.

> When I did the patch initially I gave up and went my own way because
> all this just takes way too long.

No one appreciates your unecessary comments, please stop.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Peter Stuge]

Markus Rechberger wrote:
> just improve that sloppy situation

Dude I don't care who you are, your attitude sucks, and flushes any
merit you have down the drain. It makes you look super bad. :)


> we're at 3 months now

Right, so chill out, watch a movie, go to a party, go to several,
have some fun, enjoy life - and frohes neues Jahr!


This is a piece of software that is improving by people working
together. Let it take the time it takes.


> Don't expect anyone else seriously testing this interface aside of me
> and Steinar.

That's a silly comment. You may think you know, but you have no idea.

As Greg said you are not the one having to maintain it. If I were you
I would be happy that it is getting added so smoothly! I know that I
am happy that this is getting added so smoothly!

You may know that I was the maintainer of libusb for some time, and I
think it's fantastic to have an opportunity to cut through all the
layers and get a performance boost. You may think it's lame that it's
taking longer on this platform than on others, but well, that's just
how it is.


Cheer up! New year, new opportunities.

//Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Fri, Dec 25, 2015 at 01:36:44PM -0500, Alan Stern wrote:
> That's okay; lots of drivers do this and people expect it.  It also 
> reduces the total amount of code.

Okay, fixed.

> > > > +   mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, 
> > > > _handle);
> > > Shouldn't this be GFP_USER to let limits apply?
> > This I don't really know anything about. Alan?
> There doesn't appear to be very much difference between them.  As far
> as I can tell from the documentation in include/linux/gfp.h, GPF_USER
> would indeed be more appropriate.

OK, changed this one to GFP_USER, but kept all the others at GFP_KERNEL since
it seemed the most consistent with the other code.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Fri, Dec 25, 2015 at 09:50:50AM +0100, Oliver Neukum wrote:
> > +   ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> > +   if (ret) {
> > +   module_put(THIS_MODULE);
> > +   return ret;
> > +   }
> 
> Could you do the error handling in a unified manner with "goto"?

Yes and no. I couldn't do it fully unified; note the error path below that
uses usbfs_decrease_memory_usage() instead. I could do it sort-of-unified,
but it ended up being one of those label cascades, of course:

return 0;

error_free_usbm:
kfree(usbm);
error_decrease_mem:
usbfs_decrease_memory_usage(size + sizeof(struct usb_memory));
error_module_put:
module_put(THIS_MODULE);
return ret;

> > +   mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, _handle);
> Shouldn't this be GFP_USER to let limits apply?

This I don't really know anything about. Alan?

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Oliver Neukum]

On Thu, 2015-11-26 at 01:19 +0100, Steinar H. Gunderson wrote:

Hi,

just two small nits to pick:

> +static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +   struct usb_memory *usbm = NULL;
> +   struct usb_dev_state *ps = file->private_data;
> +   size_t size = vma->vm_end - vma->vm_start;
> +   void *mem;
> +   unsigned long flags;
> +   dma_addr_t dma_handle;
> +   int ret;
> +
> +   ret = try_module_get(THIS_MODULE);
> +   if (ret)
> +   return ret;
> +
> +   ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> +   if (ret) {
> +   module_put(THIS_MODULE);
> +   return ret;
> +   }

Could you do the error handling in a unified manner with "goto"?

> +
> +   usbm = kzalloc(sizeof(struct usb_memory), GFP_KERNEL);
> +   if (!usbm) {
> +   module_put(THIS_MODULE);
> +   usbfs_decrease_memory_usage(
> +   size + sizeof(struct usb_memory));
> +   return -ENOMEM;
> +   }
> +
> +   mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, _handle);

Shouldn't this be GFP_USER to let limits apply?

> +   if (!mem) {
> +   module_put(THIS_MODULE);
> +   usbfs_decrease_memory_usage(
> +   size + sizeof(struct usb_memory));
> +   kfree(usbm);
> +   return -ENOMEM;
> +   }
> +
> +   memset(mem, 0, size);
> +
> +   usbm->mem = mem;
> +   usbm->dma_handle = dma_handle;
> +   usbm->size = size;
> +   usbm->ps = ps;
> +   usbm->vm_start = vma->vm_start;
> +   usbm->vma_use_count = 1;
> +   INIT_LIST_HEAD(>memlist);
> +
> +   if (remap_pfn_range(vma, vma->vm_start,
> +   virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> +   size, vma->vm_page_prot) < 0) {
> +   dec_usb_memory_use_count(usbm, >vma_use_count);
> +   return -EAGAIN;
> +   }
> +
> +   vma->vm_flags |= VM_IO;
> +   vma->vm_flags |= (VM_DONTEXPAND | VM_DONTDUMP);
> +   vma->vm_ops = _vm_ops;
> +   vma->vm_private_data = usbm;
> +
> +   spin_lock_irqsave(>lock, flags);
> +   list_add_tail(>memlist, >memory_list);
> +   spin_unlock_irqrestore(>lock, flags);
> +
> +   return 0;
> +}

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Fri, 25 Dec 2015, Steinar H. Gunderson wrote:

> On Fri, Dec 25, 2015 at 09:50:50AM +0100, Oliver Neukum wrote:
> > > +   ret = usbfs_increase_memory_usage(size + sizeof(struct 
> > > usb_memory));
> > > +   if (ret) {
> > > +   module_put(THIS_MODULE);
> > > +   return ret;
> > > +   }
> > 
> > Could you do the error handling in a unified manner with "goto"?
> 
> Yes and no. I couldn't do it fully unified; note the error path below that
> uses usbfs_decrease_memory_usage() instead. I could do it sort-of-unified,
> but it ended up being one of those label cascades, of course:
> 
> return 0;
> 
> error_free_usbm:
> kfree(usbm);
> error_decrease_mem:
> usbfs_decrease_memory_usage(size + sizeof(struct usb_memory));
> error_module_put:
> module_put(THIS_MODULE);
> return ret;

That's okay; lots of drivers do this and people expect it.  It also 
reduces the total amount of code.

> > > +   mem = usb_alloc_coherent(ps->dev, size, GFP_KERNEL, _handle);
> > Shouldn't this be GFP_USER to let limits apply?
> 
> This I don't really know anything about. Alan?

There doesn't appear to be very much difference between them.  As far
as I can tell from the documentation in include/linux/gfp.h, GPF_USER
would indeed be more appropriate.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Tue, Dec 22, 2015 at 10:38:57AM -0500, Alan Stern wrote:
> Steinar, you can look at those if you want to.  But I really don't 
> think anything more is needed other than the try_module_get and 
> module_put calls.  At least, the Linux Device Drivers book doesn't 
> mention anything else.

OK. I'll send a version with try_module_get/module_put added, then. Tested on
top of 4.3.3.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Tue, 22 Dec 2015, Oliver Neukum wrote:

> On Tue, 2015-12-22 at 01:12 +0100, Steinar H. Gunderson wrote:
> > On Tue, Dec 15, 2015 at 03:32:45PM +, David Laight wrote:
> > > That still isn't entirely correct.
> > > 
> > > Someone with more knowledge than either of us has needs to sort out
> > > how to handle this properly.
> > 
> > This discussion sort of stalled. Short of actually becoming a kernel VM
> > expert (not very likely), is there anything I can do to help move this
> > forward?
> 
> It may be profitable to look at how other drivers do this.
> graphics should have the same issues. Maybe sound, too.

Steinar, you can look at those if you want to.  But I really don't 
think anything more is needed other than the try_module_get and 
module_put calls.  At least, the Linux Device Drivers book doesn't 
mention anything else.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Tue, Dec 15, 2015 at 03:32:45PM +, David Laight wrote:
> That still isn't entirely correct.
> 
> Someone with more knowledge than either of us has needs to sort out
> how to handle this properly.

This discussion sort of stalled. Short of actually becoming a kernel VM
expert (not very likely), is there anything I can do to help move this
forward?

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Oliver Neukum]

On Tue, 2015-12-22 at 01:12 +0100, Steinar H. Gunderson wrote:
> On Tue, Dec 15, 2015 at 03:32:45PM +, David Laight wrote:
> > That still isn't entirely correct.
> > 
> > Someone with more knowledge than either of us has needs to sort out
> > how to handle this properly.
> 
> This discussion sort of stalled. Short of actually becoming a kernel VM
> expert (not very likely), is there anything I can do to help move this
> forward?

It may be profitable to look at how other drivers do this.
graphics should have the same issues. Maybe sound, too.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Tue, 15 Dec 2015, David Laight wrote:

> > Good point.  So it seems that the try_module_get() / module_put()
> > solution is the only one.
> 
> That still isn't entirely correct.
> 
> Someone with more knowledge than either of us has needs to sort out
> how to handle this properly.
> 
> Before calling dma_free_coherent() you (and I) need to invalidate all
> the vma mappings that reference the memory - so that any accesses
> will call the vma_ops.fault() code which will return VM_FAULT_SIGBUS.
> Then you need to hold the module present until all the mappings are
> removed.

Don't the vma mappings get invalidated before the
vm_operations_struct's ->close method is called?  In other words, when
a process calls munmap() or exits?  The try_module_get() will hold the 
module present until the ->close method has been called for all the 
mappings.

> This ordering is more important if vm_iomap_memory() is used since
> iounmap() has to be called from the .remove() entry for the hardware.

Fortunately we're not concerned with iomem.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[David Laight]

From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 14 December 2015 17:59
> On Mon, 14 Dec 2015, David Laight wrote:
> 
> > From: Alan Stern
> > > Sent: 14 December 2015 15:28
> > ...
> > > I thought of something else, a more serious problem.  According to the
> > > man page for munmap(2), closing the file descriptor does not unmap the
> > > region.  Therefore we need to explicitly make sure that the usbcore
> > > module cannot be unloaded while any memory mappings exist, by calling
> > > try_module_get(THIS_MODULE) when a mapping is created and
> > > module_put(THIS_MODULE) when a mapping is removed.
> >
> > Are you sure this is a problem?
> 
> I haven't actually tried to provoke a crash, if that's what you're
> asking.  Apart from that, yes, I'm sure this is a problem.

There are certainly potential issues with both remap_pfn_range()
and vm_iomap_memory().

> > It is more likely that either:
> > 1) the mmap() holds a reference to the physical pages - so they don't
> >   get freed even though the driver frees its reference to the memory.
> 
> The problem has nothing to do with the pages being freed.
> 
> > 2) when the driver frees the memory the user-space page tables are
> >   invalidated - so the next access generates EFAULT.
> 
> I'm not sure what you're getting at here.  The page tables are
> invalidated when the user program calls munmap (or exits), and the
> driver frees the memory when that happens.
> 
> > Not that I've done the experiment (or tried to read the code).
> >
> > If you need to monitor the mapping, you need to allow for partial unmaps.
> 
> That's not the problem either -- although I don't know how we should
> handle partial unmaps.  Can we disallow them?
> 
> The problem is that the user can map the memory, then close the file
> descriptor, then rmmod the usbcore module, then unmap the memory
> region.  When the unmap occurs, the memory subsystem will try to
> dereference the usbdev_vm_ops structure -- which has been unloaded
> along with the rest of usbcore.  This will cause an oops.

I was thinking that some side effect of dma_free_coherent() would
find the user mapping for the pages, force unmap them and invalidate
the user PTE (although the address range would have to remain reserved).
Maybe that is wishful thinking.

Similarly for iounmap().

> In theory we could prevent this problem by unmapping the memory when
> the file descriptor is closed.  But doing so would violate the
> guarantee in the munmap(2) documentation.

That doesn't help.
The mapping is inherited by fork() but you only see the last close().
So the close() need not be in the context of the process that has
the pages mapped.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[David Laight]

From: Alan Stern
> Sent: 15 December 2015 15:13
...
> > I was thinking that some side effect of dma_free_coherent() would
> > find the user mapping for the pages, force unmap them and invalidate
> > the user PTE (although the address range would have to remain reserved).
> > Maybe that is wishful thinking.
> 
> It is.  dma_free_coherent() does nothing but remove the DMA mapping and
> deallocate the memory.  It doesn't touch any user page mappings.
> 
> > Similarly for iounmap().
> >
> > > In theory we could prevent this problem by unmapping the memory when
> > > the file descriptor is closed.  But doing so would violate the
> > > guarantee in the munmap(2) documentation.
> >
> > That doesn't help.
> > The mapping is inherited by fork() but you only see the last close().
> > So the close() need not be in the context of the process that has
> > the pages mapped.
> 
> Good point.  So it seems that the try_module_get() / module_put()
> solution is the only one.

That still isn't entirely correct.

Someone with more knowledge than either of us has needs to sort out
how to handle this properly.

Before calling dma_free_coherent() you (and I) need to invalidate all
the vma mappings that reference the memory - so that any accesses
will call the vma_ops.fault() code which will return VM_FAULT_SIGBUS.
Then you need to hold the module present until all the mappings are
removed.

This ordering is more important if vm_iomap_memory() is used since
iounmap() has to be called from the .remove() entry for the hardware.

David


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Tue, 15 Dec 2015, David Laight wrote:

> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> > Sent: 14 December 2015 17:59
> > On Mon, 14 Dec 2015, David Laight wrote:
> > 
> > > From: Alan Stern
> > > > Sent: 14 December 2015 15:28
> > > ...
> > > > I thought of something else, a more serious problem.  According to the
> > > > man page for munmap(2), closing the file descriptor does not unmap the
> > > > region.  Therefore we need to explicitly make sure that the usbcore
> > > > module cannot be unloaded while any memory mappings exist, by calling
> > > > try_module_get(THIS_MODULE) when a mapping is created and
> > > > module_put(THIS_MODULE) when a mapping is removed.
> > >
> > > Are you sure this is a problem?
> > 
> > I haven't actually tried to provoke a crash, if that's what you're
> > asking.  Apart from that, yes, I'm sure this is a problem.
> 
> There are certainly potential issues with both remap_pfn_range()
> and vm_iomap_memory().

What are you thinking of?

> > > It is more likely that either:
> > > 1) the mmap() holds a reference to the physical pages - so they don't
> > >   get freed even though the driver frees its reference to the memory.
> > 
> > The problem has nothing to do with the pages being freed.
> > 
> > > 2) when the driver frees the memory the user-space page tables are
> > >   invalidated - so the next access generates EFAULT.
> > 
> > I'm not sure what you're getting at here.  The page tables are
> > invalidated when the user program calls munmap (or exits), and the
> > driver frees the memory when that happens.
> > 
> > > Not that I've done the experiment (or tried to read the code).
> > >
> > > If you need to monitor the mapping, you need to allow for partial unmaps.
> > 
> > That's not the problem either -- although I don't know how we should
> > handle partial unmaps.  Can we disallow them?
> > 
> > The problem is that the user can map the memory, then close the file
> > descriptor, then rmmod the usbcore module, then unmap the memory
> > region.  When the unmap occurs, the memory subsystem will try to
> > dereference the usbdev_vm_ops structure -- which has been unloaded
> > along with the rest of usbcore.  This will cause an oops.
> 
> I was thinking that some side effect of dma_free_coherent() would
> find the user mapping for the pages, force unmap them and invalidate
> the user PTE (although the address range would have to remain reserved).
> Maybe that is wishful thinking.

It is.  dma_free_coherent() does nothing but remove the DMA mapping and 
deallocate the memory.  It doesn't touch any user page mappings.

> Similarly for iounmap().
> 
> > In theory we could prevent this problem by unmapping the memory when
> > the file descriptor is closed.  But doing so would violate the
> > guarantee in the munmap(2) documentation.
> 
> That doesn't help.
> The mapping is inherited by fork() but you only see the last close().
> So the close() need not be in the context of the process that has
> the pages mapped.

Good point.  So it seems that the try_module_get() / module_put() 
solution is the only one.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Mon, Dec 14, 2015 at 04:41:24PM +0800, Peter Chen wrote:
>> +ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
>> +if (ret) {
>> +return ret;
>> +}
> The braces are not needed.

New patch attached.

/* Steinar */
-- 
Software Engineer, Google Switzerland
>From 659c4025a0e182a1a205fc4d945a68b8cb3602f0 Mon Sep 17 00:00:00 2001
From: "Steinar H. Gunderson" 
Date: Thu, 26 Nov 2015 01:19:13 +0100
Subject: [PATCH] Add support for usbfs zerocopy.
To: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org,mrechber...@gmail.com

Add a new interface for userspace to preallocate memory that can be
used with usbfs. This gives two primary benefits:

 - Zerocopy; data no longer needs to be copied between the userspace
   and the kernel, but can instead be read directly by the driver from
   userspace's buffers. This works for all kinds of transfers (even if
   nonsensical for control and interrupt transfers); isochronous also
   no longer need to memset() the buffer to zero to avoid leaking kernel data.

 - Once the buffers are allocated, USB transfers can no longer fail due to
   memory fragmentation; previously, long-running programs could run into
   problems finding a large enough contiguous memory chunk, especially on
   embedded systems or at high rates.

Memory is allocated by using mmap() against the usbfs file descriptor,
and similarly deallocated by munmap(). Once memory has been allocated,
using it as pointers to a bulk or isochronous operation means you will
automatically get zerocopy behavior. Note that this also means you cannot
modify outgoing data until the transfer is complete. The same holds for
data on the same cache lines as incoming data; DMA modifying them at the
same time could lead to your changes being overwritten.

There's a new capability USBDEVFS_CAP_MMAP that userspace can query to see
if the running kernel supports this functionality, if just trying mmap() is
not acceptable.

Largely based on a patch by Markus Rechberger with some updates. The original
patch can be found at:

  http://sundtek.de/support/devio_mmap_v0.4.diff

Signed-off-by: Steinar H. Gunderson 
Signed-off-by: Markus Rechberger 
Acked-by: Alan Stern 
---
 drivers/usb/core/devio.c  | 229 +-
 include/uapi/linux/usbdevice_fs.h |   1 +
 2 files changed, 205 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 38ae877c..f64dd3a 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -50,6 +50,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -69,6 +70,7 @@ struct usb_dev_state {
 	spinlock_t lock;/* protects the async urb lists */
 	struct list_head async_pending;
 	struct list_head async_completed;
+	struct list_head memory_list;
 	wait_queue_head_t wait; /* wake up if a request completed */
 	unsigned int discsignr;
 	struct pid *disc_pid;
@@ -79,6 +81,17 @@ struct usb_dev_state {
 	u32 disabled_bulk_eps;
 };
 
+struct usb_memory {
+	struct list_head memlist;
+	int vma_use_count;
+	int urb_use_count;
+	u32 size;
+	void *mem;
+	dma_addr_t dma_handle;
+	unsigned long vm_start;
+	struct usb_dev_state *ps;
+};
+
 struct async {
 	struct list_head asynclist;
 	struct usb_dev_state *ps;
@@ -89,6 +102,7 @@ struct async {
 	void __user *userbuffer;
 	void __user *userurb;
 	struct urb *urb;
+	struct usb_memory *usbm;
 	unsigned int mem_usage;
 	int status;
 	u32 secid;
@@ -157,6 +171,107 @@ static int connected(struct usb_dev_state *ps)
 			ps->dev->state != USB_STATE_NOTATTACHED);
 }
 
+static void dec_usb_memory_use_count(struct usb_memory *usbm, int *count)
+{
+	struct usb_dev_state *ps = usbm->ps;
+	unsigned long flags;
+
+	spin_lock_irqsave(>lock, flags);
+	--*count;
+	if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
+		list_del(>memlist);
+		spin_unlock_irqrestore(>lock, flags);
+
+		usb_free_coherent(ps->dev, usbm->size, usbm->mem,
+usbm->dma_handle);
+		usbfs_decrease_memory_usage(
+			usbm->size + sizeof(struct usb_memory));
+		kfree(usbm);
+	} else {
+		spin_unlock_irqrestore(>lock, flags);
+	}
+}
+
+static void usbdev_vm_open(struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = vma->vm_private_data;
+	unsigned long flags;
+
+	spin_lock_irqsave(>ps->lock, flags);
+	++usbm->vma_use_count;
+	spin_unlock_irqrestore(>ps->lock, flags);
+}
+
+static void usbdev_vm_close(struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = vma->vm_private_data;
+
+	dec_usb_memory_use_count(usbm, >vma_use_count);
+}
+
+struct vm_operations_struct usbdev_vm_ops = {
+	.open = usbdev_vm_open,
+	.close = usbdev_vm_close
+};
+
+static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct usb_memory *usbm = NULL;
+	struct usb_dev_state *ps = file->private_data;
+	size_t size = vma->vm_end - vma->vm_start;
+	

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Mon, 14 Dec 2015, Steinar H. Gunderson wrote:

> On Mon, Dec 14, 2015 at 04:41:24PM +0800, Peter Chen wrote:
> >> +  ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> >> +  if (ret) {
> >> +  return ret;
> >> +  }
> > The braces are not needed.
> 
> New patch attached.

I thought of something else, a more serious problem.  According to the
man page for munmap(2), closing the file descriptor does not unmap the
region.  Therefore we need to explicitly make sure that the usbcore
module cannot be unloaded while any memory mappings exist, by calling
try_module_get(THIS_MODULE) when a mapping is created and
module_put(THIS_MODULE) when a mapping is removed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[David Laight]

From: Alan Stern
> Sent: 14 December 2015 15:28
...
> I thought of something else, a more serious problem.  According to the
> man page for munmap(2), closing the file descriptor does not unmap the
> region.  Therefore we need to explicitly make sure that the usbcore
> module cannot be unloaded while any memory mappings exist, by calling
> try_module_get(THIS_MODULE) when a mapping is created and
> module_put(THIS_MODULE) when a mapping is removed.

Are you sure this is a problem?
It is more likely that either:
1) the mmap() holds a reference to the physical pages - so they don't
  get freed even though the driver frees its reference to the memory.
2) when the driver frees the memory the user-space page tables are
  invalidated - so the next access generates EFAULT.

Not that I've done the experiment (or tried to read the code).

If you need to monitor the mapping, you need to allow for partial unmaps.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Mon, 14 Dec 2015, David Laight wrote:

> From: Alan Stern
> > Sent: 14 December 2015 15:28
> ...
> > I thought of something else, a more serious problem.  According to the
> > man page for munmap(2), closing the file descriptor does not unmap the
> > region.  Therefore we need to explicitly make sure that the usbcore
> > module cannot be unloaded while any memory mappings exist, by calling
> > try_module_get(THIS_MODULE) when a mapping is created and
> > module_put(THIS_MODULE) when a mapping is removed.
> 
> Are you sure this is a problem?

I haven't actually tried to provoke a crash, if that's what you're 
asking.  Apart from that, yes, I'm sure this is a problem.

> It is more likely that either:
> 1) the mmap() holds a reference to the physical pages - so they don't
>   get freed even though the driver frees its reference to the memory.

The problem has nothing to do with the pages being freed.

> 2) when the driver frees the memory the user-space page tables are
>   invalidated - so the next access generates EFAULT.

I'm not sure what you're getting at here.  The page tables are 
invalidated when the user program calls munmap (or exits), and the 
driver frees the memory when that happens.

> Not that I've done the experiment (or tried to read the code).
> 
> If you need to monitor the mapping, you need to allow for partial unmaps.

That's not the problem either -- although I don't know how we should 
handle partial unmaps.  Can we disallow them?

The problem is that the user can map the memory, then close the file
descriptor, then rmmod the usbcore module, then unmap the memory
region.  When the unmap occurs, the memory subsystem will try to
dereference the usbdev_vm_ops structure -- which has been unloaded
along with the rest of usbcore.  This will cause an oops.

In theory we could prevent this problem by unmapping the memory when 
the file descriptor is closed.  But doing so would violate the 
guarantee in the munmap(2) documentation.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Peter Chen]

On Thu, Nov 26, 2015 at 01:19:13AM +0100, Steinar H. Gunderson wrote:
> +static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + struct usb_memory *usbm = NULL;
> + struct usb_dev_state *ps = file->private_data;
> + size_t size = vma->vm_end - vma->vm_start;
> + void *mem;
> + unsigned long flags;
> + dma_addr_t dma_handle;
> + int ret;
> +
> + ret = usbfs_increase_memory_usage(size + sizeof(struct usb_memory));
> + if (ret) {
> + return ret;
> + }

The braces are not needed.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Greg Kroah-Hartman]

On Thu, Nov 26, 2015 at 01:19:13AM +0100, Steinar H. Gunderson wrote:
> Add a new interface for userspace to preallocate memory that can be
> used with usbfs. This gives two primary benefits:
> 
>  - Zerocopy; data no longer needs to be copied between the userspace
>and the kernel, but can instead be read directly by the driver from
>userspace's buffers. This works for all kinds of transfers (even if
>nonsensical for control and interrupt transfers); isochronous also
>no longer need to memset() the buffer to zero to avoid leaking kernel data.
> 
>  - Once the buffers are allocated, USB transfers can no longer fail due to
>memory fragmentation; previously, long-running programs could run into
>problems finding a large enough contiguous memory chunk, especially on
>embedded systems or at high rates.
> 
> Memory is allocated by using mmap() against the usbfs file descriptor,
> and similarly deallocated by munmap(). Once memory has been allocated,
> using it as pointers to a bulk or isochronous operation means you will
> automatically get zerocopy behavior. Note that this also means you cannot
> modify outgoing data until the transfer is complete. The same holds for
> data on the same cache lines as incoming data; DMA modifying them at the
> same time could lead to your changes being overwritten.
> 
> There's a new capability USBDEVFS_CAP_MMAP that userspace can query to see
> if the running kernel supports this functionality, if just trying mmap() is
> not acceptable.

Shouldn't there also be some sort of documentation update with this
patch as well, so that people know of the new functionality we are
adding?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Wed, Dec 09, 2015 at 09:29:20AM -0500, Greg Kroah-Hartman wrote:
> Shouldn't there also be some sort of documentation update with this
> patch as well, so that people know of the new functionality we are
> adding?

I can write documentation if you can point me to a reasonable place.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Alan Stern]

On Wed, 9 Dec 2015, Steinar H. Gunderson wrote:

> On Wed, Dec 09, 2015 at 09:29:20AM -0500, Greg Kroah-Hartman wrote:
> > Shouldn't there also be some sort of documentation update with this
> > patch as well, so that people know of the new functionality we are
> > adding?
> 
> I can write documentation if you can point me to a reasonable place.

The only documentation I know of for usbfs is a dreadfully out-of-date 
section in Documentation/DocBook/usb.tmpl.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[Steinar H. Gunderson]

On Wed, Dec 09, 2015 at 10:39:35AM -0500, Alan Stern wrote:
>> I can write documentation if you can point me to a reasonable place.
> The only documentation I know of for usbfs is a dreadfully out-of-date 
> section in Documentation/DocBook/usb.tmpl.

I don't volunteer to get your documentation in shape there. :-)

I would guess most usbfs users go through libusb. I have a patch against
libusb that I'll send after the kernel patch is merged (it's hard to argue
for inclusion of something that is not in mainline), and it includes
documentation for the newly added functions, including zerocopy caveats.

/* Steinar */
-- 
Software Engineer, Google Switzerland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[kbuild test robot]

Hi Steinar,

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.4-rc3 next-20151203]

url:
https://github.com/0day-ci/linux/commits/Steinar-H-Gunderson/Add-support-for-usbfs-zerocopy/20151206-034258
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: xtensa-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
from include/linux/fs.h:4,
from drivers/usb/core/devio.c:37:
   drivers/usb/core/devio.c: In function 'proc_do_submiturb':
>> include/linux/err.h:21:38: warning: comparison between pointer and integer
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 ^
   include/linux/compiler.h:166:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
 ^
>> drivers/usb/core/devio.c:1617:6: note: in expansion of macro 'IS_ERR_VALUE'
 if (IS_ERR_VALUE(as->usbm)) {
 ^

vim +/IS_ERR_VALUE +1617 drivers/usb/core/devio.c

  1601  ret = -EFAULT;
  1602  goto error;
  1603  }
  1604  as = alloc_async(number_of_packets);
  1605  if (!as) {
  1606  ret = -ENOMEM;
  1607  goto error;
  1608  }
  1609  
  1610  u += sizeof(struct async) + sizeof(struct urb) + 
uurb->buffer_length +
  1611   num_sgs * sizeof(struct scatterlist);
  1612  ret = usbfs_increase_memory_usage(u);
  1613  if (ret)
  1614  goto error;
  1615  as->mem_usage = u;
  1616  as->usbm = find_memory_area(ps, uurb);
> 1617  if (IS_ERR_VALUE(as->usbm)) {
  1618  ret = PTR_ERR(as->usbm);
  1619  goto error;
  1620  }
  1621  
  1622  if (num_sgs) {
  1623  as->urb->sg = kmalloc(num_sgs * sizeof(struct 
scatterlist),
  1624  GFP_KERNEL);
  1625  if (!as->urb->sg) {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH] Add support for usbfs zerocopy.

[kbuild test robot]

Hi Steinar,

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.4-rc3 next-20151203]

url:
https://github.com/0day-ci/linux/commits/Steinar-H-Gunderson/Add-support-for-usbfs-zerocopy/20151206-034258
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:228:8: sparse: attribute 'no_sanitize_address': 
unknown attribute
>> drivers/usb/core/devio.c:1617:13: sparse: incompatible types for operation 
>> (>=)
   drivers/usb/core/devio.c:1617:13:left side has type struct usb_memory 
*usbm
   drivers/usb/core/devio.c:1617:13:right side has type unsigned long 
[unsigned] 
   In file included from include/linux/linkage.h:4:0,
from include/linux/fs.h:4,
from drivers/usb/core/devio.c:37:
   drivers/usb/core/devio.c: In function 'proc_do_submiturb':
   include/linux/err.h:21:38: warning: comparison between pointer and integer
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 ^
   include/linux/compiler.h:166:42: note: in definition of macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
 ^
   drivers/usb/core/devio.c:1617:6: note: in expansion of macro 'IS_ERR_VALUE'
 if (IS_ERR_VALUE(as->usbm)) {
 ^

vim +1617 drivers/usb/core/devio.c

  1601  ret = -EFAULT;
  1602  goto error;
  1603  }
  1604  as = alloc_async(number_of_packets);
  1605  if (!as) {
  1606  ret = -ENOMEM;
  1607  goto error;
  1608  }
  1609  
  1610  u += sizeof(struct async) + sizeof(struct urb) + 
uurb->buffer_length +
  1611   num_sgs * sizeof(struct scatterlist);
  1612  ret = usbfs_increase_memory_usage(u);
  1613  if (ret)
  1614  goto error;
  1615  as->mem_usage = u;
  1616  as->usbm = find_memory_area(ps, uurb);
> 1617  if (IS_ERR_VALUE(as->usbm)) {
  1618  ret = PTR_ERR(as->usbm);
  1619  goto error;
  1620  }
  1621  
  1622  if (num_sgs) {
  1623  as->urb->sg = kmalloc(num_sgs * sizeof(struct 
scatterlist),
  1624  GFP_KERNEL);
  1625  if (!as->urb->sg) {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Add support for usbfs zerocopy.

[kbuild test robot]

Hi Steinar,

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.4-rc3 next-20151203]

url:
https://github.com/0day-ci/linux/commits/Steinar-H-Gunderson/Add-support-for-usbfs-zerocopy/20151206-034258
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: x86_64-randconfig-x006-201549 (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/linkage.h:4:0,
from include/linux/fs.h:4,
from drivers/usb/core/devio.c:37:
   drivers/usb/core/devio.c: In function 'proc_do_submiturb':
   include/linux/err.h:21:38: warning: comparison between pointer and integer
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 ^
   include/linux/compiler.h:137:45: note: in definition of macro 'unlikely'
#  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : 
__branch_check__(x, 0))
^
   drivers/usb/core/devio.c:1617:6: note: in expansion of macro 'IS_ERR_VALUE'
 if (IS_ERR_VALUE(as->usbm)) {
 ^
   include/linux/err.h:21:38: warning: comparison between pointer and integer
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 ^
   include/linux/compiler.h:137:53: note: in definition of macro 'unlikely'
#  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : 
__branch_check__(x, 0))
^
   drivers/usb/core/devio.c:1617:6: note: in expansion of macro 'IS_ERR_VALUE'
 if (IS_ERR_VALUE(as->usbm)) {
 ^
   include/linux/err.h:21:38: warning: comparison between pointer and integer
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
 ^
   include/linux/compiler.h:110:47: note: in definition of macro 
'likely_notrace'
#define likely_notrace(x) __builtin_expect(!!(x), 1)
  ^
   include/linux/compiler.h:137:58: note: in expansion of macro 
'__branch_check__'
#  define unlikely(x) (__builtin_constant_p(x) ? !!(x) : 
__branch_check__(x, 0))
 ^
>> include/linux/err.h:21:25: note: in expansion of macro 'unlikely'
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
^
   drivers/usb/core/devio.c:1617:6: note: in expansion of macro 'IS_ERR_VALUE'
 if (IS_ERR_VALUE(as->usbm)) {
 ^

vim +/unlikely +21 include/linux/err.h

a5ed3cee Joe Perches2014-04-03   5  #include 
^1da177e Linus Torvalds 2005-04-16   6  
^1da177e Linus Torvalds 2005-04-16   7  #include 
^1da177e Linus Torvalds 2005-04-16   8  
^1da177e Linus Torvalds 2005-04-16   9  /*
^1da177e Linus Torvalds 2005-04-16  10   * Kernel pointers have redundant 
information, so we can use a
a5ed3cee Joe Perches2014-04-03  11   * scheme where we can return either an 
error code or a normal
^1da177e Linus Torvalds 2005-04-16  12   * pointer with the same return value.
^1da177e Linus Torvalds 2005-04-16  13   *
^1da177e Linus Torvalds 2005-04-16  14   * This should be a per-architecture 
thing, to allow different
^1da177e Linus Torvalds 2005-04-16  15   * error and pointer decisions.
^1da177e Linus Torvalds 2005-04-16  16   */
fa79837d Ralf Baechle   2006-07-01  17  #define MAX_ERRNO   4095
fa79837d Ralf Baechle   2006-07-01  18  
ebba5f9f Randy Dunlap   2006-09-27  19  #ifndef __ASSEMBLY__
ebba5f9f Randy Dunlap   2006-09-27  20  
fa79837d Ralf Baechle   2006-07-01 @21  #define IS_ERR_VALUE(x) unlikely((x) >= 
(unsigned long)-MAX_ERRNO)
07ab67c8 Linus Torvalds 2005-05-19  22  
e47103b1 Jani Nikula2010-05-24  23  static inline void * __must_check 
ERR_PTR(long error)
^1da177e Linus Torvalds 2005-04-16  24  {
^1da177e Linus Torvalds 2005-04-16  25  return (void *) error;
^1da177e Linus Torvalds 2005-04-16  26  }
^1da177e Linus Torvalds 2005-04-16  27  
e7152b97 Dan Carpenter  2013-07-03  28  static inline long __must_check 
PTR_ERR(__force const void *ptr)
^1da177e Linus Torvalds 2005-04-16  29  {

:: The code at line 21 was first introduced by commit
:: fa79837d5b562766a3e3cfad4753a3df8e0a1319 [PATCH] Fix IS_ERR Threshold 
Value

:: TO: Ralf Baechle 
:: CC: Linus Torvalds 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH] Add support for usbfs zerocopy.

[kbuild test robot]

Hi Steinar,

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on v4.4-rc3 next-20151203]

url:
https://github.com/0day-ci/linux/commits/Steinar-H-Gunderson/Add-support-for-usbfs-zerocopy/20151206-034258
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: tile-allyesconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile 

All warnings (new ones prefixed by >>):

   drivers/usb/core/devio.c: In function 'proc_do_submiturb':
>> drivers/usb/core/devio.c:1617:6: warning: comparison between pointer and 
>> integer [enabled by default]

vim +1617 drivers/usb/core/devio.c

  1601  ret = -EFAULT;
  1602  goto error;
  1603  }
  1604  as = alloc_async(number_of_packets);
  1605  if (!as) {
  1606  ret = -ENOMEM;
  1607  goto error;
  1608  }
  1609  
  1610  u += sizeof(struct async) + sizeof(struct urb) + 
uurb->buffer_length +
  1611   num_sgs * sizeof(struct scatterlist);
  1612  ret = usbfs_increase_memory_usage(u);
  1613  if (ret)
  1614  goto error;
  1615  as->mem_usage = u;
  1616  as->usbm = find_memory_area(ps, uurb);
> 1617  if (IS_ERR_VALUE(as->usbm)) {
  1618  ret = PTR_ERR(as->usbm);
  1619  goto error;
  1620  }
  1621  
  1622  if (num_sgs) {
  1623  as->urb->sg = kmalloc(num_sgs * sizeof(struct 
scatterlist),
  1624  GFP_KERNEL);
  1625  if (!as->urb->sg) {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data