diff mbox series

[07/15] parisc: use generic sys_fanotify_mark implementation

Message ID 20240620162316.3674955-8-arnd@kernel.org (mailing list archive)
State New
Headers show
Series linux system call fixes | expand

Commit Message

Arnd Bergmann June 20, 2024, 4:23 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>

The sys_fanotify_mark() syscall on parisc uses the reverse word order
for the two halves of the 64-bit argument compared to all syscalls on
all 32-bit architectures. As far as I can tell, the problem is that
the function arguments on parisc are sorted backwards (26, 25, 24, 23,
...) compared to everyone else, so the calling conventions of using an
even/odd register pair in native word order result in the lower word
coming first in function arguments, matching the expected behavior
on little-endian architectures. The system call conventions however
ended up matching what the other 32-bit architectures do.

A glibc cleanup in 2020 changed the userspace behavior in a way that
handles all architectures consistently, but this inadvertently broke
parisc32 by changing to the same method as everyone else.

The change made it into glibc-2.35 and subsequently into debian 12
(bookworm), which is the latest stable release. This means we
need to choose between reverting the glibc change or changing the
kernel to match it again, but either hange will leave some systems
broken.

Pick the option that is more likely to help current and future
users and change the kernel to match current glibc. This also
means the behavior is now consistent across architectures, but
it breaks running new kernels with old glibc builds before 2.35.

Link: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=d150181d73d9
Link: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/arch/parisc/kernel/sys_parisc.c?h=57b1dfbd5b4a39d
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I found this through code inspection, please double-check to make
sure I got the bug and the fix right.

The alternative is to fix this by reverting glibc back to the
unusual behavior.
---
 arch/parisc/Kconfig                     | 1 +
 arch/parisc/kernel/sys_parisc32.c       | 9 ---------
 arch/parisc/kernel/syscalls/syscall.tbl | 2 +-
 3 files changed, 2 insertions(+), 10 deletions(-)

Comments

Helge Deller June 20, 2024, 9:21 p.m. UTC | #1
On 6/20/24 18:23, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The sys_fanotify_mark() syscall on parisc uses the reverse word order
> for the two halves of the 64-bit argument compared to all syscalls on
> all 32-bit architectures. As far as I can tell, the problem is that
> the function arguments on parisc are sorted backwards (26, 25, 24, 23,
> ...) compared to everyone else,

r26 is arg0, r25 is arg1, and so on.
I'm not sure I would call this "sorted backwards".
I think the reason is simply that hppa is the only 32-bit big-endian
arch left...

> so the calling conventions of using an
> even/odd register pair in native word order result in the lower word
> coming first in function arguments, matching the expected behavior
> on little-endian architectures. The system call conventions however
> ended up matching what the other 32-bit architectures do.
>
> A glibc cleanup in 2020 changed the userspace behavior in a way that
> handles all architectures consistently, but this inadvertently broke
> parisc32 by changing to the same method as everyone else.

I appreciate such cleanups to make arches consistent.
But it's bad if breakages aren't noticed or reported then...

> The change made it into glibc-2.35 and subsequently into debian 12
> (bookworm), which is the latest stable release. This means we
> need to choose between reverting the glibc change or changing the
> kernel to match it again, but either hange will leave some systems
> broken.
>
> Pick the option that is more likely to help current and future
> users and change the kernel to match current glibc.

Agreed (assuming we have really a problem on parisc).

> This also
> means the behavior is now consistent across architectures, but
> it breaks running new kernels with old glibc builds before 2.35.
>
> Link: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=d150181d73d9
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/arch/parisc/kernel/sys_parisc.c?h=57b1dfbd5b4a39d
> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I found this through code inspection, please double-check to make
> sure I got the bug and the fix right.

The patch looks good at first sight.
I'll pick it up in my parisc git tree and will do some testing the
next few days and then push forward for 6.11 when it opens....

Thank you!!

Helge

