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

[Andrey Konovalov]

On Thu, Feb 6, 2020 at 7:19 AM Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
>
> Hi,
>
> Andrey Konovalov <andreyknvl@xxxxxxxxxx> writes:
> >> Andrey Konovalov <andreyknvl@xxxxxxxxxx> writes:
> >> >> >> > +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.)
> >>
> >> Fair enough, but if the only use case that this covers, is a testing
> >> scenario, we don't gain much from accepting this upstream, right?
> >
> > We gain a lot, even though it's just for testing. For one thing, once
> > the patch is upstream, all syzbot instances that target upstream-ish
> > branches will start fuzzing USB, and there won't be any need for me to
> > maintain a dedicated USB fuzzing branch manually. Another thing, is
> > that syzbot will be able to do fix/cause bisection (at least for the
> > bugs that are fixed/introduced after this patch is merged). And
> > finally, once this is upstream, we'll be able to backport this to
> > Android kernels and start testing them as well.
>
> A very respectable goal :-)
>
> I just want to take the opportunity to turn this into something more
> generic so we stop depending on kernel patches to support newer USB
> classes.
>
> I'll try to allocate some time during next week (this week, I'm totally
> swamped) to carefully review your submission.

OK, looking forward to it, thank you!

In case you'll find it helpful for view, here's a userspace
implementation of a USB keyboard via Raw Gadget:

https://github.com/xairy/raw-gadget/blob/master/examples/keyboard.c

>
> >> >> 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.)
> >>
> >> set_alt() needs to be supported if we're aiming at providing support for
> >> various USB classes to be implemented on top of what you created :-)
> >
> > What do you mean by supporting set_alt() here? AFAIU set_alt() is a
> > part of the composite gadget framework, which I don't use for this.
> > Are there some other actions (besides sending/receiving requests) that
> > need to be exposed to userspace to implement various USB classes? The
> > one that I know about is halting endpoints, it's mentioned in the TODO
> > section in documentation.
>
> Yeah, halting endpoints, cancelling all pending requests, tell userspace
> about it, and so on.
>
> >> >> 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?
> >>
> >> If that's the way you want to go, that's okay. But let's, then, prepare
> >> the code for extension later on. For example, let's add an IOCTL which
> >> returns the "version" of the ABI. Based on that, userspace can detect
> >> features and so on.
> >
> > This sounds good to me. Let's concentrate on implementing the part
> > that is essential for testing/fuzzing, as it was the initial reason
> > why I started working on this, instead of using e.g. GadgetFS. I'll
> > add such IOCTL in v6.
>
> Greg doesn't want it, so let's stop that for now.
>
> > Re GFP_ATOMIC allocations, if we're using the blocking approach,
> > should I decrease the limit of the number of such allocations or do
> > something else?
>
> I would prefer to not see GFP_ATOMIC at all here and I think it's
> totally doable, but I could be wrong.
>
> --
> balbi