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

[Felipe Balbi]

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.

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

Attachment: signature.asc
Description: PGP signature