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

[Alan Stern <stern@rowland.harvard.edu>]

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