Re: [PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove

From: Tony Krowiak <akrowiak@linux.ibm.com>
To: Jason Gunthorpe <jgg@nvidia.com>,
	Matthew Rosato <mjrosato@linux.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>,
	"Jason J. Herne" <jjherne@linux.ibm.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Liu Yi L <yi.l.liu@intel.com>, Halil Pasic <pasic@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org
Subject: Re: [PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove
Date: Fri, 3 Jun 2022 11:20:48 -0400	[thread overview]
Message-ID: <feaceeba-6d8b-513e-2611-267eaf23124a@linux.ibm.com> (raw)
In-Reply-To: <20220603133713.GZ1343366@nvidia.com>

On 6/3/22 9:37 AM, Jason Gunthorpe wrote:
> On Fri, Jun 03, 2022 at 09:25:19AM -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> The mdev is linked with the vfio_ccw_private pointer when the mdev
>>> is probed, but it's not cleared once the mdev is removed.
>>>
>>> This isn't much of a concern based on the current device lifecycle,
>>> but fix it so that things make sense in later shuffling.
>>>
>>> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>    drivers/s390/cio/vfio_ccw_ops.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>>> index a403d059a4e6..a0a3200b0b04 100644
>>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>>> @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>>>    			   private->sch->schid.ssid,
>>>    			   private->sch->schid.sch_no);
>>> +	dev_set_drvdata(&mdev->dev, NULL);
>>>    	vfio_unregister_group_dev(&private->vdev);
>>>    	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
>> Seems harmless enough.
>>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>
>> But is this just precautionary or is it fixing a real problem (if the former
>> I don't think a fixes tag makes sense)
>>
>> I also ask because I note vfio-ap clears its driver_data in mdev_remove but
>> also leaves the pointer set, meaning they might need a similar cleanup and
>> should probably have a look (CC Tony & Jason H)
> There should be no reason to clear the drvdata on remove - the driver
> must be designed to guarentee all references to the dev stop before
> the remove function returns.

Each function that gets the driver data
backs a sysfs attribute of the mdev. If the mdev is removed, these
attributes will not exist. The mdev remove callback must get the
same locks acquired by the sysfs attribute functions before clearing
the driver data, so I believe this guarantees that all references to
the dev stop before the remove function returns.

>
> Jason