diff mbox series

[v1] fs/proc/task_mmu: Fix uffd-wp confusion in pagemap_scan_pmd_entry()

Message ID 20240429114104.182890-1-ryan.roberts@arm.com (mailing list archive)
State New, archived
Headers show
Series [v1] fs/proc/task_mmu: Fix uffd-wp confusion in pagemap_scan_pmd_entry() | expand

Commit Message

Ryan Roberts April 29, 2024, 11:41 a.m. UTC 
pagemap_scan_pmd_entry() checks if uffd-wp is set on each pte to avoid
unnecessary if set. However it was previously checking with
`pte_uffd_wp(ptep_get(pte))` without first confirming that the pte was
present. It is only valid to call pte_uffd_wp() for present ptes. For
swap ptes, pte_swp_uffd_wp() must be called because the uffd-wp bit may
be kept in a different position, depending on the arch.

This was leading to test failures in the pagemap_ioctl mm selftest, when
bringing up uffd-wp support on arm64 due to incorrectly interpretting
the uffd-wp status of migration entries.

Let's fix this by using the correct check based on pte_present(). While
we are at it, let's pass the pte to make_uffd_wp_pte() to avoid the
pointless extra ptep_get() which can't be optimized out due to
READ_ONCE() on many arches.

Closes: https://lore.kernel.org/linux-arm-kernel/ZiuyGXt0XWwRgFh9@x1n/
Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 fs/proc/task_mmu.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

--
2.25.1

Comments

David Hildenbrand April 29, 2024, 12:22 p.m. UTC | #1 
On 29.04.24 13:41, Ryan Roberts wrote:
> pagemap_scan_pmd_entry() checks if uffd-wp is set on each pte to avoid
> unnecessary if set. However it was previously checking with
> `pte_uffd_wp(ptep_get(pte))` without first confirming that the pte was
> present. It is only valid to call pte_uffd_wp() for present ptes. For
> swap ptes, pte_swp_uffd_wp() must be called because the uffd-wp bit may
> be kept in a different position, depending on the arch.
> 
> This was leading to test failures in the pagemap_ioctl mm selftest, when
> bringing up uffd-wp support on arm64 due to incorrectly interpretting
> the uffd-wp status of migration entries.
> 
> Let's fix this by using the correct check based on pte_present(). While
> we are at it, let's pass the pte to make_uffd_wp_pte() to avoid the
> pointless extra ptep_get() which can't be optimized out due to
> READ_ONCE() on many arches.
> 
> Closes: https://lore.kernel.org/linux-arm-kernel/ZiuyGXt0XWwRgFh9@x1n/
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>
Muhammad Usama Anjum April 29, 2024, 1:28 p.m. UTC | #2 
Thanks for finding and fixing Ryan!

