linux-kernel - Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

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: <20200508094213.0183c645@w520.home>
Date:   Fri, 8 May 2020 09:42:13 -0600
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Peter Xu <peterx@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, cohuck@...hat.com
Subject: Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

On Fri, 8 May 2020 12:05:40 -0300
Jason Gunthorpe <jgg@...pe.ca> wrote:

> On Fri, May 08, 2020 at 10:30:42AM -0400, Peter Xu wrote:
> > On Fri, May 08, 2020 at 09:10:13AM -0300, Jason Gunthorpe wrote:  
> > > On Thu, May 07, 2020 at 10:19:39PM -0400, Peter Xu wrote:  
> > > > On Thu, May 07, 2020 at 08:54:21PM -0300, Jason Gunthorpe wrote:  
> > > > > On Thu, May 07, 2020 at 05:24:43PM -0400, Peter Xu wrote:  
> > > > > > On Tue, May 05, 2020 at 03:54:44PM -0600, Alex Williamson wrote:  
> > > > > > > With conversion to follow_pfn(), DMA mapping a PFNMAP range depends on
> > > > > > > the range being faulted into the vma.  Add support to manually provide
> > > > > > > that, in the same way as done on KVM with hva_to_pfn_remapped().
> > > > > > > 
> > > > > > > Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> > > > > > >  drivers/vfio/vfio_iommu_type1.c |   36 +++++++++++++++++++++++++++++++++---
> > > > > > >  1 file changed, 33 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > > > > > > index cc1d64765ce7..4a4cb7cd86b2 100644
> > > > > > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > > > > > @@ -317,6 +317,32 @@ static int put_pfn(unsigned long pfn, int prot)
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int follow_fault_pfn(struct vm_area_struct *vma, struct mm_struct *mm,
> > > > > > > +			    unsigned long vaddr, unsigned long *pfn,
> > > > > > > +			    bool write_fault)
> > > > > > > +{
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = follow_pfn(vma, vaddr, pfn);
> > > > > > > +	if (ret) {
> > > > > > > +		bool unlocked = false;
> > > > > > > +
> > > > > > > +		ret = fixup_user_fault(NULL, mm, vaddr,
> > > > > > > +				       FAULT_FLAG_REMOTE |
> > > > > > > +				       (write_fault ?  FAULT_FLAG_WRITE : 0),
> > > > > > > +				       &unlocked);
> > > > > > > +		if (unlocked)
> > > > > > > +			return -EAGAIN;  
> > > > > > 
> > > > > > Hi, Alex,
> > > > > > 
> > > > > > IIUC this retry is not needed too because fixup_user_fault() will guarantee the
> > > > > > fault-in is done correctly with the valid PTE as long as ret==0, even if
> > > > > > unlocked==true.  
> > > > > 
> > > > > It is true, and today it is fine, but be careful when reworking this
> > > > > to use notifiers as unlocked also means things like the vma pointer
> > > > > are invalidated.  
> > > > 
> > > > Oh right, thanks for noticing that.  Then we should probably still keep the
> > > > retry logic... because otherwise the latter follow_pfn() could be referencing
> > > > an invalid vma already...  
> > > 
> > > I looked briefly and thought this flow used the vma only once?  
> > 
> >         ret = follow_pfn(vma, vaddr, pfn);
> >         if (ret) {
> >                 bool unlocked = false;
> >  
> >                 ret = fixup_user_fault(NULL, mm, vaddr,
> >                                        FAULT_FLAG_REMOTE |
> >                                        (write_fault ?  FAULT_FLAG_WRITE : 0),
> >                                        &unlocked);
> >                 if (unlocked)
> >                         return -EAGAIN;
> >  
> >                 if (ret)
> >                         return ret;
> >  
> >                 ret = follow_pfn(vma, vaddr, pfn);      <--------------- [1]
> >         }
> > 
> > So imo the 2nd follow_pfn() [1] could be racy if without the unlocked check.  
> 
> Ah yes, I didn't notice that, you can't touch vma here if unlocked is true.

Thanks for the discussion.  I gather then that this patch is correct as
written, which probably also mean the patch Peter linked for KVM should
not be applied since the logic is the same there.  Correct?  Thanks,

Alex