kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Peter Xu <peterx@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	cohuck@redhat.com
Subject: Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas
Date: Fri, 8 May 2020 09:42:13 -0600	[thread overview]
Message-ID: <20200508094213.0183c645@w520.home> (raw)
In-Reply-To: <20200508150540.GP26002@ziepe.ca>

On Fri, 8 May 2020 12:05:40 -0300
Jason Gunthorpe <jgg@ziepe.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@redhat.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


  reply	other threads:[~2020-05-08 15:42 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-05 21:54 [PATCH v2 0/3] vfio-pci: Block user access to disabled device MMIO Alex Williamson
2020-05-05 21:54 ` [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas Alex Williamson
2020-05-07 21:24   ` Peter Xu
2020-05-07 21:47     ` Alex Williamson
2020-05-07 23:54     ` Jason Gunthorpe
2020-05-08  2:19       ` Peter Xu
2020-05-08 12:10         ` Jason Gunthorpe
2020-05-08 14:30           ` Peter Xu
2020-05-08 15:05             ` Jason Gunthorpe
2020-05-08 15:42               ` Alex Williamson [this message]
2020-05-08 16:05                 ` Peter Xu
2020-05-08 18:39   ` Peter Xu
2020-05-05 21:54 ` [PATCH v2 2/3] vfio-pci: Fault mmaps to enable vma tracking Alex Williamson
2020-05-07 21:47   ` Peter Xu
2020-05-07 22:03     ` Alex Williamson
2020-05-07 22:22       ` Peter Xu
2020-05-07 23:56         ` Jason Gunthorpe
2020-05-08  2:16           ` Peter Xu
2020-05-08  6:44             ` Jason Wang
2020-05-08 14:27               ` Peter Xu
2020-05-08 12:08             ` Jason Gunthorpe
2020-05-08 14:26               ` Peter Xu
2020-05-05 21:55 ` [PATCH v2 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory Alex Williamson
2020-05-22  2:39   ` Qian Cai
2020-05-22  4:18     ` Alex Williamson
2020-05-07 21:59 ` [PATCH v2 0/3] vfio-pci: Block user access to disabled device MMIO Peter Xu
2020-05-07 22:34   ` Alex Williamson
2020-05-08  2:31     ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200508094213.0183c645@w520.home \
    --to=alex.williamson@redhat.com \
    --cc=cohuck@redhat.com \
    --cc=jgg@ziepe.ca \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterx@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).