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

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

[Lee Jones]

A reference to struct f_hidg is shared with this driver's associated
character device handling component without provision for life-time
handling.  In some circumstances, this can lead to unwanted
behaviour depending on the order in which things are torn down.

Utilise, the reference counting functionality already provided by the
implanted character device structure to ensure the struct f_hidg's
memory is only freed once the character device handling has finished
with it.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34e..5da8f44d47d9d 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -77,6 +77,8 @@ struct f_hidg {
 
 	struct usb_ep			*in_ep;
 	struct usb_ep			*out_ep;
+
+	bool				gc;
 };
 
 static inline struct f_hidg *func_to_hidg(struct usb_function *f)
@@ -84,6 +86,11 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 
+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -273,6 +280,18 @@ static struct usb_gadget_strings *ct_func_strings[] = {
 	NULL,
 };
 
+static void hidg_free_resources(struct f_hidg *hidg)
+{
+       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 +558,16 @@ 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  = fd->private_data;
+
+	if (hidg->gc) {
+		/* Gadget has already been disconnected and we are the last f_hidg user */
+		cdev_del(&hidg->cdev);
+		hidg_free_resources(hidg);
+	}
+
 	fd->private_data = NULL;
+
 	return 0;
 }
 
@@ -1239,17 +1267,16 @@ 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);
+	if (f_hidg_is_open(hidg))
+		/*
+		 * Gadget disconnected whilst an open dev node exists.
+		 * Delay freeing resources until it closes.
+		 */
+		hidg->gc = true;
+	else
+		hidg_free_resources(hidg);
 }
 
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
-- 
2.38.1.431.g37b22c650d-goog

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

[Greg KH]

On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> A reference to struct f_hidg is shared with this driver's associated
> character device handling component without provision for life-time
> handling.  In some circumstances, this can lead to unwanted
> behaviour depending on the order in which things are torn down.
> 
> Utilise, the reference counting functionality already provided by the
> implanted character device structure to ensure the struct f_hidg's
> memory is only freed once the character device handling has finished
> with it.
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 10 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Greg KH]

On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> +{
> +	return !!kref_read(&hidg->cdev.kobj.kref);
> +}

Ick, sorry, no, that's not going to work and is not allowed at all.
That's some major layering violations there, AND it can change after you
get the value as well.

greg k-h

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

[Lee Jones]

On Thu, 17 Nov 2022, Greg KH wrote:

> On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > A reference to struct f_hidg is shared with this driver's associated
> > character device handling component without provision for life-time
> > handling.  In some circumstances, this can lead to unwanted
> > behaviour depending on the order in which things are torn down.
> > 
> > Utilise, the reference counting functionality already provided by the
> > implanted character device structure to ensure the struct f_hidg's
> > memory is only freed once the character device handling has finished
> > with it.
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
> >  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/SubmittingPatches for what needs to be done
>   here to properly describe this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 
> thanks,
> 
> greg k-h's patch email bot

This is a completely new solution to the same problem.

I'm treating this as a brand new submission.

-- 
Lee Jones [李琼斯]

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

[Lee Jones]

On Thu, 17 Nov 2022, Greg KH wrote:

> On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > +{
> > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > +}
> 
> Ick, sorry, no, that's not going to work and is not allowed at all.
> That's some major layering violations there, AND it can change after you
> get the value as well.

This cdev belongs solely to this driver.  Hence the *.*.* and not
*->*->*.  What is preventing us from reading our own data?  If we
cannot do this directly, can I create an API to do it 'officially'?

I do, however, appreciate that a little locking wouldn't go amiss.

If this solution is not acceptable either, then we're left up the
creak without a paddle.  The rules you've communicated are not
compatible with each other.

Rule 1: Only one item in a data structure can reference count.

Due to the embedded cdev struct, this rules out my first solution of
giving f_hidg its own kref so that it can conduct its own life-time
management.

A potential option to satisfy this rule would be to remove the cdev
attribute and create its data dynamically instead.  However, the
staticness of cdev is used to obtain f_hidg (with container_of()) in
the character device handling component, so it cannot be removed.

Rule 2: Reading the present refcount causes a laying violation

So we're essentially saying, if data is already being reference
counted, you have to use the present implementation instead of adding
additional counts, but there is no way to actually do that, which
kind of puts us at stalemate.

Since this is a genuine issue, doing anything is not really an option
either.  So where do we go from here?

-- 
Lee Jones [李琼斯]

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

[Alan Stern]

On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> On Thu, 17 Nov 2022, Greg KH wrote:
> 
> > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > +{
> > > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > > +}
> > 
> > Ick, sorry, no, that's not going to work and is not allowed at all.
> > That's some major layering violations there, AND it can change after you
> > get the value as well.
> 
> This cdev belongs solely to this driver.  Hence the *.*.* and not
> *->*->*.  What is preventing us from reading our own data?  If we
> cannot do this directly, can I create an API to do it 'officially'?
> 
> I do, however, appreciate that a little locking wouldn't go amiss.
> 
> If this solution is not acceptable either, then we're left up the
> creak without a paddle.  The rules you've communicated are not
> compatible with each other.
> 
> Rule 1: Only one item in a data structure can reference count.
> 
> Due to the embedded cdev struct, this rules out my first solution of
> giving f_hidg its own kref so that it can conduct its own life-time
> management.
> 
> A potential option to satisfy this rule would be to remove the cdev
> attribute and create its data dynamically instead.  However, the
> staticness of cdev is used to obtain f_hidg (with container_of()) in
> the character device handling component, so it cannot be removed.

You have not understood this rule correctly.  Only one item in a data 
structure can hold a reference count _for that structure_.  But several 
items in a structure can hold reference counts for themselves.

So for example, you could put a kref in f_hidg which would hold the 
reference count for the f_hidg structure, while at the same time 
including an embedded cdev with its own reference counter.  The point is 
that the refcount in the embedded cdev refers to the lifetime of the 
cdev, not the lifetime of the f_hidg.

To make this work properly, you have to do two additional things:

	When the cdev's refcount is initialized, increment the kref
	in f_hidg.

	When the cdev's refcount drops to 0, decrement the kref (and
	release f_hidg if the kref hits 0).

Alan Stern

> Rule 2: Reading the present refcount causes a laying violation
> 
> So we're essentially saying, if data is already being reference
> counted, you have to use the present implementation instead of adding
> additional counts, but there is no way to actually do that, which
> kind of puts us at stalemate.
> 
> Since this is a genuine issue, doing anything is not really an option
> either.  So where do we go from here?
> 
> -- 
> Lee Jones [李琼斯]

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

[Greg KH]

On Thu, Nov 17, 2022 at 01:26:21PM +0000, Lee Jones wrote:
> On Thu, 17 Nov 2022, Greg KH wrote:
> 
> > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > A reference to struct f_hidg is shared with this driver's associated
> > > character device handling component without provision for life-time
> > > handling.  In some circumstances, this can lead to unwanted
> > > behaviour depending on the order in which things are torn down.
> > > 
> > > Utilise, the reference counting functionality already provided by the
> > > implanted character device structure to ensure the struct f_hidg's
> > > memory is only freed once the character device handling has finished
> > > with it.
> > > 
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > >  drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
> > >  1 file changed, 37 insertions(+), 10 deletions(-)
> > 
> > Hi,
> > 
> > This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> > a patch that has triggered this response.  He used to manually respond
> > to these common problems, but in order to save his sanity (he kept
> > writing the same thing over and over, yet to different people), I was
> > created.  Hopefully you will not take offence and will fix the problem
> > in your patch and resubmit it so that it can be accepted into the Linux
> > kernel tree.
> > 
> > You are receiving this message because of the following common error(s)
> > as indicated below:
> > 
> > - This looks like a new version of a previously submitted patch, but you
> >   did not list below the --- line any changes from the previous version.
> >   Please read the section entitled "The canonical patch format" in the
> >   kernel file, Documentation/SubmittingPatches for what needs to be done
> >   here to properly describe this.
> > 
> > If you wish to discuss this problem further, or you have questions about
> > how to resolve this issue, please feel free to respond to this email and
> > Greg will reply once he has dug out from the pending patches received
> > from other developers.
> > 
> > thanks,
> > 
> > greg k-h's patch email bot
> 
> This is a completely new solution to the same problem.
> 
> I'm treating this as a brand new submission.

