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

[Kirti Wankhede]

On 6/24/2022 6:35 PM, Jason Gunthorpe wrote:
> 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
> 

that's correct, my bad.

> 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?
> 

Pasting here your comment:
 >>>>> 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.

Since there references are held, its safe.

Thanks,
Kirti