On Sat, 22 Oct 2022, Greg Kroah-Hartman wrote:
> On Mon, Oct 17, 2022 at 12:27:37PM +0100, Lee Jones wrote:
> > A pointer to the main HID gadget struct (*f_hidg) is shared with the
> > character device handing (read() and write(), etc support) on open().
> > However external references are presently not tracked. This could
> > easily lead to some unsavoury behaviour if gadget support is disabled
> > / disconnected. Keeping track of the refcount ensures that resources
> > are only freed *after* they are no longer in use.
> >
> > Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx
> > Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> > ---
> > drivers/usb/gadget/function/f_hid.c | 37 +++++++++++++++++++++--------
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> > index ca0a7d9eaa34e..79d4ee8a5647f 100644
> > --- a/drivers/usb/gadget/function/f_hid.c
> > +++ b/drivers/usb/gadget/function/f_hid.c
> > @@ -9,6 +9,7 @@
> > #include <linux/module.h>
> > #include <linux/hid.h>
> > #include <linux/idr.h>
> > +#include <linux/kref.h>
> > #include <linux/cdev.h>
> > #include <linux/mutex.h>
> > #include <linux/poll.h>
> > @@ -77,6 +78,8 @@ struct f_hidg {
> >
> > struct usb_ep *in_ep;
> > struct usb_ep *out_ep;
> > +
> > + struct kref refcount;
> > };
>
> While at first glance, it seems that f_hidg is not reference counted, it
> really is, with the embedded "struct cdev" a few lines above this.
>
> That is the reference count that should control the lifecycle of this
> object, not another reference here in the "outer layer" structure.
>
> But, the cdev api is tricky and messy and not really set up to control
> the lifecycle of objects it is embedded in. There have been attempts in
> the past to handle this with things like the cdev_device_del() function,
> but that's not going to work here for you as you don't have a real
> struct device in f_hidg (heck, there's no device pointer in there, which
> is a different issue...)
>
> But, you can just rip the cdev out, and make it a pointer to a cdev, and
> then you will have a better chance at getting the reference counting
> correct here. Yes, that will be 3 different reference counted objects
> all interacting at once, but hopefully it's a bit more sane. Try
> cleaning things up that way and allocate the cdev with a call to
> cdev_alloc() right before the cdev_init() call in this file, and then it
> might work better.
I'll look into it, thanks.
> Yeah, the cdev api is really messy, it's been on my todo list for 20+
> years now to clean it up :(
>
> Also, one other thing, semi-related to this change:
>
> > +static void hidg_free_resources(struct kref *ref)
> > +{
> > + struct f_hidg *hidg = container_of(ref, struct f_hidg, refcount);
> > + struct f_hid_opts *opts = container_of(hidg->func.fi, struct f_hid_opts, func_inst);
> > +
> > + mutex_lock(&opts->lock);
> > + kfree(hidg->report_desc);
> > + kfree(hidg->set_report_buf);
> > + kfree(hidg);
> > + --opts->refcnt;
>
> That's not a real reference count :(
>
> Moving that to a kref would also be good. Or it might just be able to
> be dropped entirely, I don't really understand what it's attempting to
> reference count at all here.
>From my, perhaps limited, understanding, refcnt is not being used as a
reference counter. Instead it's being used as a sort of mutual
exclusion replacement? The Ops check refcnt before doing anything
useful and if it's found to be >0, they return -EBUSY.
--
Lee Jones [李琼斯]
