Re: [PATCH 02/13] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jun 24, 2022 at 06:23:48PM +0530, Kirti Wankhede wrote:
> 
> 
> On 6/24/2022 6:03 PM, Jason Gunthorpe wrote:
> > On Fri, Jun 24, 2022 at 05:59:58PM +0530, Kirti Wankhede wrote:
> > 
> > > > The reason this is here is because the type->parent is used in a few
> > > > places and is put back in release:
> > > > 
> > > > @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj)
> > > > 
> > > >           pr_debug("Releasing group %s\n", kobj->name);
> > > >           /* Pairs with the get in add_mdev_supported_type() */
> > > > -       mdev_put_parent(type->parent);
> > > > +       put_device(type->parent->dev);
> > > >           kfree(type);
> > > >    }
> > > > 
> > > > If this was a simple sysfs kobj with only a show/store we wouldn't
> > > > need to do anything as the natural kobj parentage holds a ref up to
> > > > the struct device - but this kobj is used internally, ie dependent
> > > > from mdev_device_create(), independently of the normal sysfs
> > > > life-cycle so that doesn't protect enough either.
> > > > 
> > > 
> > > 
> > > Life span of 'type' is from mdev_register_device to mdev_unregister_device.
> > > If device/parent is being unregistered then only types are removed, so
> > > referencing 'type' from mdev_device_create() is still safe. Therefore,
> > > parent device's reference should be held and release from
> > > register-unregister call.
> > 
> > No, I've already explained this.
> 
> Its not correct.
> 
> kobject_init_and_add(&type->kobj, ...) which called from
> mdev_register_parent()
>     -> parent_create_sysfs_files() holds reference for type->kobj
          -> add_mdev_supported_type_groups()
               -> add_mdev_supported_type()
                   -> kobject_init_and_add(&type->kobj)

> This is released from
>  mdev_unregister_parent()
>      -> parent_remove_sysfs_files()
>          -> kset_unregister()

It is not kset_unregister() that puts back.
           -> remove_mdev_supported_type()
	       -> kobject_put(&type->kobj) // pairs with kobject_init_and_add

So what is the issue? This is a properly paired usage of the ref.

> In the next patch [3/13] of this series, these calltraces are changed as
> mdev_register_parent()
>     -> mdev_type_add()
>         -> kobject_init_and_add(&type->kobj, ...) holds reference for
> type->kobj
> 
> which is released from
> 
> mdev_unregister_parent()
>     -> mdev_type_remove()
>         -> kobject_put(&type->kobj)

This is the same logic? What is the problem?

Jason