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

[Greg Kroah-Hartman]

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.

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?

thanks,

greg k-h