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]

Date: Thu, 27 Jun 2024 09:26:34 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
Cc: "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>,
	"alex.williamson@...hat.com" <alex.williamson@...hat.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 11:55:59PM +0000, Tian, Kevin wrote:

> > > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > > index bb1817bd4ff3..a4eec8e88f5c 100644
> > > --- a/drivers/vfio/device_cdev.c
> > > +++ b/drivers/vfio/device_cdev.c
> > > @@ -40,12 +40,11 @@ int vfio_device_fops_cdev_open(struct inode
> > *inode, struct file *filep)
> > >  	filep->private_data = df;
> > >
> > >  	/*
> > > -	 * 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 inode of device cdev.
> > > +	 * Save the inode of device cdev in device->inode to allow
> > > +	 * unmap_mapping_range() to unmap all vmas.
> > >  	 */
> > > -	filep->f_mapping = device->inode->i_mapping;
> > > -
> > > +	device->inode = inode;
> > 
> > This doesn't seem right.. There is only one device but multiple file
> > can be opened on that device.
> > 
> > We expect every open file to have a unique inode otherwise the
> > unmap_mapping_range() will not function properly.
> 
> Does it mean that the existing code is already broken? there is only
> one vfio-fs inode per device (allocated at vfio_init_device()).

I may have gone too far, it is not that we expect evey FD to have a
unique inode, but we expect that there is only one active FD using the
mmap and only that inode will be invalidated.

So changing the inode above before we know that this is an active FD
that can do mmap isn't going to entirely work. ie someone could open
the cdev FD (and fail to make it active) while an active VFIO is using
the legacy path and clobber the invalidation inode.

Having per-FD inodes is just one answer (it is what my similar RDMA
patches were going to do), we could also move the above to the first
mmap so that the one and only active FD also sets the correct inode
for the invalidations.

Jason