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

[Greg Kroah-Hartman]

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.

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.

thanks,

greg k-h