linux-kernel - Re: [PATCH] vfio: Reuse file f

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZnQBEmjEFoO39rRO@yzhao56-desk.sh.intel.com>
Date: Thu, 20 Jun 2024 18:14:42 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
CC: <alex.williamson@...hat.com>, <kevin.tian@...el.com>, <jgg@...dia.com>,
	<peterx@...hat.com>, <ajones@...tanamicro.com>
Subject: Re: [PATCH] vfio: Reuse file f_inode as vfio device inode

On Mon, Jun 17, 2024 at 05:53:32PM +0800, Yan Zhao wrote:
...
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index ded364588d29..aaef188003b6 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -268,31 +268,14 @@ static struct file *vfio_device_open_file(struct vfio_device *device)
>  	if (ret)
>  		goto err_free;
>  
> -	/*
> -	 * We can't use anon_inode_getfd() because we need to modify
> -	 * the f_mode flags directly to allow more than just ioctls
> -	 */
> -	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
> -				   df, O_RDWR);
> +	filep = vfio_device_get_pseudo_file(device);
If getting an inode from vfio_fs_type is not a must, maybe we could use
anon_inode_create_getfile() here?
Then changes to group.c and vfio_main.c can be simplified as below:

diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
index ded364588d29..7f2f7871403f 100644
--- a/drivers/vfio/group.c
+++ b/drivers/vfio/group.c
@@ -269,29 +269,22 @@ static struct file *vfio_device_open_file(struct vfio_device *device)
                goto err_free;

        /*
-        * We can't use anon_inode_getfd() because we need to modify
-        * the f_mode flags directly to allow more than just ioctls
+        * Get a unique inode from anon_inodefs
         */
-       filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
-                                  df, O_RDWR);
+       filep = anon_inode_create_getfile("[vfio-device]", &vfio_device_fops, df,
+                                         O_RDWR, NULL);
        if (IS_ERR(filep)) {
                ret = PTR_ERR(filep);
                goto err_close_device;
        }
-
-       /*
-        * TODO: add an anon_inode interface to do this.
-        * Appears to be missing by lack of need rather than
-        * explicitly prevented.  Now there's need.
-        */
        filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE);

        /*
-        * Use the pseudo fs inode on the device to link all mmaps
-        * to the same address space, allowing us to unmap all vmas
-        * associated to this device using unmap_mapping_range().
+        * mmaps are linked to the address space of the filep->f_inode.
+        * Save the inode in device->inode to allow unmap_mapping_range() to
+        * unmap all vmas.
         */
-       filep->f_mapping = device->inode->i_mapping;
+       device->inode = filep->f_inode;

        if (device->group->type == VFIO_NO_IOMMU)
                dev_warn(device->dev, "vfio-noiommu device opened by user "

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index a5a62d9d963f..c9dac788411b 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -192,8 +192,6 @@ static void vfio_device_release(struct device *dev)
        if (device->ops->release)
                device->ops->release(device);

-       iput(device->inode);
-       simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
        kvfree(device);
 }

@@ -248,22 +246,6 @@ static struct file_system_type vfio_fs_type = {
        .kill_sb = kill_anon_super,
 };

-static struct inode *vfio_fs_inode_new(void)
-{
-       struct inode *inode;
-       int ret;
-
-       ret = simple_pin_fs(&vfio_fs_type, &vfio.vfs_mount, &vfio.fs_count);
-       if (ret)
-               return ERR_PTR(ret);
-
-       inode = alloc_anon_inode(vfio.vfs_mount->mnt_sb);
-       if (IS_ERR(inode))
-               simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
-
-       return inode;
-}
-
 /*
  * Initialize a vfio_device so it can be registered to vfio core.
  */
@@ -282,11 +264,6 @@ static int vfio_init_device(struct vfio_device *device, struct device *dev,
        init_completion(&device->comp);
        device->dev = dev;
        device->ops = ops;
-       device->inode = vfio_fs_inode_new();
-       if (IS_ERR(device->inode)) {
-               ret = PTR_ERR(device->inode);
-               goto out_inode;
-       }

        if (ops->init) {
                ret = ops->init(device);
@@ -301,9 +278,6 @@ static int vfio_init_device(struct vfio_device *device, struct device *dev,
        return 0;

 out_uninit:
-       iput(device->inode);
-       simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
-out_inode:
        vfio_release_device_set(device);
        ida_free(&vfio.device_ida, device->index);
        return ret;


> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 50128da18bca..1f8915f79fbb 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -35,6 +35,7 @@ struct vfio_device_file *
>  vfio_allocate_device_file(struct vfio_device *device);
>  
>  extern const struct file_operations vfio_device_fops;
> +struct file *vfio_device_get_pseudo_file(struct vfio_device *device);
>  
>  #ifdef CONFIG_VFIO_NOIOMMU
>  extern bool vfio_noiommu __read_mostly;
> @@ -420,6 +421,7 @@ static inline void vfio_cdev_cleanup(void)
>  {
>  }
>  #endif /* CONFIG_VFIO_DEVICE_CDEV */
> +struct file *vfio_device_get_pseduo_file(struct vfio_device *device);
Sorry, this line was included by mistake.