With the identical subject line, which plays havoc with tools :(

This is a v2, next should be v3 please.

thanks,

greg k-h

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

[Lee Jones]

On Thu, 17 Nov 2022, Alan Stern wrote:

> On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> > On Thu, 17 Nov 2022, Greg KH wrote:
> > 
> > > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > > +{
> > > > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > > > +}
> > > 
> > > Ick, sorry, no, that's not going to work and is not allowed at all.
> > > That's some major layering violations there, AND it can change after you
> > > get the value as well.
> > 
> > This cdev belongs solely to this driver.  Hence the *.*.* and not
> > *->*->*.  What is preventing us from reading our own data?  If we
> > cannot do this directly, can I create an API to do it 'officially'?
> > 
> > I do, however, appreciate that a little locking wouldn't go amiss.
> > 
> > If this solution is not acceptable either, then we're left up the
> > creak without a paddle.  The rules you've communicated are not
> > compatible with each other.
> > 
> > Rule 1: Only one item in a data structure can reference count.
> > 
> > Due to the embedded cdev struct, this rules out my first solution of
> > giving f_hidg its own kref so that it can conduct its own life-time
> > management.
> > 
> > A potential option to satisfy this rule would be to remove the cdev
> > attribute and create its data dynamically instead.  However, the
> > staticness of cdev is used to obtain f_hidg (with container_of()) in
> > the character device handling component, so it cannot be removed.
> 
> You have not understood this rule correctly.  Only one item in a data 
> structure can hold a reference count _for that structure_.  But several 
> items in a structure can hold reference counts for themselves.

Here was the review comment I was working to on this patch [0]:

 "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."

> So for example, you could put a kref in f_hidg which would hold the 
> reference count for the f_hidg structure, while at the same time 
> including an embedded cdev with its own reference counter.  The point is 
> that the refcount in the embedded cdev refers to the lifetime of the 
> cdev, not the lifetime of the f_hidg.

This was the approach in the original submission [1], which during
review I was told was unacceptable for the aforementioned reason.

[0] https://lore.kernel.org/all/Y1PnoMvDmZMqXScw@kroah.com/
[1] https://lore.kernel.org/all/20221017112737.230772-1-lee@kernel.org/

> To make this work properly, you have to do two additional things:
> 
> 	When the cdev's refcount is initialized, increment the kref
> 	in f_hidg.
> 
> 	When the cdev's refcount drops to 0, decrement the kref (and
> 	release f_hidg if the kref hits 0).

More than happy to revisit the first solution with Greg's blessing.

-- 
Lee Jones [李琼斯]

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

[Alan Stern]

On Fri, Nov 18, 2022 at 08:54:53AM +0000, Lee Jones wrote:
> On Thu, 17 Nov 2022, Alan Stern wrote:
> 
> > On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> > > On Thu, 17 Nov 2022, Greg KH wrote:
> > > 
> > > > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > > > +{
> > > > > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > > > > +}
> > > > 
> > > > Ick, sorry, no, that's not going to work and is not allowed at all.
> > > > That's some major layering violations there, AND it can change after you
> > > > get the value as well.
> > > 
> > > This cdev belongs solely to this driver.  Hence the *.*.* and not
> > > *->*->*.  What is preventing us from reading our own data?  If we
> > > cannot do this directly, can I create an API to do it 'officially'?
> > > 
> > > I do, however, appreciate that a little locking wouldn't go amiss.
> > > 
> > > If this solution is not acceptable either, then we're left up the
> > > creak without a paddle.  The rules you've communicated are not
> > > compatible with each other.
> > > 
> > > Rule 1: Only one item in a data structure can reference count.
> > > 
> > > Due to the embedded cdev struct, this rules out my first solution of
> > > giving f_hidg its own kref so that it can conduct its own life-time
> > > management.
> > > 
> > > A potential option to satisfy this rule would be to remove the cdev
> > > attribute and create its data dynamically instead.  However, the
> > > staticness of cdev is used to obtain f_hidg (with container_of()) in
> > > the character device handling component, so it cannot be removed.
> > 
> > You have not understood this rule correctly.  Only one item in a data 
> > structure can hold a reference count _for that structure_.  But several 
> > items in a structure can hold reference counts for themselves.
> 
> Here was the review comment I was working to on this patch [0]:
> 
>  "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."

It's worth noting that the review comment goes on to say:

 "But, the cdev api is tricky and messy and not really set up to control
  the lifecycle of objects it is embedded in."

This is a good indication that a separate reference counter really is 
needed (in fact it almost contradicts what was written above).

> > So for example, you could put a kref in f_hidg which would hold the 
> > reference count for the f_hidg structure, while at the same time 
> > including an embedded cdev with its own reference counter.  The point is 
> > that the refcount in the embedded cdev refers to the lifetime of the 
> > cdev, not the lifetime of the f_hidg.
> 
> This was the approach in the original submission [1], which during
> review I was told was unacceptable for the aforementioned reason.
> 
> [0] https://lore.kernel.org/all/Y1PnoMvDmZMqXScw@kroah.com/
> [1] https://lore.kernel.org/all/20221017112737.230772-1-lee@kernel.org/
> 
> > To make this work properly, you have to do two additional things:
> > 
> > 	When the cdev's refcount is initialized, increment the kref
> > 	in f_hidg.
> > 
> > 	When the cdev's refcount drops to 0, decrement the kref (and
> > 	release f_hidg if the kref hits 0).
> 
> More than happy to revisit the first solution with Greg's blessing.

Okay, let's see what Greg thinks after he reads this discussion.

Alan Stern

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

[John Keeping]

On Fri, Nov 18, 2022 at 10:59:42AM -0500, Alan Stern wrote:
> On Fri, Nov 18, 2022 at 08:54:53AM +0000, Lee Jones wrote:
> > On Thu, 17 Nov 2022, Alan Stern wrote:
> > 
> > > On Thu, Nov 17, 2022 at 01:46:26PM +0000, Lee Jones wrote:
> > > > On Thu, 17 Nov 2022, Greg KH wrote:
> > > > 
> > > > > On Thu, Nov 17, 2022 at 12:08:13PM +0000, Lee Jones wrote:
> > > > > > +static inline bool f_hidg_is_open(struct f_hidg *hidg)
> > > > > > +{
> > > > > > +	return !!kref_read(&hidg->cdev.kobj.kref);
> > > > > > +}
> > > > > 
> > > > > Ick, sorry, no, that's not going to work and is not allowed at all.
> > > > > That's some major layering violations there, AND it can change after you
> > > > > get the value as well.
> > > > 
> > > > This cdev belongs solely to this driver.  Hence the *.*.* and not
> > > > *->*->*.  What is preventing us from reading our own data?  If we
> > > > cannot do this directly, can I create an API to do it 'officially'?
> > > > 
> > > > I do, however, appreciate that a little locking wouldn't go amiss.
> > > > 
> > > > If this solution is not acceptable either, then we're left up the
> > > > creak without a paddle.  The rules you've communicated are not
> > > > compatible with each other.
> > > > 
> > > > Rule 1: Only one item in a data structure can reference count.
> > > > 
> > > > Due to the embedded cdev struct, this rules out my first solution of
> > > > giving f_hidg its own kref so that it can conduct its own life-time
> > > > management.
> > > > 
> > > > A potential option to satisfy this rule would be to remove the cdev
> > > > attribute and create its data dynamically instead.  However, the
> > > > staticness of cdev is used to obtain f_hidg (with container_of()) in
> > > > the character device handling component, so it cannot be removed.
> > > 
> > > You have not understood this rule correctly.  Only one item in a data 
> > > structure can hold a reference count _for that structure_.  But several 
> > > items in a structure can hold reference counts for themselves.
> > 
> > Here was the review comment I was working to on this patch [0]:
> > 
> >  "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."
> 
> It's worth noting that the review comment goes on to say:
> 
>  "But, the cdev api is tricky and messy and not really set up to control
>   the lifecycle of objects it is embedded in."
> 
> This is a good indication that a separate reference counter really is 
> needed (in fact it almost contradicts what was written above).

