Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/2/2019 8:13 PM, Alex Williamson wrote:
> On Tue, 2 Jul 2019 19:10:17 +0530
> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> 
>> On 7/2/2019 6:38 PM, Alex Williamson wrote:
>>> On Tue, 2 Jul 2019 18:17:41 +0530
>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>>   
>>>> On 7/2/2019 12:43 PM, Parav Pandit wrote:  
>>>>>
>>>>>     
>>>>>> -----Original Message-----
>>>>>> From: linux-kernel-owner@xxxxxxxxxxxxxxx <linux-kernel-    
>>>>>> owner@xxxxxxxxxxxxxxx> On Behalf Of Alex Williamson    
>>>>>> Sent: Tuesday, July 2, 2019 11:12 AM
>>>>>> To: Kirti Wankhede <kwankhede@xxxxxxxxxx>
>>>>>> Cc: cohuck@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>>>>>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
>>>>>>
>>>>>> On Tue, 2 Jul 2019 10:25:04 +0530
>>>>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>>>>>    
>>>>>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:    
>>>>>>>> On Mon, 1 Jul 2019 23:20:35 +0530
>>>>>>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>>>>>>>    
>>>>>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:    
>>>>>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
>>>>>>>>>> Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
>>>>>>>>>>    
>>>>>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:    
>>>>>>>>>>>> This allows udev to trigger rules when a parent device is
>>>>>>>>>>>> registered or unregistered from mdev.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>>>>>>>>>>     removing them is only tangential to the goal of this change.
>>>>>>>>>>>>    
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>>    
>>>>>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
>>>>>>>>>>>> 100644
>>>>>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
>>>>>>>>>>>> const struct mdev_parent_ops *ops)  {
>>>>>>>>>>>>  	int ret;
>>>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
>>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>>>
>>>>>>>>>>>>  	/* check for mandatory ops */
>>>>>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
>>>>>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int    
>>>>>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)    
>>>>>>>>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>>>>>>>>
>>>>>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
>>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>>>>>>>>> +
>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>
>>>>>>>>>>>>  add_dev_err:
>>>>>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>>>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
>>>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
>>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>>>
>>>>>>>>>>>>  	mutex_lock(&parent_list_lock);
>>>>>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@    
>>>>>> void    
>>>>>>>>>>>> mdev_unregister_device(struct device *dev)
>>>>>>>>>>>>  	up_write(&parent->unreg_sem);
>>>>>>>>>>>>
>>>>>>>>>>>>  	mdev_put_parent(parent);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);    
>>>>>>>>>>>
>>>>>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
>>>>>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
>>>>>>>>>>>
>>>>>>>>>>> This event should be before mdev_put_parent()    
>>>>>>>>>>
>>>>>>>>>> So you're suggesting the vendor driver is calling
>>>>>>>>>> mdev_unregister_device() without a reference to the struct device
>>>>>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
>>>>>>>>>> reference to the device so that it can't disappear out from under
>>>>>>>>>> us, the caller cannot rely on our reference and the caller
>>>>>>>>>> provided the struct device.  Thanks,
>>>>>>>>>>    
>>>>>>>>>
>>>>>>>>> 1. Register uevent is sent after mdev holding reference to device,
>>>>>>>>> then ideally, unregister path should be mirror of register path,
>>>>>>>>> send uevent and then release the reference to device.    
>>>>>>>>
>>>>>>>> I don't see the relevance here.  We're marking an event, not
>>>>>>>> unwinding state of the device from the registration process.
>>>>>>>> Additionally, the event we're trying to mark is the completion of
>>>>>>>> each process, so the notion that we need to mirror the ordering between    
>>>>>> the two is invalid.    
>>>>>>>>    
>>>>>>>>> 2. I agree that vendor driver shouldn't call
>>>>>>>>> mdev_unregister_device() without holding reference to device. But
>>>>>>>>> to be on safer side, if ever such case occur, to avoid any
>>>>>>>>> segmentation fault in kernel, better to send event before mdev release the    
>>>>>> reference to device.    
>>>>>>>>
>>>>>>>> I know that get_device() and put_device() are GPL symbols and that's
>>>>>>>> a bit of an issue, but I don't think we should be kludging the code
>>>>>>>> for a vendor driver that might have problems with that.  A) we're
>>>>>>>> using the caller provided device  for the uevent, B) we're only
>>>>>>>> releasing our own reference to the device that was acquired during
>>>>>>>> registration, the vendor driver must have other references,    
>>>>>>>
>>>>>>> Are you going to assume that someone/vendor driver is always going to
>>>>>>> do right thing?    
>>>>>>
>>>>>> mdev is a kernel driver, we make reasonable assumptions that other drivers
>>>>>> interact with it correctly.
>>>>>>    
>>>>> That is right.
>>>>> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
>>>>> And it must have a valid reference to the device for which it is invoking it.
>>>>> This is basic programming practice that a given driver has to follow.
>>>>> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
>>>>> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.    
>>>>
>>>> If assumption is vendor driver is always going to do right way, then why
>>>> need check for duplicate registration? vendor driver is always going to
>>>> do it right way, right?  
>>>
>>> Are we intentionally misinterpreting "reasonable assumptions" here?
>>>   
>>>>> It was in my TODO list to submit those patches.
>>>>> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
>>>>>
>>>>>     
>>>>>>>> C) the parent device
>>>>>>>> generally lives on a bus, with a vendor driver, there's an entire
>>>>>>>> ecosystem of references to the device below mdev.  Is this a
>>>>>>>> paranoia request or are you really concerned that your PCI device suddenly
>>>>>>>> disappears when mdev's reference to it disappears.    
>>>>>>>
>>>>>>> mdev infrastructure is not always used by PCI devices. It is designed
>>>>>>> to be generic, so that other devices (other than PCI devices) can also
>>>>>>> use this framework.    
>>>>>>
>>>>>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
>>>>>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
>>>>>> backed vGPU.    
>>>>
>>>> Its not always good to assume certain things.  
>>>
>>> It was only an attempt to relate to a specific issue that might concern
>>> you.
>>>   
>>>>>> Any physical parent device of an mdev is likely to have some sort
>>>>>> of bus infrastructure behind it holding references to the device (ie. a probe and
>>>>>> release where an implicit reference is held between these points).  A virtual
>>>>>> device would be similar, it's created as part of a module init and destroyed as
>>>>>> part of a module exit, where mdev registration would exist between these
>>>>>> points.
>>>>>>    
>>>>>>> If there is a assumption that user of mdev framework or vendor drivers
>>>>>>> are always going to use mdev in right way, then there is no need for
>>>>>>> mdev core to held reference of the device?
>>>>>>> This is not a "paranoia request". This is more of a ideal scenario,
>>>>>>> mdev should use device by holding its reference rather than assuming
>>>>>>> (or relying on) someone else holding the reference of device.    
>>>>>>
>>>>>> In fact, at one point Parav was proposing removing these references entirely,
>>>>>> but Connie and I both felt uncomfortable about that.  I think it's good practice
>>>>>> that mdev indicates the use of the parent device by incrementing the reference
>>>>>> count, with each child mdev device also taking a reference, but those
>>>>>> references balance out within the mdev core.  Their purpose is not to maintain
>>>>>> the device for outside callers, nor should outside callers assume mdev's use of
>>>>>> references to release their own.  I don't think it's unreasonable to assume that
>>>>>> the caller should have a legitimate reference to the object it's providing to this
>>>>>> function and therefore we should be able to use it after mdev's internal
>>>>>> references are balanced out.  Thanks,
>>>>>>    
>>>>
>>>> I'm not fully convinced with what is the advantage of sending uevent
>>>> after releasing reference to device or disadvantage of sending uevent
>>>> before releasing reference to device.  
>>>
>>> If mdev-core still holds a reference to the device, is it fully
>>> unregistered?  Why not send the uevent at the point where the
>>> notification is actually true?
>>>  
>>
>> By that time, device is removed from parent list, each child is removed
>> and sysfs files related to that parent are removed so that no new child
>> can be created, which means device is unregistered, only mdev_parent
>> structure is not yet freed which gets freed from mdev_put_parent().
> 
> So you're saying it's 95% unregistered, but there's still a tracking
> structure yet to free,

Its almost unregistered, that tracking structure is unusable since all
other interfaces are already removed and it is also removed from parent
list.

> so go ahead an send a uevent just in case the
> caller didn't have a valid reference to the device they passed and it
> might get freed.  Isn't this the original request which we've decided
> is unreasonable paranoia?  Please cite an instance where this makes any
> sense.  Someone called us with a reference to the device to register
> with mdev.  The references we've acquired are entirely balanced within
> the mdev-core and you're suggesting that the unregistration caller has
> released their own reference to the device and now relies on ours,
> which we're under no obligation to hold in the first place.  We're
> using a caller provided object after tearing down our own internal
> tracking, which should have no bearing on external tracking of this
> object.  How can that be an issue?  Thanks,
> 

You gave a reference in previous mail, release is deferred via workqueue

> If CONFIG_DEBUG_KOBJECT_RELEASE is enabled then the deletion of the
> kobject can occur at some random delay after the last reference is
> removed via a workqueue,

similarly if someone defers unregistration, this situation might occur.
Again this is a very acute corner case.
I still think right way is to use device by holding its reference rather
than assuming someone else holding the reference of device.

Thanks,
Kirti