[PATCH] Fix do_pages_stat to use compat_uptr_t

[PATCH] Fix do_pages_stat to use compat_uptr_t

[Christoph Berg]

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

Hi,

PostgreSQL 18 will feature NUMA introspection of its shared memory
structures. The regression tests for this are failing on Debian's
32-bit architectures which are nowadays all built on 64-bit hosts
(i386, armel, armhf, x32).

Bertrand Drouvot analyzed the issue to be in do_pages_stat:

https://www.postgresql.org/message-id/flat/a3a4fe3d-1a80-4e03-aa8e-150ee15f6c35%40vondra.me#6abe7eaa802b5b07bb70cc3229e63a9f

do_pages_stat() is already handling the input arrays correctly in
32-bit mode, but at the end of the "while (nr_pages)" loop, it
incorrectly advances the pages pointer with the wrong word size.

The attached patch fixes the problem.

Christoph

[-- Attachment #2: 0001-Fix-do_pages_stat-to-use-compat_uptr_t.patch --]
[-- Type: text/x-diff, Size: 1060 bytes --]

From 70225fb0be382c3fd443e8331688b88d0e52c04c Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Tue, 24 Jun 2025 16:44:27 +0200
Subject: [PATCH] Fix do_pages_stat to use compat_uptr_t

For arrays with more than 16 entries, the old code would incorrectly
advance the pages pointer by 16 words instead of 16 compat_uptr_t.

Signed-off-by: Christoph Berg <myon@debian.org>
---
 mm/migrate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8cf0f9c9599..542c81ec3ed 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2444,7 +2444,13 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
 			break;
 
-		pages += chunk_nr;
+		if (in_compat_syscall()) {
+			compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
+
+			pages32 += chunk_nr;
+			pages = (const void __user * __user *) pages32;
+		} else
+			pages += chunk_nr;
 		status += chunk_nr;
 		nr_pages -= chunk_nr;
 	}
-- 
2.47.2

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[Dev Jain]

On 24/06/25 8:47 pm, Christoph Berg wrote:
> Hi,
>
> PostgreSQL 18 will feature NUMA introspection of its shared memory
> structures. The regression tests for this are failing on Debian's
> 32-bit architectures which are nowadays all built on 64-bit hosts
> (i386, armel, armhf, x32).
>
> Bertrand Drouvot analyzed the issue to be in do_pages_stat:
>
> https://www.postgresql.org/message-id/flat/a3a4fe3d-1a80-4e03-aa8e-150ee15f6c35%40vondra.me#6abe7eaa802b5b07bb70cc3229e63a9f
>
> do_pages_stat() is already handling the input arrays correctly in
> 32-bit mode, but at the end of the "while (nr_pages)" loop, it
> incorrectly advances the pages pointer with the wrong word size.
>
> The attached patch fixes the problem.
>
> Christoph

Hello Christoph,

Thanks for your effort. Please take a look at Documentation/process/submitting-patches.rst
and send the patch directly as an email.

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH] Fix do_pages_stat to use compat_uptr_t

[Christoph Berg]

For arrays with more than 16 entries, the old code would incorrectly
advance the pages pointer by 16 words instead of 16 compat_uptr_t.

Signed-off-by: Christoph Berg <myon@debian.org>
Suggested-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
---
 mm/migrate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8cf0f9c9599..542c81ec3ed 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2444,7 +2444,13 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
 			break;
 
-		pages += chunk_nr;
+		if (in_compat_syscall()) {
+			compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
+
+			pages32 += chunk_nr;
+			pages = (const void __user * __user *) pages32;
+		} else
+			pages += chunk_nr;
 		status += chunk_nr;
 		nr_pages -= chunk_nr;
 	}
-- 
2.47.2

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[David Hildenbrand]

On 25.06.25 17:24, Christoph Berg wrote:

Subject should start with "mm/migrate:"

> For arrays with more than 16 entries, the old code would incorrectly
> advance the pages pointer by 16 words instead of 16 compat_uptr_t.
> 
> Signed-off-by: Christoph Berg <myon@debian.org>
> Suggested-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

