Re: [PATCH v2] vfio: Follow a strict lifetime for struct iommu_group

Christian Borntraeger
2022-10-04T17:38:12Z

Am 04.10.22 um 18:28 schrieb Jason Gunthorpe:
> On Tue, Oct 04, 2022 at 05:44:53PM +0200, Christian Borntraeger wrote:
>
>>> Does some userspace have the group FD open when it stucks like this,
>>> eg what does fuser say?
>>
>> /proc/<virtnodedevd>/fd
>> 51480 0 dr-x------. 2 root root 0 4. Okt 17:16 .
>> 43593 0 dr-xr-xr-x. 9 root root 0 4. Okt 17:16 ..
>> 65252 0 lr-x------. 1 root root 64 4. Okt 17:42 0 -> /dev/null
>> 65253 0 lrwx------. 1 root root 64 4. Okt 17:42 1 -> 'socket:[51479]'
>> 65261 0 lrwx------. 1 root root 64 4. Okt 17:42 10 -> 'anon_inode:[eventfd]'
>> 65262 0 lrwx------. 1 root root 64 4. Okt 17:42 11 -> 'socket:[51485]'
>> 65263 0 lrwx------. 1 root root 64 4. Okt 17:42 12 -> 'socket:[51487]'
>> 65264 0 lrwx------. 1 root root 64 4. Okt 17:42 13 -> 'socket:[51486]'
>> 65265 0 lrwx------. 1 root root 64 4. Okt 17:42 14 -> 'anon_inode:[eventfd]'
>> 65266 0 lrwx------. 1 root root 64 4. Okt 17:42 15 -> 'socket:[60421]'
>> 65267 0 lrwx------. 1 root root 64 4. Okt 17:42 16 -> 'anon_inode:[eventfd]'
>> 65268 0 lrwx------. 1 root root 64 4. Okt 17:42 17 -> 'socket:[28008]'
>> 65269 0 l-wx------. 1 root root 64 4. Okt 17:42 18 -> /run/libvirt/nodedev/driver.pid
>> 65270 0 lrwx------. 1 root root 64 4. Okt 17:42 19 -> 'socket:[28818]'
>> 65254 0 lrwx------. 1 root root 64 4. Okt 17:42 2 -> 'socket:[51479]'
>> 65271 0 lr-x------. 1 root root 64 4. Okt 17:42 20 -> '/dev/vfio/3 (deleted)'
>
> Seems like a userspace bug to keep the group FD open after the /dev/
> file has been deleted :|
>
> What do you think about this?
>
> commit a54a852b1484b1605917a8f4d80691db333b25ed
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date: Tue Oct 4 13:14:37 2022 -0300
>
> vfio: Make the group FD disassociate from the iommu_group
>
> Allow the vfio_group struct to exist with a NULL iommu_group pointer. When
> the pointer is NULL the vfio_group users promise not to touch the
> iommu_group. This allows a driver to be hot unplugged while userspace is
> keeping the group FD open.
>
> SPAPR mode is excluded from this behavior because of how it wrongly hacks
> part of its iommu interface through KVM. Due to this we loose control over
> what it is doing and cannot revoke the iommu_group usage in the IOMMU
> layer via vfio_group_detach_container().
>
> Thus, for SPAPR the group FDs must still be closed before a device can be
> hot unplugged.
>
> This fixes a userspace regression where we learned that virtnodedevd
> leaves a group FD open even though the /dev/ node for it has been deleted
> and all the drivers for it unplugged.
>
> Fixes: ca5f21b25749 ("vfio: Follow a strict lifetime for struct iommu_group")
> Reported-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Almost :-)

drivers/vfio/vfio_main.c: In function 'vfio_file_is_group':
drivers/vfio/vfio_main.c:1606:47: error: expected ')' before ';' token
1606 | return (file->f_op == &vfio_group_fops;
| ~ ^
| )
drivers/vfio/vfio_main.c:1606:48: error: expected ';' before '}' token
1606 | return (file->f_op == &vfio_group_fops;
| ^
| ;
1607 | }
| ~

With that fixed I get:

ERROR: modpost: "vfio_file_is_group" [drivers/vfio/pci/vfio-pci-core.ko] undefined!

With that worked around (m -> y)

Tested-by: Christian Borntraeger <borntraeger@linux.ibm.com>

At least the vfio-ap part