linux-kernel - Re: [PATCH v5 1/1] usb: gadget: add raw-gadget interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a7637ise.fsf@kernel.org>
Date:   Fri, 31 Jan 2020 17:22:09 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Alan Stern <stern@...land.harvard.edu>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Marco Elver <elver@...gle.com>
Subject: Re: [PATCH v5 1/1] usb: gadget: add raw-gadget interface


Hi,

Andrey Konovalov <andreyknvl@...gle.com> writes:
>> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
>> > new file mode 100644
>> > index 000000000000..51796af48069
>> > --- /dev/null
>> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
>> > @@ -0,0 +1,1068 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>>
>> V2 only
>
> Like this: SPDX-License-Identifier: GPL-2.0 only ?

Right, you need to choose if you want 2.0-only or 2.0-or-later and make
sure spdx and module_license() agree.

https://spdx.org/licenses/GPL-2.0-only.html

What you had before, implies GPL-2.0-only...

>> > +MODULE_LICENSE("GPL");

but this is GPL 2+

/me goes look

Actually Thomas Gleixner changed the meaning of MODULE_LICENSE("GPL"),
so I don't really know how this should look today.

>> > +static int raw_event_queue_add(struct raw_event_queue *queue,
>> > +     enum usb_raw_event_type type, size_t length, const void *data)
>> > +{
>> > +     unsigned long flags;
>> > +     struct usb_raw_event *event;
>> > +
>> > +     spin_lock_irqsave(&queue->lock, flags);
>> > +     if (WARN_ON(queue->size >= RAW_EVENT_QUEUE_SIZE)) {
>> > +             spin_unlock_irqrestore(&queue->lock, flags);
>> > +             return -ENOMEM;
>> > +     }
>> > +     event = kmalloc(sizeof(*event) + length, GFP_ATOMIC);
>>
>> I would very much prefer dropping GFP_ATOMIC here. Must you have this
>> allocation under a spinlock?
>
> The issue here is not the spinlock, but that this might be called in
> interrupt context. The number of atomic allocations here is restricted
> by 128, and we can reduce the limit even further (until some point in
> the future when and if we'll report more different events). Another
> option would be to preallocate the required number of objects
> (although we don't know the required size in advance, so we'll waste
> some memory) and use those. What would you prefer?

I think you shouldn't do either :-) Here's what I think you should do:

1. support O_NONBLOCK. This just means conditionally removing your
   wait_for_completion_interruptible().

2. Every time user calls write(), you usb_ep_alloc(), allocate a buffer
   with the write size, copy buffer to kernel space,
   usb_ep_queue(). When complete() callback is called, then you free the
   request. This would allow us to amortize the cost of copy_from_user()
   with several requests being queued to USB controller.

3. Have a pre-allocated list of requests (128?) for read(). Enqueue them
   all during set_alt(). When user calls read() you will:

   a) check if there are completed requests to be copied over to
      userspace. Recycle the request.

   b) if there are no completed requests, then it depends on O_NONBLOCK

      i) If O_NONBLOCK, return -EWOULDBLOCK
      ii) otherwise, wait_for_completion

I think this can all be done without any GFP_ATOMIC allocations.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)