On 4/29/24 4:41 PM, Ryan Roberts wrote:
> pagemap_scan_pmd_entry() checks if uffd-wp is set on each pte to avoid
> unnecessary if set. However it was previously checking with
> `pte_uffd_wp(ptep_get(pte))` without first confirming that the pte was
> present. It is only valid to call pte_uffd_wp() for present ptes. For
> swap ptes, pte_swp_uffd_wp() must be called because the uffd-wp bit may
> be kept in a different position, depending on the arch.
> 
> This was leading to test failures in the pagemap_ioctl mm selftest, when
> bringing up uffd-wp support on arm64 due to incorrectly interpretting
> the uffd-wp status of migration entries.
> 
> Let's fix this by using the correct check based on pte_present(). While
> we are at it, let's pass the pte to make_uffd_wp_pte() to avoid the
> pointless extra ptep_get() which can't be optimized out due to
> READ_ONCE() on many arches.
> 
> Closes: https://lore.kernel.org/linux-arm-kernel/ZiuyGXt0XWwRgFh9@x1n/
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> ---
>  fs/proc/task_mmu.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index af4bc1da0c01..102f48668c35 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1817,10 +1817,8 @@ static unsigned long pagemap_page_category(struct pagemap_scan_private *p,
>  }
> 
>  static void make_uffd_wp_pte(struct vm_area_struct *vma,
> -			     unsigned long addr, pte_t *pte)
> +			     unsigned long addr, pte_t *pte, pte_t ptent)
>  {
> -	pte_t ptent = ptep_get(pte);
> -
>  	if (pte_present(ptent)) {
>  		pte_t old_pte;
> 
> @@ -2175,9 +2173,12 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	if ((p->arg.flags & PM_SCAN_WP_MATCHING) && !p->vec_out) {
>  		/* Fast path for performing exclusive WP */
>  		for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> -			if (pte_uffd_wp(ptep_get(pte)))
> +			pte_t ptent = ptep_get(pte);
> +
> +			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> +			    pte_swp_uffd_wp_any(ptent))
>  				continue;
> -			make_uffd_wp_pte(vma, addr, pte);
> +			make_uffd_wp_pte(vma, addr, pte, ptent);
>  			if (!flush_end)
>  				start = addr;
>  			flush_end = addr + PAGE_SIZE;
> @@ -2190,8 +2191,10 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	    p->arg.return_mask == PAGE_IS_WRITTEN) {
>  		for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
>  			unsigned long next = addr + PAGE_SIZE;
> +			pte_t ptent = ptep_get(pte);
> 
> -			if (pte_uffd_wp(ptep_get(pte)))
> +			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> +			    pte_swp_uffd_wp_any(ptent))
>  				continue;
>  			ret = pagemap_scan_output(p->cur_vma_category | PAGE_IS_WRITTEN,
>  						  p, addr, &next);
> @@ -2199,7 +2202,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  				break;
>  			if (~p->arg.flags & PM_SCAN_WP_MATCHING)
>  				continue;
> -			make_uffd_wp_pte(vma, addr, pte);
> +			make_uffd_wp_pte(vma, addr, pte, ptent);
>  			if (!flush_end)
>  				start = addr;
>  			flush_end = next;
> @@ -2208,8 +2211,9 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	}
> 
>  	for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> +		pte_t ptent = ptep_get(pte);
>  		unsigned long categories = p->cur_vma_category |
> -					   pagemap_page_category(p, vma, addr, ptep_get(pte));
> +					   pagemap_page_category(p, vma, addr, ptent);
>  		unsigned long next = addr + PAGE_SIZE;
> 
>  		if (!pagemap_scan_is_interesting_page(categories, p))
> @@ -2224,7 +2228,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  		if (~categories & PAGE_IS_WRITTEN)
>  			continue;
> 
> -		make_uffd_wp_pte(vma, addr, pte);
> +		make_uffd_wp_pte(vma, addr, pte, ptent);
>  		if (!flush_end)
>  			start = addr;
>  		flush_end = next;
> --
> 2.25.1
>
Ryan Roberts April 29, 2024, 3:55 p.m. UTC | #3 
On 29/04/2024 12:41, Ryan Roberts wrote:
> pagemap_scan_pmd_entry() checks if uffd-wp is set on each pte to avoid
> unnecessary if set. However it was previously checking with
> `pte_uffd_wp(ptep_get(pte))` without first confirming that the pte was
> present. It is only valid to call pte_uffd_wp() for present ptes. For
> swap ptes, pte_swp_uffd_wp() must be called because the uffd-wp bit may
> be kept in a different position, depending on the arch.
> 
> This was leading to test failures in the pagemap_ioctl mm selftest, when
> bringing up uffd-wp support on arm64 due to incorrectly interpretting
> the uffd-wp status of migration entries.
> 
> Let's fix this by using the correct check based on pte_present(). While
> we are at it, let's pass the pte to make_uffd_wp_pte() to avoid the
> pointless extra ptep_get() which can't be optimized out due to
> READ_ONCE() on many arches.
> 
> Closes: https://lore.kernel.org/linux-arm-kernel/ZiuyGXt0XWwRgFh9@x1n/
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

I guess this should have cc'ed stable but I forgot to add it. Are you able to
fix this up when you take it, Andrew, or do I need to repost?

