[PATCH 0/3] mm/pagemap: A few fixes to the recent PAGEMAP_SCAN

[PATCH 0/3] mm/pagemap: A few fixes to the recent PAGEMAP_SCAN

[Peter Xu]

Muhammad: I'd rather leave these to you, but since I already started
looking into it, and you didn't yet start replying, let me try to do it.
Please still review if you can catch the train.

This series should fix two known reports from syzbot on the new
PAGEMAP_SCAN ioctl():

https://lore.kernel.org/all/000000000000b0e576060a30ee3b@google.com/
https://lore.kernel.org/all/000000000000773fa7060a31e2cc@google.com/

The 3rd patch is something I found when testing these patches.

Thanks,

Peter Xu (3):
  mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check
  mm/pagemap: Fix wr-protect even if PM_SCAN_WP_MATCHING not set
  mm/selftests: Fix pagemap_ioctl memory map test

 fs/proc/task_mmu.c                         | 26 +++++++++++++++++-----
 tools/testing/selftests/mm/pagemap_ioctl.c |  9 +++++---
 2 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH 1/3] mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check

[Peter Xu]

The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
userfault, however in the vma test it didn't explicitly require the vma to
have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.

It means the pagemap code can now apply uffd-wp bit to a page in the vma
even if not registered to userfaultfd at all.

Then in whatever way as long as the pte got written and page fault
resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
up the pte for uffd-wp bit will trigger the warning:

WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]

Fix it by doing proper check over the vma attributes when
PM_SCAN_WP_MATCHING is specified.

Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 51e0ec658457..e91085d79926 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1994,15 +1994,31 @@ static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
 	struct pagemap_scan_private *p = walk->private;
 	struct vm_area_struct *vma = walk->vma;
 	unsigned long vma_category = 0;
+	bool wp_allowed = userfaultfd_wp_async(vma) &&
+	    userfaultfd_wp_use_markers(vma);
 
-	if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
-		vma_category |= PAGE_IS_WPALLOWED;
-	else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
-		return -EPERM;
+	if (!wp_allowed) {
+		/* User requested explicit failure over wp-async capability */
+		if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
+			return -EPERM;
+		/*
+		 * User requires wr-protect, and allows silently skipping
+		 * unsupported vmas.
+		 */
+		if (p->arg.flags & PM_SCAN_WP_MATCHING)
+			return 1;
+		/*
+		 * Then the request doesn't involve wr-protects at all,
+		 * fall through to the rest checks, and allow vma walk.
+		 */
+	}
 
 	if (vma->vm_flags & VM_PFNMAP)
 		return 1;
 
+	if (wp_allowed)
+		vma_category |= PAGE_IS_WPALLOWED;
+
 	if (vma->vm_flags & VM_SOFTDIRTY)
 		vma_category |= PAGE_IS_SOFT_DIRTY;
 
-- 
2.41.0

Re: [PATCH 1/3] mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check

[David Hildenbrand]

On 16.11.23 21:15, Peter Xu wrote:
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
> 
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
> 
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
> 
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
> 
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
> 
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/3] mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check

[Andrei Vagin]

On Thu, Nov 16, 2023 at 12:15 PM Peter Xu <peterx@redhat.com> wrote:
>
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
>
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
>
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
>
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
>
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
>
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Andrei Vagin <avagin@gmail.com>

> ---
>  fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 51e0ec658457..e91085d79926 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1994,15 +1994,31 @@ static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
>         struct pagemap_scan_private *p = walk->private;
>         struct vm_area_struct *vma = walk->vma;
>         unsigned long vma_category = 0;
> +       bool wp_allowed = userfaultfd_wp_async(vma) &&
> +           userfaultfd_wp_use_markers(vma);
>
> -       if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
> -               vma_category |= PAGE_IS_WPALLOWED;
> -       else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> -               return -EPERM;
> +       if (!wp_allowed) {
> +               /* User requested explicit failure over wp-async capability */
> +               if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> +                       return -EPERM;
> +               /*
> +                * User requires wr-protect, and allows silently skipping
> +                * unsupported vmas.
> +                */
> +               if (p->arg.flags & PM_SCAN_WP_MATCHING)
> +                       return 1;
> +               /*
> +                * Then the request doesn't involve wr-protects at all,
> +                * fall through to the rest checks, and allow vma walk.
> +                */
> +       }
>
>         if (vma->vm_flags & VM_PFNMAP)
>                 return 1;
>
> +       if (wp_allowed)
> +               vma_category |= PAGE_IS_WPALLOWED;
> +
>         if (vma->vm_flags & VM_SOFTDIRTY)
>                 vma_category |= PAGE_IS_SOFT_DIRTY;
>
> --
> 2.41.0
>

Re: [PATCH 1/3] mm/pagemap: Fix ioctl(PAGEMAP_SCAN) on vma check

[Muhammad Usama Anjum]

