Re: [char-misc-next 1/3] mei: refcount mei_device

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

On Sun, Jun 22, 2025 at 09:14:10AM +0000, Usyskin, Alexander wrote:
> > Subject: Re: [char-misc-next 1/3] mei: refcount mei_device
> > 
> > On Wed, Jun 18, 2025 at 12:54:31PM +0300, Alexander Usyskin wrote:
> > > mei_device lifetime is managed by devm procedure of parent device.
> > > But such memory is freed on device_del.
> > > Mei_device object is used by client object that may be alive after
> > > parent device is removed.
> > > It may lead to use-after-free if discrete graphics driver
> > > unloads mei_gsc auxiliary device while user-space holds
> > > open handle to mei character device.
> > >
> > > Replace devm lifetime management with reference counting
> > > to eliminate the use-after-free.
> > 
> > Overall, I like the end result, but note that if you just apply this
> > patch then:
> > 
> > > --- a/drivers/misc/mei/mei_dev.h
> > > +++ b/drivers/misc/mei/mei_dev.h
> > > @@ -474,6 +474,8 @@ struct mei_dev_timeouts {
> > >   * @cdev        : character device
> > >   * @minor       : minor number allocated for device
> > >   *
> > > + * @refcnt      : struct reference count
> > > + *
> > >   * @write_list  : write pending list
> > >   * @write_waiting_list : write completion list
> > >   * @ctrl_wr_list : pending control write list
> > > @@ -560,6 +562,8 @@ struct mei_device {
> > >  	struct cdev cdev;
> > >  	int minor;
> > >
> > > +	struct kref refcnt;
> > > +
> > >  	struct list_head write_list;
> > >  	struct list_head write_waiting_list;
> > >  	struct list_head ctrl_wr_list;
> > 
> > You now have 2 reference counts controling the lifespan of this
> > structure, and it will be a mess.
> > 
> 
> It is about cdev? But static cdev (like before third patch) is not refcounted.
> What is the second reference counter?

Yes, it is cdev, that is a reference counted object.

> > Yes, you clean it up in the last patch, so overall it's ok, this is just
> > a worrying step.
> > 
> > Also, why are you using a kref?  Why not use the real struct device if
> > you want to have a reference counted device structure?  That is what
> > should be happening here, what's wrong with the struct device * that you
> > already have?  Why not have that take over ownership instead of making a
> > newer intermediate reference counted object?
> > 
> 
> The device *dev in mei_device is the parent device.

Ah, how about naming it "parent"?

> On auxiliary bus it can be removed while driver is still active.

That's not ok.

> The device_del removes all memory allocated with devm_*.
> I can't find a way how to keep driver private memory alive
> till last reference to parent device still held.

Make it a real device please.

> This why I've resorted to manual kref.
> I'm feeling that I'm missing something here.
> Is there any other way to do it?

Yes, make it a real device.

thanks,

greg k-h