> ---
>  fs/proc/task_mmu.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index af4bc1da0c01..102f48668c35 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1817,10 +1817,8 @@ static unsigned long pagemap_page_category(struct pagemap_scan_private *p,
>  }
> 
>  static void make_uffd_wp_pte(struct vm_area_struct *vma,
> -			     unsigned long addr, pte_t *pte)
> +			     unsigned long addr, pte_t *pte, pte_t ptent)
>  {
> -	pte_t ptent = ptep_get(pte);
> -
>  	if (pte_present(ptent)) {
>  		pte_t old_pte;
> 
> @@ -2175,9 +2173,12 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	if ((p->arg.flags & PM_SCAN_WP_MATCHING) && !p->vec_out) {
>  		/* Fast path for performing exclusive WP */
>  		for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> -			if (pte_uffd_wp(ptep_get(pte)))
> +			pte_t ptent = ptep_get(pte);
> +
> +			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> +			    pte_swp_uffd_wp_any(ptent))
>  				continue;
> -			make_uffd_wp_pte(vma, addr, pte);
> +			make_uffd_wp_pte(vma, addr, pte, ptent);
>  			if (!flush_end)
>  				start = addr;
>  			flush_end = addr + PAGE_SIZE;
> @@ -2190,8 +2191,10 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	    p->arg.return_mask == PAGE_IS_WRITTEN) {
>  		for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
>  			unsigned long next = addr + PAGE_SIZE;
> +			pte_t ptent = ptep_get(pte);
> 
> -			if (pte_uffd_wp(ptep_get(pte)))
> +			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
> +			    pte_swp_uffd_wp_any(ptent))
>  				continue;
>  			ret = pagemap_scan_output(p->cur_vma_category | PAGE_IS_WRITTEN,
>  						  p, addr, &next);
> @@ -2199,7 +2202,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  				break;
>  			if (~p->arg.flags & PM_SCAN_WP_MATCHING)
>  				continue;
> -			make_uffd_wp_pte(vma, addr, pte);
> +			make_uffd_wp_pte(vma, addr, pte, ptent);
>  			if (!flush_end)
>  				start = addr;
>  			flush_end = next;
> @@ -2208,8 +2211,9 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  	}
> 
>  	for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
> +		pte_t ptent = ptep_get(pte);
>  		unsigned long categories = p->cur_vma_category |
> -					   pagemap_page_category(p, vma, addr, ptep_get(pte));
> +					   pagemap_page_category(p, vma, addr, ptent);
>  		unsigned long next = addr + PAGE_SIZE;
> 
>  		if (!pagemap_scan_is_interesting_page(categories, p))
> @@ -2224,7 +2228,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>  		if (~categories & PAGE_IS_WRITTEN)
>  			continue;
> 
> -		make_uffd_wp_pte(vma, addr, pte);
> +		make_uffd_wp_pte(vma, addr, pte, ptent);
>  		if (!flush_end)
>  			start = addr;
>  		flush_end = next;
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index af4bc1da0c01..102f48668c35 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1817,10 +1817,8 @@  static unsigned long pagemap_page_category(struct pagemap_scan_private *p,
 }

 static void make_uffd_wp_pte(struct vm_area_struct *vma,
-			     unsigned long addr, pte_t *pte)
+			     unsigned long addr, pte_t *pte, pte_t ptent)
 {
-	pte_t ptent = ptep_get(pte);
-
 	if (pte_present(ptent)) {
 		pte_t old_pte;

@@ -2175,9 +2173,12 @@  static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 	if ((p->arg.flags & PM_SCAN_WP_MATCHING) && !p->vec_out) {
 		/* Fast path for performing exclusive WP */
 		for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
-			if (pte_uffd_wp(ptep_get(pte)))
+			pte_t ptent = ptep_get(pte);
+
+			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
+			    pte_swp_uffd_wp_any(ptent))
 				continue;
-			make_uffd_wp_pte(vma, addr, pte);
+			make_uffd_wp_pte(vma, addr, pte, ptent);
 			if (!flush_end)
 				start = addr;
 			flush_end = addr + PAGE_SIZE;
@@ -2190,8 +2191,10 @@  static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 	    p->arg.return_mask == PAGE_IS_WRITTEN) {
 		for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
 			unsigned long next = addr + PAGE_SIZE;
+			pte_t ptent = ptep_get(pte);

-			if (pte_uffd_wp(ptep_get(pte)))
+			if ((pte_present(ptent) && pte_uffd_wp(ptent)) ||
+			    pte_swp_uffd_wp_any(ptent))
 				continue;
 			ret = pagemap_scan_output(p->cur_vma_category | PAGE_IS_WRITTEN,
 						  p, addr, &next);
@@ -2199,7 +2202,7 @@  static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 				break;
 			if (~p->arg.flags & PM_SCAN_WP_MATCHING)
 				continue;
-			make_uffd_wp_pte(vma, addr, pte);
+			make_uffd_wp_pte(vma, addr, pte, ptent);
 			if (!flush_end)
 				start = addr;
 			flush_end = next;
@@ -2208,8 +2211,9 @@  static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 	}

 	for (addr = start; addr != end; pte++, addr += PAGE_SIZE) {
+		pte_t ptent = ptep_get(pte);
 		unsigned long categories = p->cur_vma_category |
-					   pagemap_page_category(p, vma, addr, ptep_get(pte));
+					   pagemap_page_category(p, vma, addr, ptent);
 		unsigned long next = addr + PAGE_SIZE;

 		if (!pagemap_scan_is_interesting_page(categories, p))
@@ -2224,7 +2228,7 @@  static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 		if (~categories & PAGE_IS_WRITTEN)
 			continue;

-		make_uffd_wp_pte(vma, addr, pte);
+		make_uffd_wp_pte(vma, addr, pte, ptent);
 		if (!flush_end)
 			start = addr;
 		flush_end = next;