I don't think it's at all simple to fix this - I posted a series
addressing the lifetime issues here a few years ago but didn't chase it
up and there was no feedback:

	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/

That includes a patch to remove the embedded struct cdev and manage its
lifetime separately, which I think is needed as there are two different
struct device objects here and we cannot tie their lifetimes together.

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

[Alan Stern]

On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> I don't think it's at all simple to fix this - I posted a series
> addressing the lifetime issues here a few years ago but didn't chase it
> up and there was no feedback:
> 
> 	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/
> 
> That includes a patch to remove the embedded struct cdev and manage its
> lifetime separately, which I think is needed as there are two different
> struct device objects here and we cannot tie their lifetimes together.

I still don't have a clear picture of what the real problem is.  Lee's 
original patch description just said "external references are presently 
not tracked", with no details about what those external references are. 
Why not add just proper cdev_get() and cdev_put() calls to whatever code 
handles those external references, so that they _are_ tracked?

What are the two different struct device objects?  Why do their 
lifetimes need to be tied together?  If you do need to tie their 
lifetimes somehow, why not simply make one of them (the one which is 
logically allowed to be shorter-lived) hold a reference to the other?

Alan Stern

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

[John Keeping]

On Fri, Nov 18, 2022 at 04:07:24PM -0500, Alan Stern wrote:
> On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> > I don't think it's at all simple to fix this - I posted a series
> > addressing the lifetime issues here a few years ago but didn't chase it
> > up and there was no feedback:
> > 
> > 	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/
> > 
> > That includes a patch to remove the embedded struct cdev and manage its
> > lifetime separately, which I think is needed as there are two different
> > struct device objects here and we cannot tie their lifetimes together.
> 
> I still don't have a clear picture of what the real problem is.  Lee's 
> original patch description just said "external references are presently 
> not tracked", with no details about what those external references are. 
> Why not add just proper cdev_get() and cdev_put() calls to whatever code 
> handles those external references, so that they _are_ tracked?
> 
> What are the two different struct device objects?  Why do their 
> lifetimes need to be tied together?  If you do need to tie their 
> lifetimes somehow, why not simply make one of them (the one which is 
> logically allowed to be shorter-lived) hold a reference to the other?

The problem is that we have a struct cdev embedded in f_hidg but the
lifetime of f_hidg is not tied to any kobject so we can't solve this in
the right way by setting the parent kobject of the cdev.

While refcounting struct f_hidg is necessary, it's not sufficient
because the only way to keep it alive long enough for the final
kobject_put() on the embedded cdev is to tie the lifetime to a kobject
of its own and there is no suitable object as this is not the model
followed by gadget function instances.

To show the problem (using libusbgx's example commands for conciseness,
but obviously this can be done via configfs directly):

	$ gadget-hid
	$ exec 3<> /dev/hidg0
	$ gadget-vid-pid-remove
	$ exec 3<&-

	==================================================================
	BUG: KASAN: use-after-free in kobject_put+0x24/0x250
	Read of size 1 at addr c61784a0 by task sh/264

	CPU: 1 PID: 264 Comm: sh Tainted: G        W          6.0.5 #1
	Hardware name: Rockchip (Device Tree)
	 unwind_backtrace from show_stack+0x10/0x14
	 show_stack from dump_stack_lvl+0x58/0x70
	 dump_stack_lvl from print_report+0x58/0x4dc
	 print_report from kasan_report+0x88/0xa8
	 kasan_report from kobject_put+0x24/0x250
	 kobject_put from __fput+0x1e4/0x358
	 __fput from task_work_run+0xb4/0xc4
	 task_work_run from do_work_pending+0x4d4/0x524
	 do_work_pending from slow_work_pending+0xc/0x20
	Exception stack(0xf284bfb0 to 0xf284bff8)
	bfa0:                                     00000000 00000003 02292190 00000000
	bfc0: 02293fc8 aed4e1d0 00000001 00000006 022925a0 00000000 022925a0 022923f4
	bfe0: 00532f38 b6864840 0049c218 aec57e38 60000010 0000000b

	Allocated by task 341:
	 kasan_set_track+0x20/0x28
	 ____kasan_kmalloc+0x80/0x88
	 hidg_alloc+0x24/0x1f0
	 usb_get_function+0x28/0x48
	 config_usb_cfg_link+0x90/0x114
	 configfs_symlink+0x24c/0x5d8
	 vfs_symlink+0x58/0x74
	 do_symlinkat+0xdc/0x16c
	 ret_fast_syscall+0x0/0x1c

	Freed by task 352:
	 kasan_set_track+0x20/0x28
	 kasan_set_free_info+0x28/0x34
	 ____kasan_slab_free+0xf8/0x108
	 __kasan_slab_free+0x10/0x18
	 slab_free_freelist_hook+0x9c/0xfc
	 kfree+0x118/0x258
	 hidg_free+0x44/0x6c
	 config_usb_cfg_unlink+0xd4/0xf4
	 configfs_unlink+0x118/0x15c
	 vfs_unlink+0xd8/0x1c4
	 do_unlinkat+0x180/0x28c
	 ret_fast_syscall+0x0/0x1c

	The buggy address belongs to the object at c6178400
	 which belongs to the cache kmalloc-512 of size 512
	The buggy address is located 160 bytes inside of
	 512-byte region [c6178400, c6178600)

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

[Alan Stern]

On Sun, Nov 20, 2022 at 05:22:19PM +0000, John Keeping wrote:
> On Fri, Nov 18, 2022 at 04:07:24PM -0500, Alan Stern wrote:
> > On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> > > I don't think it's at all simple to fix this - I posted a series
> > > addressing the lifetime issues here a few years ago but didn't chase it
> > > up and there was no feedback:
> > > 
> > > 	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/
> > > 
> > > That includes a patch to remove the embedded struct cdev and manage its
> > > lifetime separately, which I think is needed as there are two different
> > > struct device objects here and we cannot tie their lifetimes together.
> > 
> > I still don't have a clear picture of what the real problem is.  Lee's 
> > original patch description just said "external references are presently 
> > not tracked", with no details about what those external references are. 
> > Why not add just proper cdev_get() and cdev_put() calls to whatever code 
> > handles those external references, so that they _are_ tracked?
> > 
> > What are the two different struct device objects?  Why do their 
> > lifetimes need to be tied together?  If you do need to tie their 
> > lifetimes somehow, why not simply make one of them (the one which is 
> > logically allowed to be shorter-lived) hold a reference to the other?
> 
> The problem is that we have a struct cdev embedded in f_hidg but the
> lifetime of f_hidg is not tied to any kobject so we can't solve this in
> the right way by setting the parent kobject of the cdev.
> 
> While refcounting struct f_hidg is necessary, it's not sufficient
> because the only way to keep it alive long enough for the final
> kobject_put() on the embedded cdev is to tie the lifetime to a kobject
> of its own and there is no suitable object as this is not the model
> followed by gadget function instances.

