netdev - RE: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system

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, 13 Nov 2014 07:04:35 +0000
From:	Sathya Perla <Sathya.Perla@...lex.Com>
To:	Alex Williamson <alex.williamson@...hat.com>,
	Don Dutile <ddutile@...hat.com>
CC:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"ariel.elior@...gic.com" <ariel.elior@...gic.com>,
	"linux.nics@...el.com" <linux.nics@...el.com>,
	"shahed.shaikh@...gic.com" <shahed.shaikh@...gic.com>
Subject: RE: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs)
 to pci sub-system

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@...hat.com]
> 
> On Tue, 2014-11-11 at 14:09 -0500, Don Dutile wrote:
> > On 11/10/2014 06:53 AM, Sathya Perla wrote:
> > > A user must not be allowed to disable VFs while they are already assigned
> to
> > > a guest. This check is being made in each individual driver that
> implements
> > > the sriov_configure PCI method.
> > > This patch-set fixes this code duplication by moving this check from
> > > drivers to the sriov_nuvfs_store() routine just before invoking
> > > sriov_configure() when num_vfs is equal to 0.
> > >
> > > Vasundhara Volam (4):
> > >    pci: move pci_assivned_vfs() check while disabling VFs to pci
> > >      sub-system
> > >    bnx2x: remove pci_assigned_vfs() check while disabling VFs
> > >    i40e: remove pci_assigned_vfs() check while disabling VFs
> > >    qlcnic: remove pci_assigned_vfs() check while disabling VFs
> > >
> > >   drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
> > >   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
> > >   .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
> > >   drivers/pci/pci-sysfs.c                            |    5 +++++
> > >   4 files changed, 7 insertions(+), 17 deletions(-)
> > >
> > I have had a side conversation with Alex Williamson, VFIO author.
> >
> > VFIO is the upstream method that device-assignment is managed/handled
> on kvm now.
> > It does not set the PCI_DEV_FLAGS_ASSIGNED pci dev-flags, and thus,
> > this check will not work when VFIO is used.
> >
> > This patch set will only work for the former, kvm-managed, device-
> assignment method,
> > which is currently being deprecated in qemu as well.
> >
> > So, yes, it works for kvm managed device-assignment, but not the
> > newer, VFIO-based device-assignment.
> >
> > Note, also, that the pci_assigned_vfs() check in the drivers will
> > always return 0 when VFIO is used for device assignment, so keeping
> > these checks in the drivers doesn't do what they imply either.
> >
> > So, taking in the patch solves old, kvm-managed, device assignment,
> > but a new method is needed when VFIO is involved.
> >
> > - Don
> >
> > ps -- Note: just adding the flag setting in vfio-pci does not necessarily
> >        solve this problem.  VFIO does not know if a device is assigned to a
> guest;
> >        it only knows a caller of the ioctl requesting the device to be assigned
> >        to vfio, and to be dma-mapped for a region of memory, has been
> requested.
> >        So, a new PF<->VF mechanism needs to be put in place to
> >        determine the equivalent information.
> 
> pps -- Note: testing pci_assivned_vfs() is racy, nothing prevents the flag
> 	being added to a device between your check and removing the VF
> device.
> 	This is one of the reasons that vfio-pci doesn't use it and that this
> 	interface should be discouraged in the kernel.

Alex/Don, I agree with the points you've raised.
But, I'd like to know whether you think this patch-set should be accepted or not.
Even though this patch-set doesn't fix any of the pending issues raised here,
it's a small step forward as it reduces the number of invocations of pci_assigned_vfs()
check which is a good thing.

thanks,
-Sathya