[PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Paolo Bonzini]

This series is the first step towards fixing KVM's usage of follow_pfn.
The immediate fix here is that KVM is not checking the writability of
the PFN, which actually dates back to way before the introduction of
follow_pfn in commit add6a0cd1c5b ("KVM: MMU: try to fix up page faults
before giving up", 2016-07-05).  There are more changes needed to
invalidate gfn-to-pfn caches from MMU notifiers, but this issue will
be tackled later.

A more fundamental issue however is that the follow_pfn function is
basically impossible to use correctly.  Almost all users for example
are assuming that the page is writable; KVM was not alone in this
mistake.  follow_pte, despite not being exported for modules, is a
far saner API.  Therefore, patch 1 simplifies follow_pte a bit and
makes it available to modules.

Please review and possibly ack for inclusion in the KVM tree,
thanks!

Paolo


Paolo Bonzini (2):
  mm: provide a sane PTE walking API for modules
  KVM: do not assume PTE is writable after follow_pfn

 arch/s390/pci/pci_mmio.c |  2 +-
 fs/dax.c                 |  5 +++--
 include/linux/mm.h       |  6 ++++--
 mm/memory.c              | 35 ++++++++++++++++++++++++++++++-----
 virt/kvm/kvm_main.c      | 15 ++++++++++++---
 5 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.26.2

[PATCH 1/2] mm: provide a sane PTE walking API for modules

[Paolo Bonzini]

Currently, the follow_pfn function is exported for modules but
follow_pte is not.  However, follow_pfn is very easy to misuse,
because it does not provide protections (so most of its callers
assume the page is writable!) and because it returns after having
already unlocked the page table lock.

Provide instead a simplified version of follow_pte that does
not have the pmdpp and range arguments.  The older version
survives as follow_invalidate_pte() for use by fs/dax.c.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/s390/pci/pci_mmio.c |  2 +-
 fs/dax.c                 |  5 +++--
 include/linux/mm.h       |  6 ++++--
 mm/memory.c              | 35 ++++++++++++++++++++++++++++++-----
 4 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index 18f2d10c3176..5922dce94bfd 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -170,7 +170,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned long, mmio_addr,
 	if (!(vma->vm_flags & VM_WRITE))
 		goto out_unlock_mmap;
 
-	ret = follow_pte(vma->vm_mm, mmio_addr, NULL, &ptep, NULL, &ptl);
+	ret = follow_pte(vma->vm_mm, mmio_addr, &ptep, &ptl);
 	if (ret)
 		goto out_unlock_mmap;
 
@@ -311,7 +311,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long, mmio_addr,
 	if (!(vma->vm_flags & VM_WRITE))
 		goto out_unlock_mmap;
 
-	ret = follow_pte(vma->vm_mm, mmio_addr, NULL, &ptep, NULL, &ptl);
+	ret = follow_pte(vma->vm_mm, mmio_addr, &ptep, &ptl);
 	if (ret)
 		goto out_unlock_mmap;
 
diff --git a/fs/dax.c b/fs/dax.c
index 26d5dcd2d69e..b14874c7b983 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -810,11 +810,12 @@ static void dax_entry_mkclean(struct address_space *mapping, pgoff_t index,
 		address = pgoff_address(index, vma);
 
 		/*
-		 * Note because we provide range to follow_pte it will call
+		 * follow_invalidate_pte() will use the range to call
 		 * mmu_notifier_invalidate_range_start() on our behalf before
 		 * taking any lock.
 		 */
-		if (follow_pte(vma->vm_mm, address, &range, &ptep, &pmdp, &ptl))
+		if (follow_invalidate_pte(vma->vm_mm, address, &range, &ptep,
+					  &pmdp, &ptl))
 			continue;
 
 		/*
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..90b527260edf 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1658,9 +1658,11 @@ void free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
 		unsigned long end, unsigned long floor, unsigned long ceiling);
 int
 copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma);
+int follow_invalidate_pte(struct mm_struct *mm, unsigned long address,
+			  struct mmu_notifier_range *range, pte_t **ptepp, pmd_t **pmdpp,
+			  spinlock_t **ptlp);
 int follow_pte(struct mm_struct *mm, unsigned long address,
-		struct mmu_notifier_range *range, pte_t **ptepp, pmd_t **pmdpp,
-		spinlock_t **ptlp);
+	       pte_t **ptepp, spinlock_t **ptlp);
 int follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	unsigned long *pfn);
 int follow_phys(struct vm_area_struct *vma, unsigned long address,
diff --git a/mm/memory.c b/mm/memory.c
index feff48e1465a..b247d296931c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4709,9 +4709,9 @@ int __pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long address)
 }
 #endif /* __PAGETABLE_PMD_FOLDED */
 
-int follow_pte(struct mm_struct *mm, unsigned long address,
-	       struct mmu_notifier_range *range, pte_t **ptepp, pmd_t **pmdpp,
-	       spinlock_t **ptlp)
+int follow_invalidate_pte(struct mm_struct *mm, unsigned long address,
+			  struct mmu_notifier_range *range, pte_t **ptepp,
+			  pmd_t **pmdpp, spinlock_t **ptlp)
 {
 	pgd_t *pgd;
 	p4d_t *p4d;
@@ -4776,6 +4776,31 @@ int follow_pte(struct mm_struct *mm, unsigned long address,
 	return -EINVAL;
 }
 
+/**
+ * follow_pte - look up PTE at a user virtual address
+ * @vma: memory mapping
+ * @address: user virtual address
+ * @ptepp: location to store found PTE
+ * @ptlp: location to store the lock for the PTE
+ *
+ * On a successful return, the pointer to the PTE is stored in @ptepp;
+ * the corresponding lock is taken and its location is stored in @ptlp.
+ * The contents of the PTE are only stable until @ptlp is released;
+ * any further use, if any, must be protected against invalidation
+ * with MMU notifiers.
+ *
+ * Only IO mappings and raw PFN mappings are allowed.  The mmap semaphore
+ * should be taken for read.
+ *
+ * Return: zero on success, -ve otherwise.
+ */
+int follow_pte(struct mm_struct *mm, unsigned long address,
+	       pte_t **ptepp, spinlock_t **ptlp)
+{
+	return follow_invalidate_pte(mm, address, NULL, ptepp, NULL, ptlp);
+}
+EXPORT_SYMBOL_GPL(follow_pte);
+
 /**
  * follow_pfn - look up PFN at a user virtual address
  * @vma: memory mapping
@@ -4796,7 +4821,7 @@ int follow_pfn(struct vm_area_struct *vma, unsigned long address,
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
 		return ret;
 
-	ret = follow_pte(vma->vm_mm, address, NULL, &ptep, NULL, &ptl);
+	ret = follow_pte(vma->vm_mm, address, &ptep, &ptl);
 	if (ret)
 		return ret;
 	*pfn = pte_pfn(*ptep);
@@ -4817,7 +4842,7 @@ int follow_phys(struct vm_area_struct *vma,
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
 		goto out;
 
-	if (follow_pte(vma->vm_mm, address, NULL, &ptep, NULL, &ptl))
+	if (follow_pte(vma->vm_mm, address, &ptep, &ptl))
 		goto out;
 	pte = *ptep;
 
-- 
2.26.2

[PATCH 2/2] KVM: do not assume PTE is writable after follow_pfn

[Paolo Bonzini]

In order to convert an HVA to a PFN, KVM usually tries to use
the get_user_pages family of functions.  This however is not
possible for VM_IO vmas; in that case, KVM instead uses follow_pfn.

In doing this however KVM loses the information on whether the
PFN is writable.  That is usually not a problem because the main
use of VM_IO vmas with KVM is for BARs in PCI device assignment,
however it is a bug.  To fix it, use follow_pte and check pte_write
while under the protection of the PTE lock.  The information can
be used to fail hva_to_pfn_remapped or passed back to the
caller via *writable.

Usage of follow_pfn was introduced in commit add6a0cd1c5b ("KVM: MMU: try to fix
up page faults before giving up", 2016-07-05); however, even older version
have the same issue, all the way back to commit 2e2e3738af33 ("KVM:
Handle vma regions with no backing page", 2008-07-20), as they also did
not check whether the PFN was writable.

Fixes: 2e2e3738af33 ("KVM: Handle vma regions with no backing page")
Reported-by: David Stevens <stevensd@google.com>
Cc: 3pvd@google.com
Cc: Jann Horn <jannh@google.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 virt/kvm/kvm_main.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f34a85b93c2d..ee4ac2618ec5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1907,9 +1907,11 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 			       kvm_pfn_t *p_pfn)
 {
 	unsigned long pfn;
+	pte_t *ptep;
+	spinlock_t *ptl;
 	int r;
 
-	r = follow_pfn(vma, addr, &pfn);
+	r = follow_pte(vma->vm_mm, addr, &ptep, &ptl);
 	if (r) {
 		/*
 		 * get_user_pages fails for VM_IO and VM_PFNMAP vmas and does
@@ -1924,14 +1926,19 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 		if (r)
 			return r;
 
-		r = follow_pfn(vma, addr, &pfn);
+		r = follow_pte(vma->vm_mm, addr, &ptep, &ptl);
 		if (r)
 			return r;
+	}
 
+	if (write_fault && !pte_write(*ptep)) {
+		pfn = KVM_PFN_ERR_RO_FAULT;
+		goto out;
 	}
 
 	if (writable)
-		*writable = true;
+		*writable = pte_write(*ptep);
+	pfn = pte_pfn(*ptep);
 
 	/*
 	 * Get a reference here because callers of *hva_to_pfn* and
@@ -1946,6 +1953,8 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma,
 	 */ 
 	kvm_get_pfn(pfn);
 
+out:
+	pte_unmap_unlock(ptep, ptl);
 	*p_pfn = pfn;
 	return 0;
 }
-- 
2.26.2

Re: [PATCH 1/2] mm: provide a sane PTE walking API for modules

[Jason Gunthorpe]

On Fri, Feb 05, 2021 at 05:32:58AM -0500, Paolo Bonzini wrote:
> Currently, the follow_pfn function is exported for modules but
> follow_pte is not.  However, follow_pfn is very easy to misuse,
> because it does not provide protections (so most of its callers
> assume the page is writable!) and because it returns after having
> already unlocked the page table lock.
> 
> Provide instead a simplified version of follow_pte that does
> not have the pmdpp and range arguments.  The older version
> survives as follow_invalidate_pte() for use by fs/dax.c.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/s390/pci/pci_mmio.c |  2 +-
>  fs/dax.c                 |  5 +++--
>  include/linux/mm.h       |  6 ++++--
>  mm/memory.c              | 35 ++++++++++++++++++++++++++++++-----
>  4 files changed, 38 insertions(+), 10 deletions(-)

Looks good to me, thanks

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 2/2] KVM: do not assume PTE is writable after follow_pfn

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

Hi Paolo,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.11-rc6]
[cannot apply to kvm/linux-next hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Bonzini/KVM-do-not-assume-PTE-is-writable-after-follow_pfn/20210205-183434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2ab38c17aac10bf55ab3efde4c4db3893d8691d2
config: i386-randconfig-c004-20210205 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/c2a981899d009fddc691f2edf5ad54e9e5a57401
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Bonzini/KVM-do-not-assume-PTE-is-writable-after-follow_pfn/20210205-183434
        git checkout c2a981899d009fddc691f2edf5ad54e9e5a57401
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/x86/kvm/../../../virt/kvm/kvm_main.c:18:
   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'hva_to_pfn_remapped':
>> include/linux/kvm_host.h:89:30: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '9218868437227405314' to '2' [-Woverflow]
      89 | #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
         |                              ^
   arch/x86/kvm/../../../virt/kvm/kvm_main.c:1932:9: note: in expansion of macro 'KVM_PFN_ERR_RO_FAULT'
    1932 |   pfn = KVM_PFN_ERR_RO_FAULT;
         |         ^~~~~~~~~~~~~~~~~~~~


vim +89 include/linux/kvm_host.h

9c5b11728344e1 Xiao Guangrong 2012-08-03  86  
9c5b11728344e1 Xiao Guangrong 2012-08-03  87  #define KVM_PFN_ERR_FAULT	(KVM_PFN_ERR_MASK)
9c5b11728344e1 Xiao Guangrong 2012-08-03  88  #define KVM_PFN_ERR_HWPOISON	(KVM_PFN_ERR_MASK + 1)
81c52c56e2b435 Xiao Guangrong 2012-10-16 @89  #define KVM_PFN_ERR_RO_FAULT	(KVM_PFN_ERR_MASK + 2)
6c8ee57be9350c Xiao Guangrong 2012-08-03  90  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32689 bytes --]