I see.  The solution is simple: Embed a struct device in struct f_hidg, 
and call cdev_device_add() to add the device and the cdev.  This will 
automatically make the device the parent of the cdev, so the device's 
refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
the f_hidg's lifetime to the device's, so the device's release routine 
can safely deallocate the entire f_hidg structure.

The parent of the new struct device should be set to &gadget->dev.  If 
you can't think of a better name for the device, you could simply append 
":I" to the parent's name, where I is the interface number, or even 
append ":C.I" where C is the config number (like we do on the host 
side).

Alan Stern

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

[Lee Jones]

On Sun, 20 Nov 2022, Alan Stern wrote:

> On Sun, Nov 20, 2022 at 05:22:19PM +0000, John Keeping wrote:
> > On Fri, Nov 18, 2022 at 04:07:24PM -0500, Alan Stern wrote:
> > > On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> > > > I don't think it's at all simple to fix this - I posted a series
> > > > addressing the lifetime issues here a few years ago but didn't chase it
> > > > up and there was no feedback:
> > > > 
> > > > 	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/
> > > > 
> > > > That includes a patch to remove the embedded struct cdev and manage its
> > > > lifetime separately, which I think is needed as there are two different
> > > > struct device objects here and we cannot tie their lifetimes together.
> > > 
> > > I still don't have a clear picture of what the real problem is.  Lee's 
> > > original patch description just said "external references are presently 
> > > not tracked", with no details about what those external references are. 
> > > Why not add just proper cdev_get() and cdev_put() calls to whatever code 
> > > handles those external references, so that they _are_ tracked?
> > > 
> > > What are the two different struct device objects?  Why do their 
> > > lifetimes need to be tied together?  If you do need to tie their 
> > > lifetimes somehow, why not simply make one of them (the one which is 
> > > logically allowed to be shorter-lived) hold a reference to the other?
> > 
> > The problem is that we have a struct cdev embedded in f_hidg but the
> > lifetime of f_hidg is not tied to any kobject so we can't solve this in
> > the right way by setting the parent kobject of the cdev.
> > 
> > While refcounting struct f_hidg is necessary, it's not sufficient
> > because the only way to keep it alive long enough for the final
> > kobject_put() on the embedded cdev is to tie the lifetime to a kobject
> > of its own and there is no suitable object as this is not the model
> > followed by gadget function instances.
> 
> I see.  The solution is simple: Embed a struct device in struct f_hidg, 
> and call cdev_device_add() to add the device and the cdev.  This will 
> automatically make the device the parent of the cdev, so the device's 
> refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
> the f_hidg's lifetime to the device's, so the device's release routine 
> can safely deallocate the entire f_hidg structure.
> 
> The parent of the new struct device should be set to &gadget->dev.  If 
> you can't think of a better name for the device, you could simply append 
> ":I" to the parent's name, where I is the interface number, or even 
> append ":C.I" where C is the config number (like we do on the host 
> side).

Thanks for the suggestions Alan.

Not long finished speaking with Greg about this.  He seems okay with
the approach.  I'll knock something together and get a "v1" (*wink*)
out shortly.

-- 
Lee Jones [李琼斯]

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

[John Keeping]

On Sun, Nov 20, 2022 at 03:46:26PM -0500, Alan Stern wrote:
> On Sun, Nov 20, 2022 at 05:22:19PM +0000, John Keeping wrote:
> > On Fri, Nov 18, 2022 at 04:07:24PM -0500, Alan Stern wrote:
> > > On Fri, Nov 18, 2022 at 04:37:32PM +0000, John Keeping wrote:
> > > > I don't think it's at all simple to fix this - I posted a series
> > > > addressing the lifetime issues here a few years ago but didn't chase it
> > > > up and there was no feedback:
> > > > 
> > > > 	https://lore.kernel.org/linux-usb/20191028114228.3679219-1-john@metanate.com/
> > > > 
> > > > That includes a patch to remove the embedded struct cdev and manage its
> > > > lifetime separately, which I think is needed as there are two different
> > > > struct device objects here and we cannot tie their lifetimes together.
> > > 
> > > I still don't have a clear picture of what the real problem is.  Lee's 
> > > original patch description just said "external references are presently 
> > > not tracked", with no details about what those external references are. 
> > > Why not add just proper cdev_get() and cdev_put() calls to whatever code 
> > > handles those external references, so that they _are_ tracked?
> > > 
> > > What are the two different struct device objects?  Why do their 
> > > lifetimes need to be tied together?  If you do need to tie their 
> > > lifetimes somehow, why not simply make one of them (the one which is 
> > > logically allowed to be shorter-lived) hold a reference to the other?
> > 
> > The problem is that we have a struct cdev embedded in f_hidg but the
> > lifetime of f_hidg is not tied to any kobject so we can't solve this in
> > the right way by setting the parent kobject of the cdev.
> > 
> > While refcounting struct f_hidg is necessary, it's not sufficient
> > because the only way to keep it alive long enough for the final
> > kobject_put() on the embedded cdev is to tie the lifetime to a kobject
> > of its own and there is no suitable object as this is not the model
> > followed by gadget function instances.
> 
> I see.  The solution is simple: Embed a struct device in struct f_hidg, 
> and call cdev_device_add() to add the device and the cdev.  This will 
> automatically make the device the parent of the cdev, so the device's 
> refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
> the f_hidg's lifetime to the device's, so the device's release routine 
> can safely deallocate the entire f_hidg structure.
> 
> The parent of the new struct device should be set to &gadget->dev.  If 
> you can't think of a better name for the device, you could simply append 
> ":I" to the parent's name, where I is the interface number, or even 
> append ":C.I" where C is the config number (like we do on the host 
> side).

There is no gadget->dev at the time struct f_hidg is allocated.

AFAICT the device is the UDC, which is only associated with the gadget
when it is bound.  The functions are allocated earlier than this and I
can't see any device associated with struct usb_function_instance.

The patch below does fix the issue, but I'm wondering if there's a
deeper problem here that can only be properly solved by adding some
device/kobject hierarchy to the config side of things.

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to
the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for
conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Add a device to f_hidg so that the cdev can properly take a reference to
this and the structure will be freed only when the child device is
released.

[Also fix refcount leak on an error path.]

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 35 ++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34..7ae97e5c4d20 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -71,6 +71,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	struct device			dev;
 	int				minor;
 	struct cdev			cdev;
 	struct usb_function		func;