Hi Peter,

Thank you for taking care of it. I'm on holidays after LPC.

On 11/16/23 3:15 PM, Peter Xu wrote:
> The new ioctl(PAGEMAP_SCAN) relies on vma wr-protect capability provided by
> userfault, however in the vma test it didn't explicitly require the vma to
> have wr-protect function enabled, even if PM_SCAN_WP_MATCHING flag is set.
> 
> It means the pagemap code can now apply uffd-wp bit to a page in the vma
> even if not registered to userfaultfd at all.
> 
> Then in whatever way as long as the pte got written and page fault
> resolved, we'll apply the write bit even if uffd-wp bit is set.  We'll see
> a pte that has both UFFD_WP and WRITE bit set.  Anything later that looks
> up the pte for uffd-wp bit will trigger the warning:
> 
> WARNING: CPU: 1 PID: 5071 at arch/x86/include/asm/pgtable.h:403 pte_uffd_wp arch/x86/include/asm/pgtable.h:403 [inline]
> 
> Fix it by doing proper check over the vma attributes when
> PM_SCAN_WP_MATCHING is specified.
> 
> Fixes: 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs")
> Reported-by: syzbot+e94c5aaf7890901ebf9b@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  fs/proc/task_mmu.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 51e0ec658457..e91085d79926 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1994,15 +1994,31 @@ static int pagemap_scan_test_walk(unsigned long start, unsigned long end,
>  	struct pagemap_scan_private *p = walk->private;
>  	struct vm_area_struct *vma = walk->vma;
>  	unsigned long vma_category = 0;
> +	bool wp_allowed = userfaultfd_wp_async(vma) &&
> +	    userfaultfd_wp_use_markers(vma);
>  
> -	if (userfaultfd_wp_async(vma) && userfaultfd_wp_use_markers(vma))
> -		vma_category |= PAGE_IS_WPALLOWED;
> -	else if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> -		return -EPERM;
> +	if (!wp_allowed) {
> +		/* User requested explicit failure over wp-async capability */
> +		if (p->arg.flags & PM_SCAN_CHECK_WPASYNC)
> +			return -EPERM;
> +		/*
> +		 * User requires wr-protect, and allows silently skipping
> +		 * unsupported vmas.
> +		 */
> +		if (p->arg.flags & PM_SCAN_WP_MATCHING)
> +			return 1;
> +		/*
> +		 * Then the request doesn't involve wr-protects at all,
> +		 * fall through to the rest checks, and allow vma walk.
> +		 */
> +	}
Very simply done. I've really liked it.

Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

>  
>  	if (vma->vm_flags & VM_PFNMAP)
>  		return 1;
>  
> +	if (wp_allowed)
> +		vma_category |= PAGE_IS_WPALLOWED;
> +
>  	if (vma->vm_flags & VM_SOFTDIRTY)
>  		vma_category |= PAGE_IS_SOFT_DIRTY;
>  

-- 
BR,
Muhammad Usama Anjum

[PATCH 2/3] mm/pagemap: Fix wr-protect even if PM_SCAN_WP_MATCHING not set

[Peter Xu]

The new pagemap ioctl contains a fast path for wr-protections without
looking into category masks.  It forgets to check PM_SCAN_WP_MATCHING
before applying the wr-protections.  It can cause, e.g., pte markers
installed on archs that do not even support uffd wr-protect.

WARNING: CPU: 0 PID: 5059 at mm/memory.c:1520 zap_pte_range mm/memory.c:1520 [inline]

Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
Reported-by: syzbot+7ca4b2719dc742b8d0a4@syzkaller.appspotmail.com
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 fs/proc/task_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e91085d79926..d19924bf0a39 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -2171,7 +2171,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
 		return 0;
 	}
 
-	if (!p->vec_out) {
+	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)))
-- 
2.41.0

Re: [PATCH 2/3] mm/pagemap: Fix wr-protect even if PM_SCAN_WP_MATCHING not set

[David Hildenbrand]

On 16.11.23 21:15, Peter Xu wrote:
> The new pagemap ioctl contains a fast path for wr-protections without
> looking into category masks.  It forgets to check PM_SCAN_WP_MATCHING
> before applying the wr-protections.  It can cause, e.g., pte markers
> installed on archs that do not even support uffd wr-protect.
> 
> WARNING: CPU: 0 PID: 5059 at mm/memory.c:1520 zap_pte_range mm/memory.c:1520 [inline]
> 
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Reported-by: syzbot+7ca4b2719dc742b8d0a4@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   fs/proc/task_mmu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e91085d79926..d19924bf0a39 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -2171,7 +2171,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>   		return 0;
>   	}
>   
> -	if (!p->vec_out) {
> +	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)))

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/3] mm/pagemap: Fix wr-protect even if PM_SCAN_WP_MATCHING not set

[Andrei Vagin]