Re: [PATCH 2/2] KVM: do not assume PTE is writable after follow_pfn

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

Hi Paolo,

I love your patch! Yet something to improve:

[auto build test ERROR on linux/master]
[also build test ERROR on linus/master v5.11-rc6 next-20210125]
[cannot apply to kvm/linux-next hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Paolo-Bonzini/KVM-do-not-assume-PTE-is-writable-after-follow_pfn/20210205-183434
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2ab38c17aac10bf55ab3efde4c4db3893d8691d2
config: i386-randconfig-a011-20210205 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/c2a981899d009fddc691f2edf5ad54e9e5a57401
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paolo-Bonzini/KVM-do-not-assume-PTE-is-writable-after-follow_pfn/20210205-183434
        git checkout c2a981899d009fddc691f2edf5ad54e9e5a57401
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/kvm/../../../virt/kvm/kvm_main.c:18:
   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'hva_to_pfn_remapped':
>> include/linux/kvm_host.h:89:30: error: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '9218868437227405314' to '2' [-Werror=overflow]
      89 | #define KVM_PFN_ERR_RO_FAULT (KVM_PFN_ERR_MASK + 2)
         |                              ^
   arch/x86/kvm/../../../virt/kvm/kvm_main.c:1932:9: note: in expansion of macro 'KVM_PFN_ERR_RO_FAULT'
    1932 |   pfn = KVM_PFN_ERR_RO_FAULT;
         |         ^~~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +89 include/linux/kvm_host.h

9c5b11728344e1 Xiao Guangrong 2012-08-03  86  
9c5b11728344e1 Xiao Guangrong 2012-08-03  87  #define KVM_PFN_ERR_FAULT	(KVM_PFN_ERR_MASK)
9c5b11728344e1 Xiao Guangrong 2012-08-03  88  #define KVM_PFN_ERR_HWPOISON	(KVM_PFN_ERR_MASK + 1)
81c52c56e2b435 Xiao Guangrong 2012-10-16 @89  #define KVM_PFN_ERR_RO_FAULT	(KVM_PFN_ERR_MASK + 2)
6c8ee57be9350c Xiao Guangrong 2012-08-03  90  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40584 bytes --]

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Peter Xu]

