usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer

PATCH In review Lee Jones 2 participants 3 messages Oct 17, 2022 – Oct 24, 2022 USB

Share & tools

Lee Jones

author

2022-10-17 11:28 UTC #0 lore raw

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

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 <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

Signed-off-by Lee Jones <[email protected]>

---
 drivers/usb/gadget/function/f_hid.c | 37 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)

drivers/usb/gadget/function/f_hid.c +27 ~~-10~~

index ca0a7d9eaa34e..79d4ee8a5647f 100644
@@ -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;
 };
 
 static inline struct f_hidg *func_to_hidg(struct usb_function *f)

@@ -273,6 +276,19 @@ static struct usb_gadget_strings *ct_func_strings[] = {
 	NULL,
 };
 
+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;
+	mutex_unlock(&opts->lock);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                              Char Device                                */

@@ -539,7 +555,13 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
+	struct f_hidg *hidg =
+		container_of(inode->i_cdev, struct f_hidg, cdev);
+
 	fd->private_data = NULL;
+
+	kref_put(&hidg->refcount, hidg_free_resources);
+
 	return 0;
 }

@@ -548,6 +570,8 @@ static int f_hidg_open(struct inode *inode, struct file *fd)
 	struct f_hidg *hidg =
 		container_of(inode->i_cdev, struct f_hidg, cdev);
 
+	kref_get(&hidg->refcount);
+
 	fd->private_data = hidg;
 
 	return 0;

@@ -1239,17 +1263,9 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 
 static void hidg_free(struct usb_function *f)
 {
-	struct f_hidg *hidg;
-	struct f_hid_opts *opts;
+	struct f_hidg *hidg = func_to_hidg(f);
 
-	hidg = func_to_hidg(f);
-	opts = container_of(f->fi, struct f_hid_opts, func_inst);
-	kfree(hidg->report_desc);
-	kfree(hidg->set_report_buf);
-	kfree(hidg);
-	mutex_lock(&opts->lock);
-	--opts->refcnt;
-	mutex_unlock(&opts->lock);
+	kref_put(&hidg->refcount, hidg_free_resources);
 }
 
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)

@@ -1271,6 +1287,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
 	if (!hidg)
 		return ERR_PTR(-ENOMEM);
+	kref_init(&hidg->refcount);
 
 	opts = container_of(fi, struct f_hid_opts, func_inst);

Signature · 1 line

2.38.0.413.g74048e4d9e-goog

Greg Kroah-Hartman

maintainer

2022-10-22 12:53 UTC #1 lore raw

Quoted 28 lines · Lee Jones

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 <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Lee Jones <[email protected]>
---
 drivers/usb/gadget/function/f_hid.c | 37 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)
drivers/usb/gadget/function/f_hid.c +1 -0
index ca0a7d9eaa34e..79d4ee8a5647f 100644
@@ -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>

drivers/usb/gadget/function/f_hid.c struct f_hidg { · line 83

Quoted 6 lines

drivers/usb/gadget/function/f_hid.c +2 -0

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

drivers/usb/gadget/function/f_hid.c static struct usb_gadget_strings *ct_func_strings[] = { · line 288

Quoted 10 lines

drivers/usb/gadget/function/f_hid.c +10 -0

+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

Lee Jones

author

2022-10-24 07:17 UTC #2 lore raw

Quoted 49 lines · Greg Kroah-Hartman

Quoted 28 lines · Lee Jones
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 <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc [email protected]
Signed-off-by Lee Jones <[email protected]>
---
 drivers/usb/gadget/function/f_hid.c | 37 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)
drivers/usb/gadget/function/f_hid.c +1 -0
index ca0a7d9eaa34e..79d4ee8a5647f 100644
@@ -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>
drivers/usb/gadget/function/f_hid.c struct f_hidg { · line 83
Quoted 6 lines
drivers/usb/gadget/function/f_hid.c +2 -0
@@ -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...)

Quoted 7 lines

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.

Quoted 18 lines

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:
drivers/usb/gadget/function/f_hid.c static struct usb_gadget_strings *ct_func_strings[] = { · line 288
Quoted 10 lines
drivers/usb/gadget/function/f_hid.c +10 -0
+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 :(

Quoted 3 lines

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.

Signature · 1 line

Lee Jones [李琼斯]