On Thu, Nov 16, 2023 at 12:15 PM Peter Xu <peterx@redhat.com> wrote:
>
> The new pagemap ioctl contains a fast path for wr-protections without
> looking into category masks.  It forgets to check PM_SCAN_WP_MATCHING
> before applying the wr-protections.  It can cause, e.g., pte markers
> installed on archs that do not even support uffd wr-protect.
>
> WARNING: CPU: 0 PID: 5059 at mm/memory.c:1520 zap_pte_range mm/memory.c:1520 [inline]
>
> Fixes: 12f6b01a0bcb ("fs/proc/task_mmu: add fast paths to get/clear PAGE_IS_WRITTEN flag")
> Reported-by: syzbot+7ca4b2719dc742b8d0a4@syzkaller.appspotmail.com
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Andrei Vagin <avagin@gmail.com>

> ---
>  fs/proc/task_mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e91085d79926..d19924bf0a39 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -2171,7 +2171,7 @@ static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>                 return 0;
>         }
>
> -       if (!p->vec_out) {
> +       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)))
> --
> 2.41.0
>

[PATCH 3/3] mm/selftests: Fix pagemap_ioctl memory map test

[Peter Xu]

__FILE__ is not guaranteed to exist in current dir.  Replace that with
argv[0] for memory map test.

Fixes: 46fd75d4a3c9 ("selftests: mm: add pagemap ioctl tests")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/mm/pagemap_ioctl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c b/tools/testing/selftests/mm/pagemap_ioctl.c
index befab43719ba..d59517ed3d48 100644
--- a/tools/testing/selftests/mm/pagemap_ioctl.c
+++ b/tools/testing/selftests/mm/pagemap_ioctl.c
@@ -36,6 +36,7 @@ int pagemap_fd;
 int uffd;
 int page_size;
 int hpage_size;
+const char *progname;
 
 #define LEN(region)	((region.end - region.start)/page_size)
 
@@ -1149,11 +1150,11 @@ int sanity_tests(void)
 	munmap(mem, mem_size);
 
 	/* 9. Memory mapped file */
-	fd = open(__FILE__, O_RDONLY);
+	fd = open(progname, O_RDONLY);
 	if (fd < 0)
 		ksft_exit_fail_msg("%s Memory mapped file\n", __func__);
 
-	ret = stat(__FILE__, &sbuf);
+	ret = stat(progname, &sbuf);
 	if (ret < 0)
 		ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno));
 
@@ -1472,12 +1473,14 @@ static void transact_test(int page_size)
 			      extra_thread_faults);
 }
 
-int main(void)
+int main(int argc, char *argv[])
 {
 	int mem_size, shmid, buf_size, fd, i, ret;
 	char *mem, *map, *fmem;
 	struct stat sbuf;
 
+	progname = argv[0];
+
 	ksft_print_header();
 
 	if (init_uffd())
-- 
2.41.0

Re: [PATCH 3/3] mm/selftests: Fix pagemap_ioctl memory map test

[David Hildenbrand]

On 16.11.23 21:15, Peter Xu wrote:
> __FILE__ is not guaranteed to exist in current dir.  Replace that with
> argv[0] for memory map test.
> 
> Fixes: 46fd75d4a3c9 ("selftests: mm: add pagemap ioctl tests")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   tools/testing/selftests/mm/pagemap_ioctl.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c b/tools/testing/selftests/mm/pagemap_ioctl.c
> index befab43719ba..d59517ed3d48 100644
> --- a/tools/testing/selftests/mm/pagemap_ioctl.c
> +++ b/tools/testing/selftests/mm/pagemap_ioctl.c
> @@ -36,6 +36,7 @@ int pagemap_fd;
>   int uffd;
>   int page_size;
>   int hpage_size;
> +const char *progname;
>   
>   #define LEN(region)	((region.end - region.start)/page_size)
>   
> @@ -1149,11 +1150,11 @@ int sanity_tests(void)
>   	munmap(mem, mem_size);
>   
>   	/* 9. Memory mapped file */
> -	fd = open(__FILE__, O_RDONLY);
> +	fd = open(progname, O_RDONLY);
>   	if (fd < 0)
>   		ksft_exit_fail_msg("%s Memory mapped file\n", __func__);
>   
> -	ret = stat(__FILE__, &sbuf);
> +	ret = stat(progname, &sbuf);
>   	if (ret < 0)
>   		ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno));
>   
> @@ -1472,12 +1473,14 @@ static void transact_test(int page_size)
>   			      extra_thread_faults);
>   }
>   
> -int main(void)
> +int main(int argc, char *argv[])
>   {
>   	int mem_size, shmid, buf_size, fd, i, ret;
>   	char *mem, *map, *fmem;
>   	struct stat sbuf;
>   
> +	progname = argv[0];
> +
>   	ksft_print_header();
>   
>   	if (init_uffd())

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb