| Message ID | 20240620162316.3674955-8-arnd@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux system call fixes | expand |
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
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