> The alternative is to fix this by reverting glibc back to the
> unusual behavior.
> ---
>   arch/parisc/Kconfig                     | 1 +
>   arch/parisc/kernel/sys_parisc32.c       | 9 ---------
>   arch/parisc/kernel/syscalls/syscall.tbl | 2 +-
>   3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
> index daafeb20f993..dc9b902de8ea 100644
> --- a/arch/parisc/Kconfig
> +++ b/arch/parisc/Kconfig
> @@ -16,6 +16,7 @@ config PARISC
>   	select ARCH_HAS_UBSAN
>   	select ARCH_HAS_PTE_SPECIAL
>   	select ARCH_NO_SG_CHAIN
> +	select ARCH_SPLIT_ARG64 if !64BIT
>   	select ARCH_SUPPORTS_HUGETLBFS if PA20
>   	select ARCH_SUPPORTS_MEMORY_FAILURE
>   	select ARCH_STACKWALK
> diff --git a/arch/parisc/kernel/sys_parisc32.c b/arch/parisc/kernel/sys_parisc32.c
> index 2a12a547b447..826c8e51b585 100644
> --- a/arch/parisc/kernel/sys_parisc32.c
> +++ b/arch/parisc/kernel/sys_parisc32.c
> @@ -23,12 +23,3 @@ asmlinkage long sys32_unimplemented(int r26, int r25, int r24, int r23,
>       	current->comm, current->pid, r20);
>       return -ENOSYS;
>   }
> -
> -asmlinkage long sys32_fanotify_mark(compat_int_t fanotify_fd, compat_uint_t flags,
> -	compat_uint_t mask0, compat_uint_t mask1, compat_int_t dfd,
> -	const char  __user * pathname)
> -{
> -	return sys_fanotify_mark(fanotify_fd, flags,
> -			((__u64)mask1 << 32) | mask0,
> -			 dfd, pathname);
> -}
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 39e67fab7515..66dc406b12e4 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -364,7 +364,7 @@
>   320	common	accept4			sys_accept4
>   321	common	prlimit64		sys_prlimit64
>   322	common	fanotify_init		sys_fanotify_init
> -323	common	fanotify_mark		sys_fanotify_mark		sys32_fanotify_mark
> +323	common	fanotify_mark		sys_fanotify_mark		compat_sys_fanotify_mark
>   324	32	clock_adjtime		sys_clock_adjtime32
>   324	64	clock_adjtime		sys_clock_adjtime
>   325	common	name_to_handle_at	sys_name_to_handle_at
LEROY Christophe June 21, 2024, 5:26 a.m. UTC | #2
Le 20/06/2024 à 23:21, Helge Deller a écrit :
> [Vous ne recevez pas souvent de courriers de deller@gmx.de. Découvrez
> pourquoi ceci est important à
> https://aka.ms/LearnAboutSenderIdentification ]
>
> On 6/20/24 18:23, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The sys_fanotify_mark() syscall on parisc uses the reverse word order
>> for the two halves of the 64-bit argument compared to all syscalls on
>> all 32-bit architectures. As far as I can tell, the problem is that
>> the function arguments on parisc are sorted backwards (26, 25, 24, 23,
>> ...) compared to everyone else,
>
> r26 is arg0, r25 is arg1, and so on.
> I'm not sure I would call this "sorted backwards".
> I think the reason is simply that hppa is the only 32-bit big-endian
> arch left...

powerpc/32 is big-endian: r3 is arg0, r4 is arg1, ... r10 is arg7.

In case of a 64bits arg, r3 is the high part and r4 is the low part.

Christophe

>
>> so the calling conventions of using an
>> even/odd register pair in native word order result in the lower word
>> coming first in function arguments, matching the expected behavior
>> on little-endian architectures. The system call conventions however
>> ended up matching what the other 32-bit architectures do.
>>
>> A glibc cleanup in 2020 changed the userspace behavior in a way that
>> handles all architectures consistently, but this inadvertently broke
>> parisc32 by changing to the same method as everyone else.
>
> I appreciate such cleanups to make arches consistent.
> But it's bad if breakages aren't noticed or reported then...
>
>> The change made it into glibc-2.35 and subsequently into debian 12
>> (bookworm), which is the latest stable release. This means we
>> need to choose between reverting the glibc change or changing the
>> kernel to match it again, but either hange will leave some systems
>> broken.
>>
>> Pick the option that is more likely to help current and future
>> users and change the kernel to match current glibc.
>
> Agreed (assuming we have really a problem on parisc).
>
>> This also
>> means the behavior is now consistent across architectures, but
>> it breaks running new kernels with old glibc builds before 2.35.
>>
>> Link:
>> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=d150181d73d9
>> Link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/arch/parisc/kernel/sys_parisc.c?h=57b1dfbd5b4a39d
>> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> I found this through code inspection, please double-check to make
>> sure I got the bug and the fix right.
>
> The patch looks good at first sight.
> I'll pick it up in my parisc git tree and will do some testing the
> next few days and then push forward for 6.11 when it opens....
>
> Thank you!!
>
> Helge
>
>> The alternative is to fix this by reverting glibc back to the
>> unusual behavior.
>> ---
>>   arch/parisc/Kconfig                     | 1 +
>>   arch/parisc/kernel/sys_parisc32.c       | 9 ---------
>>   arch/parisc/kernel/syscalls/syscall.tbl | 2 +-
>>   3 files changed, 2 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
>> index daafeb20f993..dc9b902de8ea 100644
>> --- a/arch/parisc/Kconfig
>> +++ b/arch/parisc/Kconfig
>> @@ -16,6 +16,7 @@ config PARISC
>>       select ARCH_HAS_UBSAN
>>       select ARCH_HAS_PTE_SPECIAL
>>       select ARCH_NO_SG_CHAIN
>> +     select ARCH_SPLIT_ARG64 if !64BIT
>>       select ARCH_SUPPORTS_HUGETLBFS if PA20
>>       select ARCH_SUPPORTS_MEMORY_FAILURE
>>       select ARCH_STACKWALK
>> diff --git a/arch/parisc/kernel/sys_parisc32.c
>> b/arch/parisc/kernel/sys_parisc32.c
>> index 2a12a547b447..826c8e51b585 100644
>> --- a/arch/parisc/kernel/sys_parisc32.c
>> +++ b/arch/parisc/kernel/sys_parisc32.c
>> @@ -23,12 +23,3 @@ asmlinkage long sys32_unimplemented(int r26, int
>> r25, int r24, int r23,
>>               current->comm, current->pid, r20);
>>       return -ENOSYS;
>>   }
>> -
>> -asmlinkage long sys32_fanotify_mark(compat_int_t fanotify_fd,
>> compat_uint_t flags,
>> -     compat_uint_t mask0, compat_uint_t mask1, compat_int_t dfd,
>> -     const char  __user * pathname)
>> -{
>> -     return sys_fanotify_mark(fanotify_fd, flags,
>> -                     ((__u64)mask1 << 32) | mask0,
>> -                      dfd, pathname);
>> -}
>> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl
>> b/arch/parisc/kernel/syscalls/syscall.tbl
>> index 39e67fab7515..66dc406b12e4 100644
>> --- a/arch/parisc/kernel/syscalls/syscall.tbl
>> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
>> @@ -364,7 +364,7 @@
>>   320 common  accept4                 sys_accept4
>>   321 common  prlimit64               sys_prlimit64
>>   322 common  fanotify_init           sys_fanotify_init
>> -323  common  fanotify_mark           sys_fanotify_mark
>> sys32_fanotify_mark
>> +323  common  fanotify_mark           sys_fanotify_mark
>> compat_sys_fanotify_mark
>>   324 32      clock_adjtime           sys_clock_adjtime32
>>   324 64      clock_adjtime           sys_clock_adjtime
>>   325 common  name_to_handle_at       sys_name_to_handle_at
>
Arnd Bergmann June 21, 2024, 6:28 a.m. UTC | #3
On Fri, Jun 21, 2024, at 07:26, LEROY Christophe wrote:
> Le 20/06/2024 à 23:21, Helge Deller a écrit :
>> [Vous ne recevez pas souvent de courriers de deller@gmx.de. Découvrez
>> pourquoi ceci est important à
>> https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On 6/20/24 18:23, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> The sys_fanotify_mark() syscall on parisc uses the reverse word order
>>> for the two halves of the 64-bit argument compared to all syscalls on
>>> all 32-bit architectures. As far as I can tell, the problem is that
>>> the function arguments on parisc are sorted backwards (26, 25, 24, 23,
>>> ...) compared to everyone else,
>>
>> r26 is arg0, r25 is arg1, and so on.
>> I'm not sure I would call this "sorted backwards".
>> I think the reason is simply that hppa is the only 32-bit big-endian
>> arch left...
>
> powerpc/32 is big-endian: r3 is arg0, r4 is arg1, ... r10 is arg7.

Right, I'm pretty sure the ordering is the same on arm, mips,
s390, m68k, openrisc, sh and sparc when running 32-bit big-endian
code.

It's more likely to be related to the upward growing stack.
I checked the gcc sources and found that out of the 50 supported
architectures, ARGS_GROW_DOWNWARD is set on everything except
for gcn, stormy16 and  32-bit parisc. The other two are
little-endian though. STACK_GROWS_DOWNWARD in turn is set on
everything other than parisc (both 32-bit and 64-bit).

      Arnd
John Paul Adrian Glaubitz June 21, 2024, 8:52 a.m. UTC | #4
Hi Helge and Arnd,

On Thu, 2024-06-20 at 23:21 +0200, Helge Deller wrote:
> The patch looks good at first sight.
> I'll pick it up in my parisc git tree and will do some testing the
> next few days and then push forward for 6.11 when it opens....

Isn't this supposed to go in as one series or can arch maintainers actually
pick the patches for their architecture and merge them individually?

If yes, I would prefer to do that for the SuperH patch as well as I usually
prefer merging SuperH patches in my own tree.

Adrian
John Paul Adrian Glaubitz June 21, 2024, 8:54 a.m. UTC | #5
Hi,

On Fri, 2024-06-21 at 08:28 +0200, Arnd Bergmann wrote:
> It's more likely to be related to the upward growing stack.
> I checked the gcc sources and found that out of the 50 supported
> architectures, ARGS_GROW_DOWNWARD is set on everything except
> for gcn, stormy16 and  32-bit parisc. The other two are
> little-endian though. STACK_GROWS_DOWNWARD in turn is set on
> everything other than parisc (both 32-bit and 64-bit).

Wait a second! Does that mean that on 64-bit PA-RISC, the stack is
actually growing downwards? If yes, that would be a strong argument
for creating a 64-bit PA-RISC port in Debian and replacing the 32-bit
port.

Adrian
Arnd Bergmann June 21, 2024, 8:56 a.m. UTC | #6
On Fri, Jun 21, 2024, at 10:52, John Paul Adrian Glaubitz wrote:
> Hi Helge and Arnd,
>
> On Thu, 2024-06-20 at 23:21 +0200, Helge Deller wrote:
>> The patch looks good at first sight.
>> I'll pick it up in my parisc git tree and will do some testing the
>> next few days and then push forward for 6.11 when it opens....
>
> Isn't this supposed to go in as one series or can arch maintainers actually
> pick the patches for their architecture and merge them individually?
>
> If yes, I would prefer to do that for the SuperH patch as well as I usually
> prefer merging SuperH patches in my own tree.

The patches are all independent of one another, except for a couple
of context changes where multiple patches touch the same lines.

Feel free to pick up the sh patch directly, I'll just merge whatever
is left in the end. I mainly want to ensure we can get all the bugfixes
done for v6.10 so I can build my longer cleanup series on top of it
for 6.11.

   Arnd
John Paul Adrian Glaubitz June 21, 2024, 9:03 a.m. UTC | #7
On Fri, 2024-06-21 at 10:56 +0200, Arnd Bergmann wrote:
> The patches are all independent of one another, except for a couple
> of context changes where multiple patches touch the same lines.

OK.

> Feel free to pick up the sh patch directly, I'll just merge whatever
> is left in the end. I mainly want to ensure we can get all the bugfixes
> done for v6.10 so I can build my longer cleanup series on top of it
> for 6.11.

This series is still for 6.10?

Adrian
Arnd Bergmann June 21, 2024, 9:52 a.m. UTC | #8
On Fri, Jun 21, 2024, at 11:03, John Paul Adrian Glaubitz wrote:
> On Fri, 2024-06-21 at 10:56 +0200, Arnd Bergmann wrote:
>> Feel free to pick up the sh patch directly, I'll just merge whatever
>> is left in the end. I mainly want to ensure we can get all the bugfixes
>> done for v6.10 so I can build my longer cleanup series on top of it
>> for 6.11.
>
> This series is still for 6.10?

Yes, these are all the bugfixes that I think we want to backport
to stable kernels, so it makes sense to merge them as quickly as
possible. The actual stuff I'm working on will come as soon as
I have it in a state for public review and won't need to be
backported.

     Arnd
John David Anglin June 21, 2024, 12:22 p.m. UTC | #9
On 2024-06-21 4:54 a.m., John Paul Adrian Glaubitz wrote:
> Hi,
>
> On Fri, 2024-06-21 at 08:28 +0200, Arnd Bergmann wrote:
>> It's more likely to be related to the upward growing stack.
>> I checked the gcc sources and found that out of the 50 supported
>> architectures, ARGS_GROW_DOWNWARD is set on everything except
>> for gcn, stormy16 and  32-bit parisc. The other two are
>> little-endian though. STACK_GROWS_DOWNWARD in turn is set on
>> everything other than parisc (both 32-bit and 64-bit).
> Wait a second! Does that mean that on 64-bit PA-RISC, the stack is
> actually growing downwards? If yes, that would be a strong argument
> for creating a 64-bit PA-RISC port in Debian and replacing the 32-bit
> port.
No, the stack grows upward on both 32 and 64-bit parisc.  But stack arguments
grow upwards on 64-bit parisc.  The argument pointer is needed to access these
arguments.  In 32-bit parisc, the argument pointer is at a fixed offset relative to the
stack pointer and it can be eliminated.

Dave
Helge Deller June 21, 2024, 4:28 p.m. UTC | #10
On 6/21/24 11:52, Arnd Bergmann wrote:
> On Fri, Jun 21, 2024, at 11:03, John Paul Adrian Glaubitz wrote:
>> On Fri, 2024-06-21 at 10:56 +0200, Arnd Bergmann wrote:
>>> Feel free to pick up the sh patch directly, I'll just merge whatever
>>> is left in the end. I mainly want to ensure we can get all the bugfixes
>>> done for v6.10 so I can build my longer cleanup series on top of it
>>> for 6.11.
>>
>> This series is still for 6.10?
>
> Yes, these are all the bugfixes that I think we want to backport
> to stable kernels, so it makes sense to merge them as quickly as
> possible. The actual stuff I'm working on will come as soon as
> I have it in a state for public review and won't need to be
> backported.

Ah, OK.... in that case would you please keep the two parisc
patches in your git tree? I didn't plan to send a new pull
request during v6.10, so it's easier for me if you keep them
and send them together with your other remaining patches.
(I'll drop them now from the parisc tree)

I tested both patches, so you may add:
Tested-by: Helge Deller <deller@gmx.de>
Acked-by: Helge Deller <deller@gmx.de>

Thank you!
Helge
diff mbox series

Patch

diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index daafeb20f993..dc9b902de8ea 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -16,6 +16,7 @@  config PARISC
 	select ARCH_HAS_UBSAN
 	select ARCH_HAS_PTE_SPECIAL
 	select ARCH_NO_SG_CHAIN
+	select ARCH_SPLIT_ARG64 if !64BIT
 	select ARCH_SUPPORTS_HUGETLBFS if PA20
 	select ARCH_SUPPORTS_MEMORY_FAILURE
 	select ARCH_STACKWALK
diff --git a/arch/parisc/kernel/sys_parisc32.c b/arch/parisc/kernel/sys_parisc32.c
index 2a12a547b447..826c8e51b585 100644
--- a/arch/parisc/kernel/sys_parisc32.c
+++ b/arch/parisc/kernel/sys_parisc32.c
@@ -23,12 +23,3 @@  asmlinkage long sys32_unimplemented(int r26, int r25, int r24, int r23,
     	current->comm, current->pid, r20);
     return -ENOSYS;
 }
-
-asmlinkage long sys32_fanotify_mark(compat_int_t fanotify_fd, compat_uint_t flags,
-	compat_uint_t mask0, compat_uint_t mask1, compat_int_t dfd,
-	const char  __user * pathname)
-{
-	return sys_fanotify_mark(fanotify_fd, flags,
-			((__u64)mask1 << 32) | mask0,
-			 dfd, pathname);
-}
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 39e67fab7515..66dc406b12e4 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -364,7 +364,7 @@ 
 320	common	accept4			sys_accept4
 321	common	prlimit64		sys_prlimit64
 322	common	fanotify_init		sys_fanotify_init
-323	common	fanotify_mark		sys_fanotify_mark		sys32_fanotify_mark
+323	common	fanotify_mark		sys_fanotify_mark		compat_sys_fanotify_mark
 324	32	clock_adjtime		sys_clock_adjtime32
 324	64	clock_adjtime		sys_clock_adjtime
 325	common	name_to_handle_at	sys_name_to_handle_at