On Fri, Feb 05, 2021 at 05:32:57AM -0500, Paolo Bonzini wrote:
> This series is the first step towards fixing KVM's usage of follow_pfn.
> The immediate fix here is that KVM is not checking the writability of
> the PFN, which actually dates back to way before the introduction of
> follow_pfn in commit add6a0cd1c5b ("KVM: MMU: try to fix up page faults
> before giving up", 2016-07-05).  There are more changes needed to
> invalidate gfn-to-pfn caches from MMU notifiers, but this issue will
> be tackled later.
> 
> A more fundamental issue however is that the follow_pfn function is
> basically impossible to use correctly.  Almost all users for example
> are assuming that the page is writable; KVM was not alone in this
> mistake.  follow_pte, despite not being exported for modules, is a
> far saner API.  Therefore, patch 1 simplifies follow_pte a bit and
> makes it available to modules.
> 
> Please review and possibly ack for inclusion in the KVM tree,
> thanks!

FWIW, the patches look correct to me (if with patch 2 report fixed):

Reviewed-by: Peter Xu <peterx@redhat.com>

But I do have a question on why dax as the only user needs to pass in the
notifier to follow_pte() for initialization.

Indeed there're a difference on start/end init of the notifier depending on
whether it's a huge pmd but since there's the pmdp passed over too so I assume
the caller should know how to init the notifier anyways.

The thing is at least in current code we could send meaningless notifiers,
e.g., in follow_pte():

	if (range) {
		mmu_notifier_range_init(range, MMU_NOTIFY_CLEAR, 0, NULL, mm,
					address & PAGE_MASK,
					(address & PAGE_MASK) + PAGE_SIZE);
		mmu_notifier_invalidate_range_start(range);
	}
	ptep = pte_offset_map_lock(mm, pmd, address, ptlp);
	if (!pte_present(*ptep))
		goto unlock;
	*ptepp = ptep;
	return 0;
unlock:
	pte_unmap_unlock(ptep, *ptlp);
	if (range)
		mmu_notifier_invalidate_range_end(range);

The notify could be meaningless if we do the "goto unlock" path.

Ideally it seems we can move the notifier code to caller (as what most mmu
notifier users do) and we can also avoid doing that if follow_pte returned
-EINVAL.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] mm: provide a sane PTE walking API for modules

