Re: [PATCH] Add support for usbfs zerocopy.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 5, 2016 at 4:57 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 5 Jan 2016, David Laight wrote:
>
>> From: Steinar H. Gunderson [mailto:sesse@xxxxxxxxxx]
>> > 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(&ps->lock, flags);
>> > +   --*count;
>> > +   if (usbm->urb_use_count == 0 && usbm->vma_use_count == 0) {
>> ...
>> > +           module_put(THIS_MODULE);
>> > +   } else {
>> > +           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, &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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux