Hi Arnd, On Fri, 2024-06-21 at 11:41 +0200, Arnd Bergmann wrote: > On Fri, Jun 21, 2024, at 10:44, John Paul Adrian Glaubitz wrote: > > On Thu, 2024-06-20 at 18:23 +0200, Arnd Bergmann wrote: > > > From: Arnd Bergmann <arnd@xxxxxxxx> > > > > > > The unusual function calling conventions on superh ended up causing > > ^^^^^^ > > It's spelled SuperH > > Fixed now. > > > > diff --git a/arch/sh/kernel/sys_sh32.c b/arch/sh/kernel/sys_sh32.c > > > index 9dca568509a5..d5a4f7c697d8 100644 > > > --- a/arch/sh/kernel/sys_sh32.c > > > +++ b/arch/sh/kernel/sys_sh32.c > > > @@ -59,3 +59,14 @@ asmlinkage int sys_fadvise64_64_wrapper(int fd, u32 offset0, u32 offset1, > > > (u64)len0 << 32 | len1, advice); > > > #endif > > > } > > > + > > > +/* > > > + * swap the arguments the way that libc wants it instead of > > > > I think "swap the arguments to the order that libc wants them" would > > be easier to understand here. > > Done Thanks for the two improvements! > > > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl > > > index bbf83a2db986..c55fd7696d40 100644 > > > --- a/arch/sh/kernel/syscalls/syscall.tbl > > > +++ b/arch/sh/kernel/syscalls/syscall.tbl > > > @@ -321,7 +321,7 @@ > > > 311 common set_robust_list sys_set_robust_list > > > 312 common get_robust_list sys_get_robust_list > > > 313 common splice sys_splice > > > -314 common sync_file_range sys_sync_file_range > > > +314 common sync_file_range sys_sh_sync_file_range6 > > ^^^^^^ > > Why the suffix 6 here? > > In a later part of my cleanup, I'm consolidating all the > copies of this function (arm64, mips, parisc, powerpc, > s390, sh, sparc, x86) and picked the name > sys_sync_file_range6() for common implementation. > > I end up with four entry points here, so the naming is a bit > confusing: > > - sys_sync_file_range() is only used on 64-bit architectures, > on x32 and on mips-n32. This uses four arguments, including > two 64-bit wide ones. > > - sys_sync_file_range2() continues to be used on arm, powerpc, > xtensa and now on sh, hexagon and csky. I change the > implementation to take six 32-bit arguments, but the ABI > remains the same as before, with the flags before offset. > > - sys_sync_file_range6() is used for most other 32-bit ABIs: > arc, m68k, microblaze, nios2, openrisc, parisc, s390, sh, sparc > and x86. This also has six 32-bit arguments but in the > default order (fd, offset, nbytes, flags). > > - sys_sync_file_range7() is exclusive to mips-o32, this one > has an unused argument and is otherwise the same as > sys_sync_file_range6(). > > My plan is to then have some infrastructure to ensure > userspace tools (libc, strace, qemu, rust, ...) use the > same calling conventions as the kernel. I'm doing the > same thing for all other syscalls that have architecture > specific calling conventions, so far I'm using > > fadvise64_64_7 > fanotify_mark6 > truncate3 > truncate4 > ftruncate3 > ftruncate4 > fallocate6 > pread5 > pread6 > pwrite5 > pwrite6 > preadv5 > preadv6 > pwritev5 > pwritev6 > sync_file_range6 > fadvise64_64_2 > fadvise64_64_6 > fadvise64_5 > fadvise64_6 > readahead4 > readahead5 > > The last number here is usually the number of 32-bit > arguments, except for fadvise64_64_2 that uses the > same argument reordering trick as sync_file_range2. > > I'm not too happy with the naming but couldn't come up with > anything clearer either, so let me know if you have any > ideas there. OK, gotcha. I thought the 6 suffix was for SH only. I'm fine with the naming scheme. > > > 315 common tee sys_tee > > > 316 common vmsplice sys_vmsplice > > > 317 common move_pages sys_move_pages > > > @@ -395,6 +395,7 @@ > > > 385 common pkey_alloc sys_pkey_alloc > > > 386 common pkey_free sys_pkey_free > > > 387 common rseq sys_rseq > > > +388 common sync_file_range2 sys_sync_file_range2 > > > # room for arch specific syscalls > > > 393 common semget sys_semget > > > 394 common semctl sys_semctl > > > > I wonder how you discovered this bug. Did you look up the calling > > convention on SuperH > > and compare the argument order for the sys_sync_file_range system call > > documented there > > with the order in the kernel? > > I had to categorize all architectures based on their calling > conventions to see if 64-bit arguments need aligned pairs or > not, so I wrote a set of simple C files that I compiled for > all architectures to see in which cases they insert unused > arguments or swap the order of the upper and lower halves. > > SuperH, parisc and s390 are each slightly different from all the > others here, so I ended up reading the ELF psABI docs and/or > the compiler sources to be sure. > I also a lot of git history. Great job, thanks for doing the extra work to verify the ABI. > > Did you also check what order libc uses? I would expect libc on SuperH > > misordering the > > arguments as well unless I am missing something. Or do we know that the > > code is actually > > currently broken? > > Yes, I checked glibc, musl and uclibc-ng for all the cases in > which the ABI made no sense, as well as to check that my analysis > of the kernel sources matches the expectations of the libc. OK, awesome. Will you send a v2 so I can ack the updated version of the patch? I'm also fine with the patch going through your tree, as I would like to start with the changes for v6.11 this week. Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer `. `' Physicist `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
