Date:   Mon, 3 Feb 2020 19:08:41 +0100
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Felipe Balbi <balbi@...nel.org>
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

On Fri, Jan 31, 2020 at 4:22 PM Felipe Balbi <balbi@...nel.org> wrote:
>
>
> 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().

I don't think non blocking read/writes will work for us. We do
coverage-guided fuzzing and need to collect coverage for each syscall.
In the USB case "syscall" means processing a USB request from start to
end, and thus we need to wait until the kernel finishes processing it,
otherwise we'll fail to associate coverage properly (It's actually a
bit more complex, as we collect coverage for the whole initial
enumeration process as for one "syscall", but the general idea stands,
that we need to wait until the operation finishes.)

>
> 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.

I'm not sure I really get this part. We'll still need to call
copy_from_user() and usb_ep_queue() once per each operation/request.
How does it get amortized? Or do you mean that having multiple
requests queued will allow USB controller to process them in bulk?
This makes sense, but again, we"ll then have an issue with coverage
association.

>
> 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

See response to #1, if we prequeue requests, then the kernel will
start handling them before we do read(), and we'll fail to associate
coverage properly. (This will also require adding another ioctl to
imitate set_alt(), like the USB_RAW_IOCTL_CONFIGURE that we have.)

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

Overall, supporting O_NONBLOCK might be a useful feature for people
who are doing something else other than fuzzing, We can account for
potential future extensions that'll support it, so detecting
O_NONBLOCK and returning an error for now makes sense.

WDYT?

Thanks!

Powered by blists - more mailing lists