linux-kernel - Re: [PATCH] vfio: add a singleton check for vfio_group_pin

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: <20200915133011.1e3652ee@x1.home>
Date:   Tue, 15 Sep 2020 13:30:11 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Yan Zhao <yan.y.zhao@...el.com>
Cc:     cohuck@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfio: add a singleton check for vfio_group_pin_pages

On Tue, 15 Sep 2020 13:03:01 -0600
Alex Williamson <alex.williamson@...hat.com> wrote:

> On Tue, 15 Sep 2020 08:30:42 +0800
> Yan Zhao <yan.y.zhao@...el.com> wrote:
> 
> > vfio_pin_pages() and vfio_group_pin_pages() are used purely to mark dirty
> > pages to devices with IOMMU backend as they already have all VM pages
> > pinned at VM startup.  
> 
> This is wrong.  The entire initial basis of mdev devices is for
> non-IOMMU backed devices which provide mediation outside of the scope
> of the IOMMU.  That mediation includes interpreting device DMA
> programming and making use of the vfio_pin_pages() interface to
> translate and pin IOVA address to HPA.  Marking pages dirty is a
> secondary feature.
> 
> > when there're multiple devices in the vfio group, the dirty pages
> > marked through pin_pages interface by one device is not useful as the
> > other devices may access and dirty any VM pages.  
> 
> I don't know of any cases where there are multiple devices in a group
> that would make use of this interface, however, all devices within a
> group necessarily share an IOMMU context and any one device dirtying a
> page will dirty that page for all devices, so I don't see that this is
> a valid statement either.
> 
> > So added a check such that only singleton IOMMU groups can pin pages
> > in vfio_group_pin_pages. for mdevs, there's always only one dev in a
> > vfio group.
> > This is a fix to the commit below that added a singleton IOMMU group
> > check in vfio_pin_pages.  
> 
> None of the justification above is accurate, please try again.  Thanks,

FWIW, I think this should read something like "Page pinning is used
both to translate and pin device mappings for DMA purpose, as well as
to indicate to the IOMMU backend to limit the dirty page scope to those
pages that have been pinned, in the case of an IOMMU backed device.  To
support this, the vfio_pin_pages() interface limits itself to only
singleton groups such that the IOMMU backend can consider dirty page
scope only at the group level.  Implement the same requirement for the
vfio_group_pin_pages() interface."  Thanks,

Alex


> > Fixes: 95fc87b44104 (vfio: Selective dirty page tracking if IOMMU backed
> > device pins pages)
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> > ---
> >  drivers/vfio/vfio.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 5e6e0511b5aa..2f0fa272ebf2 100644
> > --- a/drivers/vfio/vfio.c
> > +++ b/drivers/vfio/vfio.c
> > @@ -2053,6 +2053,9 @@ int vfio_group_pin_pages(struct vfio_group *group,
> >  	if (!group || !user_iova_pfn || !phys_pfn || !npage)
> >  		return -EINVAL;
> >  
> > +	if (group->dev_counter > 1)
> > +		return  -EINVAL;
> > +
> >  	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
> >  		return -E2BIG;
> >    
>