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

[Christian Borntraeger <borntraeger@linux.ibm.com>]

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