@@ -84,6 +85,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 
+static void hidg_release(struct device *dev)
+{
+	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+
+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -999,6 +1008,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 
 	/* create char device */
 	cdev_init(&hidg->cdev, &f_hidg_fops);
+	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
 	dev = MKDEV(major, hidg->minor);
 	status = cdev_add(&hidg->cdev, dev, 1);
 	if (status)
@@ -1244,9 +1254,7 @@ static void hidg_free(struct usb_function *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);
+	device_unregister(&hidg->dev);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);
@@ -1266,6 +1274,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 {
 	struct f_hidg *hidg;
 	struct f_hid_opts *opts;
+	int ret;
 
 	/* allocate and initialize one new instance */
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
@@ -1277,17 +1286,27 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	++opts->refcnt;
 
+	device_initialize(&hidg->dev);
+	hidg->dev.release = hidg_release;
+	ret = dev_set_name(&hidg->dev, "hidg%d", hidg->minor);
+	if (ret) {
+		--opts->refcnt;
+		mutex_unlock(&opts->lock);
+		return ERR_PTR(ret);
+	}
+
 	hidg->minor = opts->minor;
 	hidg->bInterfaceSubClass = opts->subclass;
 	hidg->bInterfaceProtocol = opts->protocol;
 	hidg->report_length = opts->report_length;
 	hidg->report_desc_length = opts->report_desc_length;
 	if (opts->report_desc) {
-		hidg->report_desc = kmemdup(opts->report_desc,
+		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
 					    opts->report_desc_length,
 					    GFP_KERNEL);
 		if (!hidg->report_desc) {
-			kfree(hidg);
+			put_device(&hidg->dev);
+			--opts->refcnt;
 			mutex_unlock(&opts->lock);
 			return ERR_PTR(-ENOMEM);
 		}
@@ -1307,6 +1326,12 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	/* this could be made configurable at some point */
 	hidg->qlen	   = 4;
 
+	ret = device_add(&hidg->dev);
+	if (ret) {
+		put_device(&hidg->dev);
+		return ERR_PTR(ret);
+	}
+
 	return &hidg->func;
 }
 
-- 
2.38.1

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

[Alan Stern]

On Mon, Nov 21, 2022 at 12:38:37PM +0000, John Keeping wrote:
> On Sun, Nov 20, 2022 at 03:46:26PM -0500, Alan Stern wrote:
> > I see.  The solution is simple: Embed a struct device in struct f_hidg, 
> > and call cdev_device_add() to add the device and the cdev.  This will 
> > automatically make the device the parent of the cdev, so the device's 
> > refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
> > the f_hidg's lifetime to the device's, so the device's release routine 
> > can safely deallocate the entire f_hidg structure.
> > 
> > The parent of the new struct device should be set to &gadget->dev.  If 
> > you can't think of a better name for the device, you could simply append 
> > ":I" to the parent's name, where I is the interface number, or even 
> > append ":C.I" where C is the config number (like we do on the host 
> > side).
> 
> There is no gadget->dev at the time struct f_hidg is allocated.
> 
> AFAICT the device is the UDC, which is only associated with the gadget
> when it is bound.  The functions are allocated earlier than this and I
> can't see any device associated with struct usb_function_instance.

Ah, that's a shame.  All right, so be it.

> The patch below does fix the issue, but I'm wondering if there's a
> deeper problem here that can only be properly solved by adding some
> device/kobject hierarchy to the config side of things.

I don't believe there's any deeper problem.  If someone wants to have an 
fhidg device as a child of the gadget, I think it could be added and 
removed in the ->set_alt() and ->disable() callbacks.  Or maybe the 
->bind() and ->unbind() callbacks (I've never had to work with configfs 
so I'm not clear on the details).

> -- >8 --
> Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> 
> The embedded struct cdev does not have its lifetime correctly tied to
> the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
> is held open while the gadget is deleted.
> 
> This can readily be replicated with libusbgx's example programs (for
> conciseness - operating directly via configfs is equivalent):
> 
> 	gadget-hid
> 	exec 3<> /dev/hidg0
> 	gadget-vid-pid-remove
> 	exec 3<&-
> 
> Add a device to f_hidg so that the cdev can properly take a reference to
> this and the structure will be freed only when the child device is
> released.
> 
> [Also fix refcount leak on an error path.]
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/usb/gadget/function/f_hid.c | 35 ++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index ca0a7d9eaa34..7ae97e5c4d20 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -71,6 +71,7 @@ struct f_hidg {
>  	wait_queue_head_t		write_queue;
>  	struct usb_request		*req;
>  
> +	struct device			dev;
>  	int				minor;
>  	struct cdev			cdev;
>  	struct usb_function		func;
> @@ -84,6 +85,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
>  	return container_of(f, struct f_hidg, func);
>  }
>  
> +static void hidg_release(struct device *dev)
> +{
> +	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
> +
> +	kfree(hidg->set_report_buf);
> +	kfree(hidg);
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  /*                           Static descriptors                            */
>  
> @@ -999,6 +1008,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	/* create char device */
>  	cdev_init(&hidg->cdev, &f_hidg_fops);
> +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
>  	dev = MKDEV(major, hidg->minor);
>  	status = cdev_add(&hidg->cdev, dev, 1);

Instead of doing it by hand like this, why not call cdev_device_add()?

>  	if (status)
> @@ -1244,9 +1254,7 @@ static void hidg_free(struct usb_function *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);
> +	device_unregister(&hidg->dev);

Are you certain at this point that hidg->dev has been registered?  Even 
on all the error paths?

>  	mutex_lock(&opts->lock);
>  	--opts->refcnt;
>  	mutex_unlock(&opts->lock);
> @@ -1266,6 +1274,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  {
>  	struct f_hidg *hidg;
>  	struct f_hid_opts *opts;
> +	int ret;
>  
>  	/* allocate and initialize one new instance */
>  	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
> @@ -1277,17 +1286,27 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  	mutex_lock(&opts->lock);
>  	++opts->refcnt;
>  
> +	device_initialize(&hidg->dev);
> +	hidg->dev.release = hidg_release;
> +	ret = dev_set_name(&hidg->dev, "hidg%d", hidg->minor);
> +	if (ret) {
> +		--opts->refcnt;
> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(ret);
> +	}
> +
>  	hidg->minor = opts->minor;
>  	hidg->bInterfaceSubClass = opts->subclass;
>  	hidg->bInterfaceProtocol = opts->protocol;
>  	hidg->report_length = opts->report_length;
>  	hidg->report_desc_length = opts->report_desc_length;
>  	if (opts->report_desc) {
> -		hidg->report_desc = kmemdup(opts->report_desc,
> +		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
>  					    opts->report_desc_length,
>  					    GFP_KERNEL);
>  		if (!hidg->report_desc) {
> -			kfree(hidg);
> +			put_device(&hidg->dev);
> +			--opts->refcnt;
>  			mutex_unlock(&opts->lock);
>  			return ERR_PTR(-ENOMEM);
>  		}
> @@ -1307,6 +1326,12 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  	/* this could be made configurable at some point */
>  	hidg->qlen	   = 4;
>  
> +	ret = device_add(&hidg->dev);
> +	if (ret) {
> +		put_device(&hidg->dev);
> +		return ERR_PTR(ret);
> +	}

Why do this here instead of when the cdev is registered?  Or to put it 
another way, why not add the two of them at the same time by calling 
cdev_device_add() rather than adding them at two separate places?

Alan Stern

> +
>  	return &hidg->func;
>  }
>  
> -- 
> 2.38.1
> 

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

[John Keeping]

On Mon, Nov 21, 2022 at 11:18:34AM -0500, Alan Stern wrote:
> On Mon, Nov 21, 2022 at 12:38:37PM +0000, John Keeping wrote:
> > On Sun, Nov 20, 2022 at 03:46:26PM -0500, Alan Stern wrote:
> > > I see.  The solution is simple: Embed a struct device in struct f_hidg, 
> > > and call cdev_device_add() to add the device and the cdev.  This will 
> > > automatically make the device the parent of the cdev, so the device's 
> > > refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
> > > the f_hidg's lifetime to the device's, so the device's release routine 
> > > can safely deallocate the entire f_hidg structure.
> > > 
> > > The parent of the new struct device should be set to &gadget->dev.  If 
> > > you can't think of a better name for the device, you could simply append 
> > > ":I" to the parent's name, where I is the interface number, or even 
> > > append ":C.I" where C is the config number (like we do on the host 
> > > side).
> > 
> > There is no gadget->dev at the time struct f_hidg is allocated.
> > 
> > AFAICT the device is the UDC, which is only associated with the gadget
> > when it is bound.  The functions are allocated earlier than this and I
> > can't see any device associated with struct usb_function_instance.
> 
> Ah, that's a shame.  All right, so be it.
> 
> > The patch below does fix the issue, but I'm wondering if there's a
> > deeper problem here that can only be properly solved by adding some
> > device/kobject hierarchy to the config side of things.
> 
> I don't believe there's any deeper problem.  If someone wants to have an 
> fhidg device as a child of the gadget, I think it could be added and 
> removed in the ->set_alt() and ->disable() callbacks.  Or maybe the 
> ->bind() and ->unbind() callbacks (I've never had to work with configfs 
> so I'm not clear on the details).

It turns out there's already a device being created here, just not
associated with the structure.  Your suggestions around
cdev_device_add() made me spot what's going on with that so the actual
fix is to pull its lifetime up to match struct f_hidg.

-- >8 --
Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev

The embedded struct cdev does not have its lifetime correctly tied to
the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
is held open while the gadget is deleted.

This can readily be replicated with libusbgx's example programs (for
conciseness - operating directly via configfs is equivalent):

	gadget-hid
	exec 3<> /dev/hidg0
	gadget-vid-pid-remove
	exec 3<&-

Pull the existing device up in to struct f_hidg and make use of the
cdev_device_{add,del}() helpers.  This changes the lifetime of the
device object to match struct f_hidg, but note that it is still added
and deleted at the same time.

[Also fix refcount leak on an error path.]

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34..0b94668a3812 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -71,7 +71,7 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
-	int				minor;
+	struct device			dev;
 	struct cdev			cdev;
 	struct usb_function		func;
 
@@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 
+static void hidg_release(struct device *dev)
+{
+	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
+
+	kfree(hidg->set_report_buf);
+	kfree(hidg);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	struct usb_ep		*ep;
 	struct f_hidg		*hidg = func_to_hidg(f);
 	struct usb_string	*us;
-	struct device		*device;
 	int			status;
-	dev_t			dev;
 
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,
@@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 
 	/* create char device */
 	cdev_init(&hidg->cdev, &f_hidg_fops);
-	dev = MKDEV(major, hidg->minor);
-	status = cdev_add(&hidg->cdev, dev, 1);
+	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
+	status = cdev_device_add(&hidg->cdev, &hidg->dev);
 	if (status)
 		goto fail_free_descs;
 
-	device = device_create(hidg_class, NULL, dev, NULL,
-			       "%s%d", "hidg", hidg->minor);
-	if (IS_ERR(device)) {
-		status = PTR_ERR(device);
-		goto del;
-	}
-
 	return 0;
-del:
-	cdev_del(&hidg->cdev);
 fail_free_descs:
 	usb_free_all_descriptors(f);
 fail:
@@ -1244,9 +1241,7 @@ static void hidg_free(struct usb_function *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);
+	put_device(&hidg->dev);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);
@@ -1256,8 +1251,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct f_hidg *hidg = func_to_hidg(f);
 
-	device_destroy(hidg_class, MKDEV(major, hidg->minor));
-	cdev_del(&hidg->cdev);
+	cdev_device_del(&hidg->cdev);
 
 	usb_free_all_descriptors(f);
 }
@@ -1266,6 +1260,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 {
 	struct f_hidg *hidg;
 	struct f_hid_opts *opts;
+	int ret;
 
 	/* allocate and initialize one new instance */
 	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
@@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	++opts->refcnt;
 
-	hidg->minor = opts->minor;
+	device_initialize(&hidg->dev);
+	hidg->dev.release = hidg_release;
+	hidg->dev.class = hidg_class;
+	hidg->dev.devt = MKDEV(major, opts->minor);
+	ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor);
+	if (ret) {
+		--opts->refcnt;
+		mutex_unlock(&opts->lock);
+		return ERR_PTR(ret);
+	}
+
 	hidg->bInterfaceSubClass = opts->subclass;
 	hidg->bInterfaceProtocol = opts->protocol;
 	hidg->report_length = opts->report_length;
 	hidg->report_desc_length = opts->report_desc_length;
 	if (opts->report_desc) {
-		hidg->report_desc = kmemdup(opts->report_desc,
+		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
 					    opts->report_desc_length,
 					    GFP_KERNEL);
 		if (!hidg->report_desc) {
-			kfree(hidg);
+			put_device(&hidg->dev);
+			--opts->refcnt;
 			mutex_unlock(&opts->lock);
 			return ERR_PTR(-ENOMEM);
 		}
-- 
2.38.1

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

[Alan Stern]

On Mon, Nov 21, 2022 at 06:54:55PM +0000, John Keeping wrote:
> It turns out there's already a device being created here, just not
> associated with the structure.  Your suggestions around
> cdev_device_add() made me spot what's going on with that so the actual
> fix is to pull its lifetime up to match struct f_hidg.

It's not obvious from the patch what device was already being created, 
but never mind...

> -- >8 --
> Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> 
> The embedded struct cdev does not have its lifetime correctly tied to
> the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
> is held open while the gadget is deleted.
> 
> This can readily be replicated with libusbgx's example programs (for
> conciseness - operating directly via configfs is equivalent):
> 
> 	gadget-hid
> 	exec 3<> /dev/hidg0
> 	gadget-vid-pid-remove
> 	exec 3<&-
> 
> Pull the existing device up in to struct f_hidg and make use of the
> cdev_device_{add,del}() helpers.  This changes the lifetime of the
> device object to match struct f_hidg, but note that it is still added
> and deleted at the same time.
> 
> [Also fix refcount leak on an error path.]
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index ca0a7d9eaa34..0b94668a3812 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c

> @@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	/* create char device */
>  	cdev_init(&hidg->cdev, &f_hidg_fops);
> -	dev = MKDEV(major, hidg->minor);
> -	status = cdev_add(&hidg->cdev, dev, 1);
> +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);

This line isn't needed; cdev_device_add() does it for you because 
hidg->dev.devt has been set.

> +	status = cdev_device_add(&hidg->cdev, &hidg->dev);
>  	if (status)
>  		goto fail_free_descs;

> @@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  	mutex_lock(&opts->lock);
>  	++opts->refcnt;
>  
> -	hidg->minor = opts->minor;
> +	device_initialize(&hidg->dev);
> +	hidg->dev.release = hidg_release;
> +	hidg->dev.class = hidg_class;
> +	hidg->dev.devt = MKDEV(major, opts->minor);
> +	ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor);
> +	if (ret) {
> +		--opts->refcnt;
> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(ret);
> +	}
> +