Likely we want a

Fixes:

and then this is probably "Reported-by:" paired with a "Closes:" link
to any such report.

But I'm wondering how long this has already been like that. :)

> ---
>   mm/migrate.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8cf0f9c9599..542c81ec3ed 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2444,7 +2444,13 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>   		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
>   			break;
>   
> -		pages += chunk_nr;
> +		if (in_compat_syscall()) {
> +			compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
> +
> +			pages32 += chunk_nr;
> +			pages = (const void __user * __user *) pages32;
> +		} else
> +			pages += chunk_nr;
>   		status += chunk_nr;
>   		nr_pages -= chunk_nr;
>   	}

Something a bit more elegant might be:

diff --git a/mm/migrate.c b/mm/migrate.c
index ea8c74d996592..fc99448771041 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2398,6 +2398,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
  
  static int get_compat_pages_array(const void __user *chunk_pages[],
                                   const void __user * __user *pages,
+                                 unsigned long chunk_offs,
                                   unsigned long chunk_nr)
  {
         compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
@@ -2405,7 +2406,7 @@ static int get_compat_pages_array(const void __user *chunk_pages[],
         int i;
  
         for (i = 0; i < chunk_nr; i++) {
-               if (get_user(p, pages32 + i))
+               if (get_user(p, pages32 + i + chunk_offs))
                         return -EFAULT;
                 chunk_pages[i] = compat_ptr(p);
         }
@@ -2424,13 +2425,14 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
  #define DO_PAGES_STAT_CHUNK_NR 16UL
         const void __user *chunk_pages[DO_PAGES_STAT_CHUNK_NR];
         int chunk_status[DO_PAGES_STAT_CHUNK_NR];
+       unsigned long chunk_offs = 0;
  
         while (nr_pages) {
                 unsigned long chunk_nr = min(nr_pages, DO_PAGES_STAT_CHUNK_NR);
  
                 if (in_compat_syscall()) {
                         if (get_compat_pages_array(chunk_pages, pages,
-                                                  chunk_nr))
+                                                  chunk_offs, chunk_nr))
                                 break;
                 } else {
                         if (copy_from_user(chunk_pages, pages,
@@ -2440,11 +2442,11 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
  
                 do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
  
-               if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
+               if (copy_to_user(status + chunk_offs, chunk_status,
+                                chunk_nr * sizeof(*status)))
                         break;
  
-               pages += chunk_nr;
-               status += chunk_nr;
+               chunk_offs += chunk_nr;
                 nr_pages -= chunk_nr;
         }
         return nr_pages ? -EFAULT : 0;

(untested, of course)


-- 
Cheers,

David / dhildenb

[PATCH v2] mm/migrate: Fix do_pages_stat in 32-bit mode

[Christoph Berg]

Re: David Hildenbrand
> Subject should start with "mm/migrate:"
> Likely we want a
> Fixes:
> and then this is probably "Reported-by:" paired with a "Closes:" link
> to any such report.

I included these now, except for "Closes:" which I have to idea what
to put in.

> But I'm wondering how long this has already been like that. :)

The now-offending "pages += chunk_nr" line is from 2010, but I think
the bug is rather from 5b1b561ba73c8ab9c98e5dfd14dc7ee47efb6530 (2021)
which reshuffled the array-vs-32-bit handling.

> Something a bit more elegant might be:

Thanks, I used your patch draft with some minor changes.

>  static int get_compat_pages_array(const void __user *chunk_pages[],
>                                   const void __user * __user *pages,
> +                                 unsigned long chunk_offs,

I replaced chunk_offs with "chunk_offset" since "offs" looked too much
like plural (list of offsets) to me.

>                 if (in_compat_syscall()) {
>                         if (get_compat_pages_array(chunk_pages, pages,
> -                                                  chunk_nr))
> +                                                  chunk_offs, chunk_nr))
>                                 break;
>                 } else {
>                         if (copy_from_user(chunk_pages, pages,

The else branch here needs tweaking as well:

                } else {
-                       if (copy_from_user(chunk_pages, pages,
+                       if (copy_from_user(chunk_pages, pages + chunk_offset,
                                      chunk_nr * sizeof(*chunk_pages)))


> @@ -2440,11 +2442,11 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>                 do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
> -               if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
> +               if (copy_to_user(status + chunk_offs, chunk_status,
> +                                chunk_nr * sizeof(*status)))

This seems to work, but honestly I am wondering, if copy_from_user
needs a special 32-bit case, doesn't copy_to_user need special casing
as well?

> (untested, of course)

The attached patch makes PG18's new numa test pass on amd64 kernels
both in amd64 and i386 userlands.

(In the meantime, PG git head got a workaround that limits the chunk
size to the same 16 as used in do_pages_stat; I tested with the
version before that.)

Christoph


From fdbcbc88825bc2e857dfeeebc91d62864e0774dd Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Tue, 24 Jun 2025 16:44:27 +0200
Subject: [PATCH v2] mm/migrate: Fix do_pages_stat in 32-bit mode

For arrays with more than 16 entries, the old code would incorrectly
advance the pages pointer by 16 words instead of 16 compat_uptr_t.
Fix by doing the pointer arithmetic inside get_compat_pages_array where
pages32 is already a correctly-typed pointer.

Discovered while working on PostgreSQL 18's new NUMA introspection code.

Signed-off-by: Christoph Berg <myon@debian.org>
Reported-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reported-by: Tomas Vondra <tomas@vondra.me>
Suggested-by: David Hildenbrand <david@redhat.com>
Fixes: 5b1b561ba73c8ab9c98e5dfd14dc7ee47efb6530
---
 mm/migrate.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8cf0f9c9599d..2c88f3b33833 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2399,6 +2399,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
 
 static int get_compat_pages_array(const void __user *chunk_pages[],
 				  const void __user * __user *pages,
+				  unsigned long chunk_offset,
 				  unsigned long chunk_nr)
 {
 	compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
@@ -2406,7 +2407,7 @@ static int get_compat_pages_array(const void __user *chunk_pages[],
 	int i;
 
 	for (i = 0; i < chunk_nr; i++) {
-		if (get_user(p, pages32 + i))
+		if (get_user(p, pages32 + chunk_offset + i))
 			return -EFAULT;
 		chunk_pages[i] = compat_ptr(p);
 	}
@@ -2425,27 +2426,28 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 #define DO_PAGES_STAT_CHUNK_NR 16UL
 	const void __user *chunk_pages[DO_PAGES_STAT_CHUNK_NR];
 	int chunk_status[DO_PAGES_STAT_CHUNK_NR];
+	unsigned long chunk_offset = 0;
 
 	while (nr_pages) {
 		unsigned long chunk_nr = min(nr_pages, DO_PAGES_STAT_CHUNK_NR);
 
 		if (in_compat_syscall()) {
 			if (get_compat_pages_array(chunk_pages, pages,
-						   chunk_nr))
+						   chunk_offset, chunk_nr))
 				break;
 		} else {
-			if (copy_from_user(chunk_pages, pages,
+			if (copy_from_user(chunk_pages, pages + chunk_offset,
 				      chunk_nr * sizeof(*chunk_pages)))
 				break;
 		}
 
 		do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
 
-		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
+		if (copy_to_user(status + chunk_offset, chunk_status,
+				 chunk_nr * sizeof(*status)))
 			break;
 
-		pages += chunk_nr;
-		status += chunk_nr;
+		chunk_offset += chunk_nr;
 		nr_pages -= chunk_nr;
 	}
 	return nr_pages ? -EFAULT : 0;
-- 
2.47.2

Re: [PATCH v2] mm/migrate: Fix do_pages_stat in 32-bit mode

[Zi Yan]

On 1 Jul 2025, at 12:58, Christoph Berg wrote:

> Re: David Hildenbrand
>> Subject should start with "mm/migrate:"
>> Likely we want a
>> Fixes:
>> and then this is probably "Reported-by:" paired with a "Closes:" link
>> to any such report.
>
> I included these now, except for "Closes:" which I have to idea what
> to put in.

Fixes should be:

Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")

Closes could be a link to the bug report.

>
>> But I'm wondering how long this has already been like that. :)
>
> The now-offending "pages += chunk_nr" line is from 2010, but I think
> the bug is rather from 5b1b561ba73c8ab9c98e5dfd14dc7ee47efb6530 (2021)
> which reshuffled the array-vs-32-bit handling.
>
>> Something a bit more elegant might be:
>
> Thanks, I used your patch draft with some minor changes.
>
>>  static int get_compat_pages_array(const void __user *chunk_pages[],
>>                                   const void __user * __user *pages,
>> +                                 unsigned long chunk_offs,
>
> I replaced chunk_offs with "chunk_offset" since "offs" looked too much
> like plural (list of offsets) to me.
>
>>                 if (in_compat_syscall()) {
>>                         if (get_compat_pages_array(chunk_pages, pages,
>> -                                                  chunk_nr))
>> +                                                  chunk_offs, chunk_nr))
>>                                 break;
>>                 } else {
>>                         if (copy_from_user(chunk_pages, pages,
>
> The else branch here needs tweaking as well:
>
>                 } else {
> -                       if (copy_from_user(chunk_pages, pages,
> +                       if (copy_from_user(chunk_pages, pages + chunk_offset,
>                                       chunk_nr * sizeof(*chunk_pages)))
>
>
>> @@ -2440,11 +2442,11 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>>                 do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
>> -               if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
>> +               if (copy_to_user(status + chunk_offs, chunk_status,
>> +                                chunk_nr * sizeof(*status)))
>
> This seems to work, but honestly I am wondering, if copy_from_user
> needs a special 32-bit case, doesn't copy_to_user need special casing
> as well?
>
>> (untested, of course)
>
> The attached patch makes PG18's new numa test pass on amd64 kernels
> both in amd64 and i386 userlands.
>
> (In the meantime, PG git head got a workaround that limits the chunk
> size to the same 16 as used in do_pages_stat; I tested with the
> version before that.)
>
> Christoph
>
>
> From fdbcbc88825bc2e857dfeeebc91d62864e0774dd Mon Sep 17 00:00:00 2001
> From: Christoph Berg <myon@debian.org>
> Date: Tue, 24 Jun 2025 16:44:27 +0200
> Subject: [PATCH v2] mm/migrate: Fix do_pages_stat in 32-bit mode
>
> For arrays with more than 16 entries, the old code would incorrectly
> advance the pages pointer by 16 words instead of 16 compat_uptr_t.
> Fix by doing the pointer arithmetic inside get_compat_pages_array where
> pages32 is already a correctly-typed pointer.
>
> Discovered while working on PostgreSQL 18's new NUMA introspection code.
>
> Signed-off-by: Christoph Berg <myon@debian.org>
> Reported-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
> Reported-by: Tomas Vondra <tomas@vondra.me>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Fixes: 5b1b561ba73c8ab9c98e5dfd14dc7ee47efb6530
> ---
>  mm/migrate.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8cf0f9c9599d..2c88f3b33833 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2399,6 +2399,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
>
>  static int get_compat_pages_array(const void __user *chunk_pages[],
>  				  const void __user * __user *pages,
> +				  unsigned long chunk_offset,
>  				  unsigned long chunk_nr)
>  {
>  	compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
> @@ -2406,7 +2407,7 @@ static int get_compat_pages_array(const void __user *chunk_pages[],
>  	int i;
>
>  	for (i = 0; i < chunk_nr; i++) {
> -		if (get_user(p, pages32 + i))
> +		if (get_user(p, pages32 + chunk_offset + i))
>  			return -EFAULT;
>  		chunk_pages[i] = compat_ptr(p);
>  	}
> @@ -2425,27 +2426,28 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>  #define DO_PAGES_STAT_CHUNK_NR 16UL
>  	const void __user *chunk_pages[DO_PAGES_STAT_CHUNK_NR];
>  	int chunk_status[DO_PAGES_STAT_CHUNK_NR];
> +	unsigned long chunk_offset = 0;
>
>  	while (nr_pages) {
>  		unsigned long chunk_nr = min(nr_pages, DO_PAGES_STAT_CHUNK_NR);
>
>  		if (in_compat_syscall()) {
>  			if (get_compat_pages_array(chunk_pages, pages,
> -						   chunk_nr))
> +						   chunk_offset, chunk_nr))
>  				break;
>  		} else {
> -			if (copy_from_user(chunk_pages, pages,
> +			if (copy_from_user(chunk_pages, pages + chunk_offset,
>  				      chunk_nr * sizeof(*chunk_pages)))
>  				break;
>  		}
>
>  		do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
>
> -		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
> +		if (copy_to_user(status + chunk_offset, chunk_status,
> +				 chunk_nr * sizeof(*status)))
>  			break;
>
> -		pages += chunk_nr;
> -		status += chunk_nr;
> +		chunk_offset += chunk_nr;
>  		nr_pages -= chunk_nr;
>  	}
>  	return nr_pages ? -EFAULT : 0;
> -- 
> 2.47.2


Best Regards,
Yan, Zi

Re: [PATCH v3] mm/migrate: Fix do_pages_stat in 32-bit mode

[Christoph Berg]

Re: Zi Yan
> Fixes should be:
> 
> Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")
> 
> Closes could be a link to the bug report.

Updated, thanks.

> > This seems to work, but honestly I am wondering, if copy_from_user
> > needs a special 32-bit case, doesn't copy_to_user need special casing
> > as well?

Scratch that, it works because an int[] is copied back, and that's
always the same size.

So I think the patch is good to go.

Christoph


From 426c93d558572248273cf386ca784626ae431413 Mon Sep 17 00:00:00 2001
From: Christoph Berg <myon@debian.org>
Date: Tue, 24 Jun 2025 16:44:27 +0200
Subject: [PATCH v3] mm/migrate: Fix do_pages_stat in 32-bit mode

For arrays with more than 16 entries, the old code would incorrectly
advance the pages pointer by 16 words instead of 16 compat_uptr_t.
Fix by doing the pointer arithmetic inside get_compat_pages_array where
pages32 is already a correctly-typed pointer.

Discovered while working on PostgreSQL 18's new NUMA introspection code.

Signed-off-by: Christoph Berg <myon@debian.org>
Suggested-by: David Hildenbrand <david@redhat.com>
Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")
Reported-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Reported-by: Tomas Vondra <tomas@vondra.me>
Closes: https://www.postgresql.org/message-id/flat/6342f601-77de-4ee0-8c2a-3deb50ceac5b%40vondra.me#86402e3d80c031788f5f55b42c459471
---
 mm/migrate.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8cf0f9c9599d..2c88f3b33833 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2399,6 +2399,7 @@ static void do_pages_stat_array(struct mm_struct *mm, unsigned long nr_pages,
 
 static int get_compat_pages_array(const void __user *chunk_pages[],
 				  const void __user * __user *pages,
+				  unsigned long chunk_offset,
 				  unsigned long chunk_nr)
 {
 	compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
@@ -2406,7 +2407,7 @@ static int get_compat_pages_array(const void __user *chunk_pages[],
 	int i;
 
 	for (i = 0; i < chunk_nr; i++) {
-		if (get_user(p, pages32 + i))
+		if (get_user(p, pages32 + chunk_offset + i))
 			return -EFAULT;
 		chunk_pages[i] = compat_ptr(p);
 	}
@@ -2425,27 +2426,28 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
 #define DO_PAGES_STAT_CHUNK_NR 16UL
 	const void __user *chunk_pages[DO_PAGES_STAT_CHUNK_NR];
 	int chunk_status[DO_PAGES_STAT_CHUNK_NR];
+	unsigned long chunk_offset = 0;
 
 	while (nr_pages) {
 		unsigned long chunk_nr = min(nr_pages, DO_PAGES_STAT_CHUNK_NR);
 
 		if (in_compat_syscall()) {
 			if (get_compat_pages_array(chunk_pages, pages,
-						   chunk_nr))
+						   chunk_offset, chunk_nr))
 				break;
 		} else {
-			if (copy_from_user(chunk_pages, pages,
+			if (copy_from_user(chunk_pages, pages + chunk_offset,
 				      chunk_nr * sizeof(*chunk_pages)))
 				break;
 		}
 
 		do_pages_stat_array(mm, chunk_nr, chunk_pages, chunk_status);
 
-		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
+		if (copy_to_user(status + chunk_offset, chunk_status,
+				 chunk_nr * sizeof(*status)))
 			break;
 
-		pages += chunk_nr;
-		status += chunk_nr;
+		chunk_offset += chunk_nr;
 		nr_pages -= chunk_nr;
 	}
 	return nr_pages ? -EFAULT : 0;
-- 
2.47.2

Re: [PATCH v3] mm/migrate: Fix do_pages_stat in 32-bit mode

[David Hildenbrand]

On 01.07.25 19:55, Christoph Berg wrote:
> Re: Zi Yan
>> Fixes should be:
>>
>> Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")
>>
>> Closes could be a link to the bug report.
> 
> Updated, thanks.
> 
>>> This seems to work, but honestly I am wondering, if copy_from_user
>>> needs a special 32-bit case, doesn't copy_to_user need special casing
>>> as well?
> 
> Scratch that, it works because an int[] is copied back, and that's
> always the same size.
> 
> So I think the patch is good to go.
> 
> Christoph
> 
> 

If you have to resend, next time resend the full patch separately, not 
as reply to the previous version.

>  From 426c93d558572248273cf386ca784626ae431413 Mon Sep 17 00:00:00 2001
> From: Christoph Berg <myon@debian.org>
> Date: Tue, 24 Jun 2025 16:44:27 +0200
> Subject: [PATCH v3] mm/migrate: Fix do_pages_stat in 32-bit mode

s/32-bit mode/compat mode/ ?

Because on native 32bit it should be working fine.

> 
> For arrays with more than 16 entries, the old code would incorrectly
> advance the pages pointer by 16 words instead of 16 compat_uptr_t.
> Fix by doing the pointer arithmetic inside get_compat_pages_array where
> pages32 is already a correctly-typed pointer.
> 
> Discovered while working on PostgreSQL 18's new NUMA introspection code.
> 
> Signed-off-by: Christoph Berg <myon@debian.org>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")

Hmm, still not sure if 5b1b561ba73c really introduced the issue. I think 
it only messed with the "pages" pointer, not with the "status" pointer?

Hmmmm


I assume we want to Cc stable. @Andrew can do that.

> Reported-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
> Reported-by: Tomas Vondra <tomas@vondra.me>
> Closes: https://www.postgresql.org/message-id/flat/6342f601-77de-4ee0-8c2a-3deb50ceac5b%40vondra.me#86402e3d80c031788f5f55b42c459471
> ---

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

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] mm/migrate: Fix do_pages_stat in 32-bit mode

[Christoph Berg]

Re: David Hildenbrand
> > For arrays with more than 16 entries, the old code would incorrectly
> > advance the pages pointer by 16 words instead of 16 compat_uptr_t.
> > Fix by doing the pointer arithmetic inside get_compat_pages_array where
> > pages32 is already a correctly-typed pointer.
> > 
> > Discovered while working on PostgreSQL 18's new NUMA introspection code.
> > 
> > Signed-off-by: Christoph Berg <myon@debian.org>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages")
> 
> Hmm, still not sure if 5b1b561ba73c really introduced the issue. I think it
> only messed with the "pages" pointer, not with the "status" pointer?

"pages" was the broken one. "status" isn't affected by compat mode.

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

Thanks!

Christoph

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[Andrew Morton]

On Wed, 25 Jun 2025 17:24:14 +0200 Christoph Berg <myon@debian.org> wrote:

> For arrays with more than 16 entries, the old code would incorrectly
> advance the pages pointer by 16 words instead of 16 compat_uptr_t.
> 
> ...
>
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2444,7 +2444,13 @@ static int do_pages_stat(struct mm_struct *mm, unsigned long nr_pages,
>  		if (copy_to_user(status, chunk_status, chunk_nr * sizeof(*status)))
>  			break;
>  
> -		pages += chunk_nr;
> +		if (in_compat_syscall()) {
> +			compat_uptr_t __user *pages32 = (compat_uptr_t __user *)pages;
> +
> +			pages32 += chunk_nr;
> +			pages = (const void __user * __user *) pages32;
> +		} else
> +			pages += chunk_nr;
>  		status += chunk_nr;
>  		nr_pages -= chunk_nr;
>  	}

Seems this has been present since 2010.

I'll update the Subject: as David suggests and I'll add a cc:stable,
thanks.  I'll also add a note that David suggested an alternative, so
please let's advance that option.

Also, a coding-style tweak:

--- a/mm/migrate.c~fix-do_pages_stat-to-use-compat_uptr_t-fix
+++ a/mm/migrate.c
@@ -2449,8 +2449,9 @@ static int do_pages_stat(struct mm_struc
 
 			pages32 += chunk_nr;
 			pages = (const void __user * __user *) pages32;
-		} else
+		} else {
 			pages += chunk_nr;
+		}
 		status += chunk_nr;
 		nr_pages -= chunk_nr;
 	}
_

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[Christoph Berg]

Re: Andrew Morton
> I'll update the Subject: as David suggests and I'll add a cc:stable,
> thanks.  I'll also add a note that David suggested an alternative, so
> please let's advance that option.

Sorry, I'm new here. Do I have to do anything now? The above sounds
like the alternative coding by David would be preferred, but the other
mails say the patches have already been pushed to a hotfix branch? Or
are we doing both, i.e. stable gets the simple fix and next the pretty
one?

Christoph

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[Andrew Morton]

On Wed, 25 Jun 2025 23:10:13 +0200 Christoph Berg <myon@debian.org> wrote:

> Re: Andrew Morton
> > I'll update the Subject: as David suggests and I'll add a cc:stable,
> > thanks.  I'll also add a note that David suggested an alternative, so
> > please let's advance that option.
> 
> Sorry, I'm new here. Do I have to do anything now? The above sounds
> like the alternative coding by David would be preferred, but the other
> mails say the patches have already been pushed to a hotfix branch? Or
> are we doing both, i.e. stable gets the simple fix and next the pretty
> one?

Well I added the v1 patch for testing and so it doesn't get lost -
fixing the bug is the most important thing.

But if a nicer v2 patch is sent in the next few days, I can switch to
that version.

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[David Hildenbrand]

On 25.06.25 23:15, Andrew Morton wrote:
> On Wed, 25 Jun 2025 23:10:13 +0200 Christoph Berg <myon@debian.org> wrote:
> 
>> Re: Andrew Morton
>>> I'll update the Subject: as David suggests and I'll add a cc:stable,
>>> thanks.  I'll also add a note that David suggested an alternative, so
>>> please let's advance that option.
>>
>> Sorry, I'm new here. Do I have to do anything now? The above sounds
>> like the alternative coding by David would be preferred

See if my proposal makes sense, and then incorporate+test it.

You can feel free to add my

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

-- 
Cheers,

David / dhildenb

Re: [PATCH] Fix do_pages_stat to use compat_uptr_t

[Christoph Berg]

Re: David Hildenbrand
> See if my proposal makes sense, and then incorporate+test it.
> 
> You can feel free to add my
> 
> Suggested-by: David Hildenbrand <david@redhat.com>

Thanks, will do. But it will be next week, currently busy with the ham
radio fair in Friedrichshafen.

Christoph