Message-ID: <Y3e0zAa7+HiNVrKN@donbot>
Date:   Fri, 18 Nov 2022 16:37:32 +0000
From:   John Keeping <john@...anate.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Lee Jones <lee@...nel.org>, Greg KH <gregkh@...uxfoundation.org>,
        balbi@...nel.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on
 shared f_hidg pointer

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.

Powered by blists - more mailing lists