Otherwise this looks okay (although I don't know any of the details of 
how fhidg works, so you shouldn't take my word for it).

Alan Stern

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

[Lee Jones]

On Mon, 21 Nov 2022, John Keeping wrote:
65;6999;1c
> On Mon, Nov 21, 2022 at 11:18:34AM -0500, Alan Stern wrote:
> > On Mon, Nov 21, 2022 at 12:38:37PM +0000, John Keeping wrote:
> > > On Sun, Nov 20, 2022 at 03:46:26PM -0500, Alan Stern wrote:
> > > > I see.  The solution is simple: Embed a struct device in struct f_hidg, 
> > > > and call cdev_device_add() to add the device and the cdev.  This will 
> > > > automatically make the device the parent of the cdev, so the device's 
> > > > refcount won't go to 0 until the cdev's refcount does.  Then you can tie 
> > > > the f_hidg's lifetime to the device's, so the device's release routine 
> > > > can safely deallocate the entire f_hidg structure.
> > > > 
> > > > The parent of the new struct device should be set to &gadget->dev.  If 
> > > > you can't think of a better name for the device, you could simply append 
> > > > ":I" to the parent's name, where I is the interface number, or even 
> > > > append ":C.I" where C is the config number (like we do on the host 
> > > > side).
> > > 
> > > There is no gadget->dev at the time struct f_hidg is allocated.
> > > 
> > > AFAICT the device is the UDC, which is only associated with the gadget
> > > when it is bound.  The functions are allocated earlier than this and I
> > > can't see any device associated with struct usb_function_instance.
> > 
> > Ah, that's a shame.  All right, so be it.
> > 
> > > The patch below does fix the issue, but I'm wondering if there's a
> > > deeper problem here that can only be properly solved by adding some
> > > device/kobject hierarchy to the config side of things.
> > 
> > I don't believe there's any deeper problem.  If someone wants to have an 
> > fhidg device as a child of the gadget, I think it could be added and 
> > removed in the ->set_alt() and ->disable() callbacks.  Or maybe the 
> > ->bind() and ->unbind() callbacks (I've never had to work with configfs 
> > so I'm not clear on the details).
> 
> It turns out there's already a device being created here, just not
> associated with the structure.  Your suggestions around
> cdev_device_add() made me spot what's going on with that so the actual
> fix is to pull its lifetime up to match struct f_hidg.
> 
> -- >8 --
> Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> 
> The embedded struct cdev does not have its lifetime correctly tied to
> the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
> is held open while the gadget is deleted.
> 
> This can readily be replicated with libusbgx's example programs (for
> conciseness - operating directly via configfs is equivalent):
> 
> 	gadget-hid
> 	exec 3<> /dev/hidg0
> 	gadget-vid-pid-remove
> 	exec 3<&-
> 
> Pull the existing device up in to struct f_hidg and make use of the
> cdev_device_{add,del}() helpers.  This changes the lifetime of the
> device object to match struct f_hidg, but note that it is still added
> and deleted at the same time.

This is much better, thanks for re-spinning.

> [Also fix refcount leak on an error path.]
> 
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>  drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index ca0a7d9eaa34..0b94668a3812 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -71,7 +71,7 @@ struct f_hidg {
>  	wait_queue_head_t		write_queue;
>  	struct usb_request		*req;
>  
> -	int				minor;
> +	struct device			dev;
>  	struct cdev			cdev;
>  	struct usb_function		func;
>  
> @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
>  	return container_of(f, struct f_hidg, func);
>  }
>  
> +static void hidg_release(struct device *dev)
> +{
> +	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);

Could we store/fetch this with dev_{set,get}_drvdata(), and make
hidg->dev a pointer reducing the size of the struct f_hidg.

> +	kfree(hidg->set_report_buf);
> +	kfree(hidg);
> +}
> +
>  /*-------------------------------------------------------------------------*/
>  /*                           Static descriptors                            */
>  
> @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  	struct usb_ep		*ep;
>  	struct f_hidg		*hidg = func_to_hidg(f);
>  	struct usb_string	*us;
> -	struct device		*device;
>  	int			status;
> -	dev_t			dev;
>  
>  	/* maybe allocate device-global string IDs, and patch descriptors */
>  	us = usb_gstrings_attach(c->cdev, ct_func_strings,
> @@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	/* create char device */
>  	cdev_init(&hidg->cdev, &f_hidg_fops);
> -	dev = MKDEV(major, hidg->minor);
> -	status = cdev_add(&hidg->cdev, dev, 1);
> +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);

