Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer

[Lee Jones]

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 [李琼斯]