| 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
| ||
|
Date: Wed, 26 Jun 2024 08:36:26 +0000
From: "Tian, Kevin" <kevin.tian@...el.com>
To: "Zhao, Yan Y" <yan.y.zhao@...el.com>, "kvm@...r.kernel.org"
<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
CC: "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"jgg@...dia.com" <jgg@...dia.com>, "peterx@...hat.com" <peterx@...hat.com>,
"ajones@...tanamicro.com" <ajones@...tanamicro.com>
Subject: RE: [PATCH] vfio: Reuse file f_inode as vfio device inode
> From: Zhao, Yan Y <yan.y.zhao@...el.com>
> Sent: Thursday, June 20, 2024 6:15 PM
>
> 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:
not familiar with file system, but at a glance the anon_inodefs is similar
to vfio's own pseudo fs so it might work. anyway what is required here
is to have an unique inode per vfio device to hold an unique address space
and anon_inode_create_getfile() appears to achieve it.
>
> 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.
> - */
why removing this comment?
> 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,
> };
then vfio_fs_type can be removed too.
>
> -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.
Powered by blists - more mailing lists