[07/15] parisc: use generic sys_fanotify_mark implementation
Checks
Commit Message
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
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
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
>
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
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
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
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
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
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
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
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
@@ -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
@@ -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);
-}
@@ -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