[Christoph Hellwig]

> +int follow_invalidate_pte(struct mm_struct *mm, unsigned long address,
> +			  struct mmu_notifier_range *range, pte_t **ptepp, pmd_t **pmdpp,
> +			  spinlock_t **ptlp);

This adds a very pointless overy long line.

> +/**
> + * follow_pte - look up PTE at a user virtual address
> + * @vma: memory mapping
> + * @address: user virtual address
> + * @ptepp: location to store found PTE
> + * @ptlp: location to store the lock for the PTE
> + *
> + * On a successful return, the pointer to the PTE is stored in @ptepp;
> + * the corresponding lock is taken and its location is stored in @ptlp.
> + * The contents of the PTE are only stable until @ptlp is released;
> + * any further use, if any, must be protected against invalidation
> + * with MMU notifiers.
> + *
> + * Only IO mappings and raw PFN mappings are allowed.  The mmap semaphore
> + * should be taken for read.
> + *
> + * Return: zero on success, -ve otherwise.
> + */
> +int follow_pte(struct mm_struct *mm, unsigned long address,
> +	       pte_t **ptepp, spinlock_t **ptlp)
> +{
> +	return follow_invalidate_pte(mm, address, NULL, ptepp, NULL, ptlp);
> +}
> +EXPORT_SYMBOL_GPL(follow_pte);

