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

[Andrey Konovalov]

On Sat, Jan 11, 2020 at 9:02 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 18, 2019 at 08:26:57PM +0100, Andrey Konovalov wrote:
> > USB Raw Gadget is a kernel module that provides a userspace interface for
> > the USB Gadget subsystem. Essentially it allows to emulate USB devices
> > from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is
> > currently a strictly debugging feature and shouldn't be used in
> > production.
> >
> > Raw Gadget is similar to GadgetFS, but provides a more low-level and
> > direct access to the USB Gadget layer for the userspace. The key
> > differences are:
> >
> > 1. Every USB request is passed to the userspace to get a response, while
> >    GadgetFS responds to some USB requests internally based on the provided
> >    descriptors. However note, that the UDC driver might respond to some
> >    requests on its own and never forward them to the Gadget layer.
> >
> > 2. GadgetFS performs some sanity checks on the provided USB descriptors,
> >    while Raw Gadget allows you to provide arbitrary data as responses to
> >    USB requests.
> >
> > 3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> >    while GadgetFS currently binds to the first available UDC.
> >
> > 4. Raw Gadget uses predictable endpoint names (handles) across different
> >    UDCs (as long as UDCs have enough endpoints of each required transfer
> >    type).
> >
> > 5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
>
> This looks good to me, with the one minor problem below that you can fix
> with a follow-on patch.

Great, thanks! I'd prefer to send out v5 to keep this a single patch
if that's OK.

I've also found an issue, but I'm not sure if that is the bug in Raw
Gadget, or in the gadget layer (in the former case I'll add this fix
to v5 as well). What I believe I'm seeing is
__fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind()
racing with dummy_timer()->gadget_setup(). In my case it results in
gadget_unbind() doing set_gadget_data(gadget, NULL), and then
gadget_setup() dereferencing get_gadget_data(gadget).

Alan, does it look possible for those two functions to race? Should
this be prevented by the gadget layer, or should I use some kind of
locking in my gadget driver to prevent this?

> This should probably go through Felipe's tree
> though, so I'll wait for him to review the gadget side and then queue it
> up in his tree:

OK, looking forward to Felipe's review.

> > +/* The type of event fetched with the USB_RAW_IOCTL_EVENT_FETCH ioctl. */
> > +enum usb_raw_event_type {
> > +     USB_RAW_EVENT_INVALID,
> > +
> > +     /* This event is queued when the driver has bound to a UDC. */
> > +     USB_RAW_EVENT_CONNECT,
> > +
> > +     /* This event is queued when a new control request arrived to ep0. */
> > +     USB_RAW_EVENT_CONTROL,
> > +
> > +     /* The list might grow in the future. */
> > +};
>
> You have to manually specify the enum values in the .h file for all
> entries in order to assure that both userspace and the kernel will be in
> sync with the same values.  I think that's documented in the "how to
> write an ioctl interface" document that is somewhere in
> Documentation/...

Will fix in v5, thanks!