Message-ID: <87tv4556ke.fsf@kernel.org>
Date:   Wed, 05 Feb 2020 18:42:41 +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:
>> >> > +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 can
still support both block and nonblock, but let's at least give the
option.

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

yes :-)

> This makes sense, but again, we"ll then have an issue with coverage
> association.

You can still enqueue one by one, but this would turn your raw-gadget
interface more interesting for other use cases.

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

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

cheers

-- 
balbi

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

Powered by blists - more mailing lists