I still don't think this is good as a general API.  Please document this
as KVM only for now, and hopefully next merge window I'll finish an
export variant restricting us to specific modules.

Re: [PATCH 1/2] mm: provide a sane PTE walking API for modules

[Paolo Bonzini]

On 08/02/21 18:39, Christoph Hellwig wrote:
>> +int follow_pte(struct mm_struct *mm, unsigned long address,
>> +	       pte_t **ptepp, spinlock_t **ptlp)
>> +{
>> +	return follow_invalidate_pte(mm, address, NULL, ptepp, NULL, ptlp);
>> +}
>> +EXPORT_SYMBOL_GPL(follow_pte);
>
> I still don't think this is good as a general API.  Please document this
> as KVM only for now, and hopefully next merge window I'll finish an
> export variant restricting us to specific modules.

Fair enough.  I would expect that pretty much everyone using follow_pfn 
will at least want to switch to this one (as it's less bad and not 
impossible to use correctly), but I'll squash this in:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 90b527260edf..24b292fce8e5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1659,8 +1659,8 @@ void free_pgd_range(struct mmu_gather *tlb, 
unsigned long addr,
  int
  copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct 
*src_vma);
  int follow_invalidate_pte(struct mm_struct *mm, unsigned long address,
-			  struct mmu_notifier_range *range, pte_t **ptepp, pmd_t **pmdpp,
-			  spinlock_t **ptlp);
+			  struct mmu_notifier_range *range, pte_t **ptepp,
+			  pmd_t **pmdpp, spinlock_t **ptlp);
  int follow_pte(struct mm_struct *mm, unsigned long address,
  	       pte_t **ptepp, spinlock_t **ptlp);
  int follow_pfn(struct vm_area_struct *vma, unsigned long address,
diff --git a/mm/memory.c b/mm/memory.c
index 3632f7416248..c8679b15c004 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4792,6 +4792,9 @@ int follow_invalidate_pte(struct mm_struct *mm, 
unsigned long address,
   * Only IO mappings and raw PFN mappings are allowed.  The mmap semaphore
   * should be taken for read.
   *
+ * KVM uses this function.  While it is arguably less bad than
+ * ``follow_pfn``, it is not a good general-purpose API.
+ *
   * Return: zero on success, -ve otherwise.
   */
  int follow_pte(struct mm_struct *mm, unsigned long address,
@@ -4809,6 +4812,9 @@ EXPORT_SYMBOL_GPL(follow_pte);
   *
   * Only IO mappings and raw PFN mappings are allowed.
   *
+ * This function does not allow the caller to read the permissions
+ * of the PTE.  Do not use it.
+ *
   * Return: zero and the pfn at @pfn on success, -ve otherwise.
   */
  int follow_pfn(struct vm_area_struct *vma, unsigned long address,


(apologies in advance if Thunderbird destroys the patch).

Paolo

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Jason Gunthorpe]

On Fri, Feb 05, 2021 at 01:14:11PM -0500, Peter Xu wrote:

> But I do have a question on why dax as the only user needs to pass in the
> notifier to follow_pte() for initialization.

Not sure either, why does DAX opencode something very much like
page_mkclean() with dax_entry_mkclean()?

Also it looks like DAX uses the wrong notifier, it calls
MMU_NOTIFY_CLEAR but page_mkclean_one() uses
MMU_NOTIFY_PROTECTION_PAGE for the same PTE modification sequence??

page_mkclean() has some technique to make the notifier have the right
size without becoming entangled in the PTL locks..

Jason

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Peter Xu]

On Mon, Feb 08, 2021 at 02:51:33PM -0400, Jason Gunthorpe wrote:
> On Fri, Feb 05, 2021 at 01:14:11PM -0500, Peter Xu wrote:
> 
> > But I do have a question on why dax as the only user needs to pass in the
> > notifier to follow_pte() for initialization.
> 
> Not sure either, why does DAX opencode something very much like
> page_mkclean() with dax_entry_mkclean()?
> 
> Also it looks like DAX uses the wrong notifier, it calls
> MMU_NOTIFY_CLEAR but page_mkclean_one() uses
> MMU_NOTIFY_PROTECTION_PAGE for the same PTE modification sequence??
> 
> page_mkclean() has some technique to make the notifier have the right
> size without becoming entangled in the PTL locks..

Right.  I guess it's because dax doesn't have "struct page*" on the back, so it
can't directly use page_mkclean().  However the whole logic does look very
similar, so maybe they can be merged in some way.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Jason Gunthorpe]

On Mon, Feb 08, 2021 at 05:02:59PM -0500, Peter Xu wrote:
> On Mon, Feb 08, 2021 at 02:51:33PM -0400, Jason Gunthorpe wrote:
> > On Fri, Feb 05, 2021 at 01:14:11PM -0500, Peter Xu wrote:
> > 
> > > But I do have a question on why dax as the only user needs to pass in the
> > > notifier to follow_pte() for initialization.
> > 
> > Not sure either, why does DAX opencode something very much like
> > page_mkclean() with dax_entry_mkclean()?
> > 
> > Also it looks like DAX uses the wrong notifier, it calls
> > MMU_NOTIFY_CLEAR but page_mkclean_one() uses
> > MMU_NOTIFY_PROTECTION_PAGE for the same PTE modification sequence??
> > 
> > page_mkclean() has some technique to make the notifier have the right
> > size without becoming entangled in the PTL locks..
> 
> Right.  I guess it's because dax doesn't have "struct page*" on the
> back, so it

It doesn't? I thought DAX cases did?

Jason

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Peter Xu]

