On 2026-02-26 03:52 PM, Alex Williamson wrote:
> On Thu, 29 Jan 2026 21:24:53 +0000 David Matlack <[email protected]> wrote:
> > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > index 8ceca24ac136..935f84a35875 100644
> > --- a/drivers/vfio/device_cdev.c
> > +++ b/drivers/vfio/device_cdev.c
> > @@ -52,6 +46,19 @@ int vfio_device_fops_cdev_open(struct inode *inode,
> > struct file *filep)
> > vfio_device_put_registration(device);
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(__vfio_device_fops_cdev_open);
>
> I really dislike that we're exporting the underscore variant, which
> implies it's an internal function that the caller should understand the
> constraints, without outlining any constraints.
>
> I'm not sure what a good alternative is. We can drop fops since this
> isn't called from file_operations. Maybe vfio_device_cdev_open_file().
Ack. Due to the bug you pointed out below, I think the changes in this
file will look fairly different in the next version. But no matter what
I'll avoid exporting a underscore variant without outlining the
constraints.
> > + /*
> > + * Simulate opening the character device using an anonymous inode. The
> > + * returned file has the same properties as a cdev file (e.g. operations
> > + * are blocked until BIND_IOMMUFD is called).
> > + */
> > + file = anon_inode_getfile_fmode("[vfio-device-liveupdate]",
> > + &vfio_device_fops, NULL,
> > + O_RDWR, FMODE_PREAD | FMODE_PWRITE);
> > + if (IS_ERR(file)) {
> > + ret = PTR_ERR(file);
> > + goto out;
> > + }
> > +
> > + ret = __vfio_device_fops_cdev_open(device, file);
> > + if (ret) {
> > + fput(file);
>
> Don't we end up calling vfio_device_fops.release with NULL
> file->private_data here with inevitable segfaults? Thanks,
Yes indeed. In that case I think we need to call
vfio_device_try_get_registration() and vfio_allocate_device_file()
before anon_inode_getfile_fmode(). We could play games with file->fops
to avoid it calling vfio_device_fops.release here instead, but that
feels hacky.
