| 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
| ||
|
Message-ID: <ZnvbRFcfcufERwr7@yzhao56-desk.sh.intel.com>
Date: Wed, 26 Jun 2024 17:11:32 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"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
On Wed, Jun 26, 2024 at 04:36:26PM +0800, Tian, Kevin wrote:
> > 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.
Right.
> >
> > 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?
I found it's confusing, as now an anon_inode is available but
"filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE)" still cannot be avoided.
To avoid it, the file needs be opened through do_dentry_open() interface,
which is not easily achieved.
> > 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.
Right. Thanks for catching it.
> >
> > -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