On Mon, Feb 08, 2021 at 07:26:25PM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 08, 2021 at 05:02:59PM -0500, Peter Xu wrote:
> > On Mon, Feb 08, 2021 at 02:51:33PM -0400, Jason Gunthorpe wrote:
> > > On Fri, Feb 05, 2021 at 01:14:11PM -0500, Peter Xu wrote:
> > > 
> > > > But I do have a question on why dax as the only user needs to pass in the
> > > > notifier to follow_pte() for initialization.
> > > 
> > > Not sure either, why does DAX opencode something very much like
> > > page_mkclean() with dax_entry_mkclean()?
> > > 
> > > Also it looks like DAX uses the wrong notifier, it calls
> > > MMU_NOTIFY_CLEAR but page_mkclean_one() uses
> > > MMU_NOTIFY_PROTECTION_PAGE for the same PTE modification sequence??
> > > 
> > > page_mkclean() has some technique to make the notifier have the right
> > > size without becoming entangled in the PTL locks..
> > 
> > Right.  I guess it's because dax doesn't have "struct page*" on the
> > back, so it
> 
> It doesn't? I thought DAX cases did?

I'm not familiar with dax at all.. but it seems so: e.g. dax_iomap_pte_fault()
looks like the general fault handler for dax mappings, in which there's calls
to things like vmf_insert_mixed_mkwrite() trying to install ptes with raw pfn.
Or I could also be missing something very important..  Thanks,

