Re: [PATCH net-next] auxiliary: Implement refcounting

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Feb 16, 2023 at 02:42:41PM +0200, Leon Romanovsky wrote:
> + Greg KH
> 
> On Thu, Feb 16, 2023 at 01:16:21PM +0100, Temerkhanov, Sergey wrote:
> > From: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> > 
> > Implement reference counting to make it possible to synchronize
> > deinitialization and removal of interfaces published via aux bus
> > with the client modules.
> > Reference counting can be used in both sleeping and non-sleeping
> > contexts so this approach is intended to replace device_lock()
> > (mutex acquisition) with an additional lock on top of it
> > which is not always possible to take in client code.
> 
> I want to see this patch as part of your client code series.
> It is unclear why same aux device is used from different drivers.
> 
> Otherwise, this whole refcnt is useless and just hides bugs in driver
> which accesses released devices.
> 
> > 
> > Signed-off-by: Sergey Temerkhanov <sergey.temerkhanov@intel.com>
> > ---
> >  drivers/base/auxiliary.c      | 18 ++++++++++++++++++
> >  include/linux/auxiliary_bus.h | 34 +++++++++++++++++++++++++---------
> >  2 files changed, 43 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
> > index 8c5e65930617..082b3ebd143d 100644
> > --- a/drivers/base/auxiliary.c
> > +++ b/drivers/base/auxiliary.c
> > @@ -287,10 +287,28 @@ int auxiliary_device_init(struct auxiliary_device *auxdev)
> >  
> >  	dev->bus = &auxiliary_bus_type;
> >  	device_initialize(&auxdev->dev);
> > +	init_waitqueue_head(&auxdev->wq_head);
> > +	refcount_set(&auxdev->refcnt, 1);
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(auxiliary_device_init);
> >  
> > +void auxiliary_device_uninit(struct auxiliary_device *auxdev)
> > +{
> > +	wait_event_interruptible(auxdev->wq_head,
> > +				 refcount_dec_if_one(&auxdev->refcnt));
> > +}
> > +EXPORT_SYMBOL_GPL(auxiliary_device_uninit);
> > +
> > +void auxiliary_device_delete(struct auxiliary_device *auxdev)
> > +{
> > +	WARN_ON(refcount_read(&auxdev->refcnt));

There are 3 things wrong with this single line of code, pretty
impressive!

First off, never use WARN_ON unless you want to reboot people's
machines.  Handle the issue if it can really happen and keep on moving.
Don't just throw up your hands and yell at userspace and stop the box
(remember panic-on-warn is real.)

Second, if this is a real problem, HANDLE IT!  Don't just free memory
as obviously someone actually has the memory in use!

Third, NEVER read the reference count and do something based on it.
That's not how reference counts work at all, unless you are using the
correct reference count function (hint, this is not it.)

> > +
> > +	device_del(&auxdev->dev);

No, no no no no .

I see why you didn't cc me on this thread to start with, you were trying
to get this by without review, that's not ok.

You now have the required penance of having to get an internal-Intel
review and signed-off-by FIRST before sending this patch series out
again.

greg k-h