| Message ID | 158871568480.15589.17339878308143043906.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-pci: Block user access to disabled device MMIO | expand |
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 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ 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. Note: there's another patch just removed the similar retry in kvm: https://lore.kernel.org/kvm/20200416155906.267462-1-peterx@redhat.com/ Thanks, > + > + if (ret) > + return ret; > + > + ret = follow_pfn(vma, vaddr, pfn); > + } > + > + return ret; > +}
On Thu, 7 May 2020 17:24:43 -0400 Peter Xu <peterx@redhat.com> 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 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ 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. > > Note: there's another patch just removed the similar retry in kvm: > > https://lore.kernel.org/kvm/20200416155906.267462-1-peterx@redhat.com/ Great, I was basing this on that kvm code, so I can make essentially an identical fix. Thanks! Alex
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. Jason
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... Thanks,
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? Jason
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. Thanks,
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. Jason
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
On Fri, May 08, 2020 at 09:42:13AM -0600, Alex Williamson wrote: > 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, Right. I've already replied yesterday in that thread so Paolo unqueue that patch too. Thanks,
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> And since after the discussion... Reviewed-by: Peter Xu <peterx@redhat.com> Thanks,
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index cc1d64765ce7..4a4cb7cd86b2 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ 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; + + if (ret) + return ret; + + ret = follow_pfn(vma, vaddr, pfn); + } + + return ret; +} + static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, int prot, unsigned long *pfn) { @@ -339,12 +365,16 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, vaddr = untagged_addr(vaddr); +retry: vma = find_vma_intersection(mm, vaddr, vaddr + 1); if (vma && vma->vm_flags & VM_PFNMAP) { - if (!follow_pfn(vma, vaddr, pfn) && - is_invalid_reserved_pfn(*pfn)) - ret = 0; + ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE); + if (ret == -EAGAIN) + goto retry; + + if (!ret && !is_invalid_reserved_pfn(*pfn)) + ret = -EFAULT; } done: up_read(&mm->mmap_sem);
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(-)