-- 
Peter Xu

Re: [PATCH 1/2] mm: provide a sane PTE walking API for modules

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 07:18:56PM +0100, Paolo Bonzini wrote:
> Fair enough.  I would expect that pretty much everyone using follow_pfn will
> at least want to switch to this one (as it's less bad and not impossible to
> use correctly), but I'll squash this in:


Daniel looked into them, so he may correct me, but the other follow_pfn
users and their destiny are:

 - SGX, which is not modular and I think I just saw a patch to kill them
 - v4l videobuf and frame vector: I think those are going away
   entirely as they implement a rather broken pre-dmabuf P2P scheme
 - vfio: should use MMU notifiers eventually

Daniel, what happened to your follow_pfn series?

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Christoph Hellwig]

On Mon, Feb 08, 2021 at 07:26:25PM -0400, Jason Gunthorpe wrote:
> > > page_mkclean() has some technique to make the notifier have the right
> > > size without becoming entangled in the PTL locks..
> > 
> > Right.  I guess it's because dax doesn't have "struct page*" on the
> > back, so it
> 
> It doesn't? I thought DAX cases did?

File system DAX has a struct page, device DAX does not.  Which means
everything using iomap should have a page available, but i'm adding
Dan as he should know the details :)

Re: [PATCH 1/2] mm: provide a sane PTE walking API for modules

[Paolo Bonzini]

On 09/02/21 09:14, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 07:18:56PM +0100, Paolo Bonzini wrote:
>> Fair enough.  I would expect that pretty much everyone using follow_pfn will
>> at least want to switch to this one (as it's less bad and not impossible to
>> use correctly), but I'll squash this in:
> 
> 
> Daniel looked into them, so he may correct me, but the other follow_pfn
> users and their destiny are:
> 
>   - SGX, which is not modular and I think I just saw a patch to kill them
>   - v4l videobuf and frame vector: I think those are going away
>     entirely as they implement a rather broken pre-dmabuf P2P scheme
>   - vfio: should use MMU notifiers eventually

Yes, I'm thinking mostly of vfio, which could use follow_pte as a 
short-term fix for just the missing permission check.

There's also s390 PCI, which is also not modular.

Paolo

> Daniel, what happened to your follow_pfn series?

Re: [PATCH 0/2] KVM: do not assume PTE is writable after follow_pfn

[Joao Martins]

On 2/9/21 8:19 AM, Christoph Hellwig wrote:
> On Mon, Feb 08, 2021 at 07:26:25PM -0400, Jason Gunthorpe wrote:
>>>> page_mkclean() has some technique to make the notifier have the right
>>>> size without becoming entangled in the PTL locks..
>>>
>>> Right.  I guess it's because dax doesn't have "struct page*" on the
>>> back, so it
>>
>> It doesn't? I thought DAX cases did?
> 
> File system DAX has a struct page, device DAX does not.  Which means
> everything using iomap should have a page available, but i'm adding
> Dan as he should know the details :)
> 
Both fsdax and device-dax have struct page. It's the pmem block device
that doesn't.