cdev_device_add() should take care of this, so long as:

    if (dev->devt)
        dev_set_parent(cdev, &dev->kobj);

> +	status = cdev_device_add(&hidg->cdev, &hidg->dev);
>  	if (status)
>  		goto fail_free_descs;
>  
> -	device = device_create(hidg_class, NULL, dev, NULL,
> -			       "%s%d", "hidg", hidg->minor);
> -	if (IS_ERR(device)) {
> -		status = PTR_ERR(device);
> -		goto del;
> -	}
> -
>  	return 0;
> -del:
> -	cdev_del(&hidg->cdev);
>  fail_free_descs:
>  	usb_free_all_descriptors(f);
>  fail:
> @@ -1244,9 +1241,7 @@ static void hidg_free(struct usb_function *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);
> +	put_device(&hidg->dev);
>  	mutex_lock(&opts->lock);
>  	--opts->refcnt;
>  	mutex_unlock(&opts->lock);
> @@ -1256,8 +1251,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
>  {
>  	struct f_hidg *hidg = func_to_hidg(f);
>  
> -	device_destroy(hidg_class, MKDEV(major, hidg->minor));
> -	cdev_del(&hidg->cdev);
> +	cdev_device_del(&hidg->cdev);
>  
>  	usb_free_all_descriptors(f);
>  }
> @@ -1266,6 +1260,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  {
>  	struct f_hidg *hidg;
>  	struct f_hid_opts *opts;
> +	int ret;
>  
>  	/* allocate and initialize one new instance */
>  	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
> @@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
>  	mutex_lock(&opts->lock);
>  	++opts->refcnt;
>  
> -	hidg->minor = opts->minor;
> +	device_initialize(&hidg->dev);
> +	hidg->dev.release = hidg_release;
> +	hidg->dev.class = hidg_class;
> +	hidg->dev.devt = MKDEV(major, opts->minor);
> +	ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor);
> +	if (ret) {
> +		--opts->refcnt;

Since we're holding the opts lock at this point, is there anything
preventing us from incrementing the refcnt at the end, just before
giving up the lock, thus saving 2 decrements in the error paths?

> +		mutex_unlock(&opts->lock);
> +		return ERR_PTR(ret);
> +	}
> +
>  	hidg->bInterfaceSubClass = opts->subclass;
>  	hidg->bInterfaceProtocol = opts->protocol;
>  	hidg->report_length = opts->report_length;
>  	hidg->report_desc_length = opts->report_desc_length;
>  	if (opts->report_desc) {
> -		hidg->report_desc = kmemdup(opts->report_desc,
> +		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,

Nice.

>  					    opts->report_desc_length,
>  					    GFP_KERNEL);
>  		if (!hidg->report_desc) {
> -			kfree(hidg);
> +			put_device(&hidg->dev);
> +			--opts->refcnt;
>  			mutex_unlock(&opts->lock);
>  			return ERR_PTR(-ENOMEM);
>  		}

Thanks for doing this John, your work is appreciated.

-- 
Lee Jones [李琼斯]

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

[John Keeping]

On Mon, Nov 21, 2022 at 02:17:56PM -0500, Alan Stern wrote:
> On Mon, Nov 21, 2022 at 06:54:55PM +0000, John Keeping wrote:
> > It turns out there's already a device being created here, just not
> > associated with the structure.  Your suggestions around
> > cdev_device_add() made me spot what's going on with that so the actual
> > fix is to pull its lifetime up to match struct f_hidg.
> 
> It's not obvious from the patch what device was already being created, 
> but never mind...

The patch has:

-       device = device_create(hidg_class, NULL, dev, NULL,
-                              "%s%d", "hidg", hidg->minor);

but this device was not previously associated with the cdev (apart from
indirectly via dev_t).

> > -- >8 --
> > Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> > 
> > The embedded struct cdev does not have its lifetime correctly tied to
> > the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
> > is held open while the gadget is deleted.
> > 
> > This can readily be replicated with libusbgx's example programs (for
> > conciseness - operating directly via configfs is equivalent):
> > 
> > 	gadget-hid
> > 	exec 3<> /dev/hidg0
> > 	gadget-vid-pid-remove
> > 	exec 3<&-
> > 
> > Pull the existing device up in to struct f_hidg and make use of the
> > cdev_device_{add,del}() helpers.  This changes the lifetime of the
> > device object to match struct f_hidg, but note that it is still added
> > and deleted at the same time.
> > 
> > [Also fix refcount leak on an error path.]
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> > index ca0a7d9eaa34..0b94668a3812 100644
> > --- a/drivers/usb/gadget/function/f_hid.c
> > +++ b/drivers/usb/gadget/function/f_hid.c
> 
> > @@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
> >  
> >  	/* create char device */
> >  	cdev_init(&hidg->cdev, &f_hidg_fops);
> > -	dev = MKDEV(major, hidg->minor);
> > -	status = cdev_add(&hidg->cdev, dev, 1);
> > +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
> 
> This line isn't needed; cdev_device_add() does it for you because 
> hidg->dev.devt has been set.

