Re: [PATCH 09/15] sh: rework sync_file_range ABI

[John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>]

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@arndb.de>
> > > 
> > > 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