Thanks, I'll drop this line.

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

[John Keeping]

On Tue, Nov 22, 2022 at 08:31:08AM +0000, Lee Jones wrote:
> On Mon, 21 Nov 2022, John Keeping wrote:
> > Subject: [PATCH] usb: gadget: f_hid: fix f_hidg lifetime vs cdev
> > 
> > The embedded struct cdev does not have its lifetime correctly tied to
> > the enclosing struct f_hidg, so there is a use-after-free if /dev/hidgN
> > is held open while the gadget is deleted.
> > 
> > This can readily be replicated with libusbgx's example programs (for
> > conciseness - operating directly via configfs is equivalent):
> > 
> > 	gadget-hid
> > 	exec 3<> /dev/hidg0
> > 	gadget-vid-pid-remove
> > 	exec 3<&-
> > 
> > Pull the existing device up in to struct f_hidg and make use of the
> > cdev_device_{add,del}() helpers.  This changes the lifetime of the
> > device object to match struct f_hidg, but note that it is still added
> > and deleted at the same time.
> 
> This is much better, thanks for re-spinning.
> 
> > [Also fix refcount leak on an error path.]
> > 
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> >  drivers/usb/gadget/function/f_hid.c | 50 ++++++++++++++++-------------
> >  1 file changed, 28 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> > index ca0a7d9eaa34..0b94668a3812 100644
> > --- a/drivers/usb/gadget/function/f_hid.c
> > +++ b/drivers/usb/gadget/function/f_hid.c
> > @@ -71,7 +71,7 @@ struct f_hidg {
> >  	wait_queue_head_t		write_queue;
> >  	struct usb_request		*req;
> >  
> > -	int				minor;
> > +	struct device			dev;
> >  	struct cdev			cdev;
> >  	struct usb_function		func;
> >  
> > @@ -84,6 +84,14 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
> >  	return container_of(f, struct f_hidg, func);
> >  }
> >  
> > +static void hidg_release(struct device *dev)
> > +{
> > +	struct f_hidg *hidg = container_of(dev, struct f_hidg, dev);
> 
> Could we store/fetch this with dev_{set,get}_drvdata(), and make
> hidg->dev a pointer reducing the size of the struct f_hidg.

That will reduce the size of struct f_hidg, but we'll have an extra
allocation for the device object, so I don't think that's a real
benefit.

It seems simpler to keep a single allocation and embed the device.

> > +	kfree(hidg->set_report_buf);
> > +	kfree(hidg);
> > +}
> > +
> >  /*-------------------------------------------------------------------------*/
> >  /*                           Static descriptors                            */
> >  
> > @@ -904,9 +912,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
> >  	struct usb_ep		*ep;
> >  	struct f_hidg		*hidg = func_to_hidg(f);
> >  	struct usb_string	*us;
> > -	struct device		*device;
> >  	int			status;
> > -	dev_t			dev;
> >  
> >  	/* maybe allocate device-global string IDs, and patch descriptors */
> >  	us = usb_gstrings_attach(c->cdev, ct_func_strings,
> > @@ -999,21 +1005,12 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
> >  
> >  	/* create char device */
> >  	cdev_init(&hidg->cdev, &f_hidg_fops);
> > -	dev = MKDEV(major, hidg->minor);
> > -	status = cdev_add(&hidg->cdev, dev, 1);
> > +	cdev_set_parent(&hidg->cdev, &hidg->dev.kobj);
> 
> cdev_device_add() should take care of this, so long as:
> 
>     if (dev->devt)
>         dev_set_parent(cdev, &dev->kobj);

Thanks, I'll change this.

> > +	status = cdev_device_add(&hidg->cdev, &hidg->dev);
> >  	if (status)
> >  		goto fail_free_descs;
> >  
> > -	device = device_create(hidg_class, NULL, dev, NULL,
> > -			       "%s%d", "hidg", hidg->minor);
> > -	if (IS_ERR(device)) {
> > -		status = PTR_ERR(device);
> > -		goto del;
> > -	}
> > -
> >  	return 0;
> > -del:
> > -	cdev_del(&hidg->cdev);
> >  fail_free_descs:
> >  	usb_free_all_descriptors(f);
> >  fail:
> > @@ -1244,9 +1241,7 @@ static void hidg_free(struct usb_function *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);
> > +	put_device(&hidg->dev);
> >  	mutex_lock(&opts->lock);
> >  	--opts->refcnt;
> >  	mutex_unlock(&opts->lock);
> > @@ -1256,8 +1251,7 @@ static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
> >  {
> >  	struct f_hidg *hidg = func_to_hidg(f);
> >  
> > -	device_destroy(hidg_class, MKDEV(major, hidg->minor));
> > -	cdev_del(&hidg->cdev);
> > +	cdev_device_del(&hidg->cdev);
> >  
> >  	usb_free_all_descriptors(f);
> >  }
> > @@ -1266,6 +1260,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
> >  {
> >  	struct f_hidg *hidg;
> >  	struct f_hid_opts *opts;
> > +	int ret;
> >  
> >  	/* allocate and initialize one new instance */
> >  	hidg = kzalloc(sizeof(*hidg), GFP_KERNEL);
> > @@ -1277,17 +1272,28 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
> >  	mutex_lock(&opts->lock);
> >  	++opts->refcnt;
> >  
> > -	hidg->minor = opts->minor;
> > +	device_initialize(&hidg->dev);
> > +	hidg->dev.release = hidg_release;
> > +	hidg->dev.class = hidg_class;
> > +	hidg->dev.devt = MKDEV(major, opts->minor);
> > +	ret = dev_set_name(&hidg->dev, "hidg%d", opts->minor);
> > +	if (ret) {
> > +		--opts->refcnt;
> 
> Since we're holding the opts lock at this point, is there anything
> preventing us from incrementing the refcnt at the end, just before
> giving up the lock, thus saving 2 decrements in the error paths?

This makes sense.  I'll respin this as a series and include a patch
tidying up the error handling as a final step.

> > +		mutex_unlock(&opts->lock);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> >  	hidg->bInterfaceSubClass = opts->subclass;
> >  	hidg->bInterfaceProtocol = opts->protocol;
> >  	hidg->report_length = opts->report_length;
> >  	hidg->report_desc_length = opts->report_desc_length;
> >  	if (opts->report_desc) {
> > -		hidg->report_desc = kmemdup(opts->report_desc,
> > +		hidg->report_desc = devm_kmemdup(&hidg->dev, opts->report_desc,
> 
> Nice.
> 
> >  					    opts->report_desc_length,
> >  					    GFP_KERNEL);
> >  		if (!hidg->report_desc) {
> > -			kfree(hidg);
> > +			put_device(&hidg->dev);
> > +			--opts->refcnt;
> >  			mutex_unlock(&opts->lock);
> >  			return ERR_PTR(-ENOMEM);
> >  		}
> 
> Thanks for doing this John, your work is appreciated.

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@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: linux-usb@vger.kernel.org
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> >  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 [李琼斯]

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@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>  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

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

[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 <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Lee Jones <lee@kernel.org>
---
 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;
 };
 
 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);
 
-- 
2.38.0.413.g74048e4d9e-goog