Message-ID: <e0e373fa13636a403322fd0ba96915fd25dbbefa.camel@physik.fu-berlin.de>
Date: Mon, 24 Jun 2024 08:14:36 +0200
From: John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>
To: Arnd Bergmann <arnd@...db.de>, Arnd Bergmann <arnd@...nel.org>,
 Linux-Arch <linux-arch@...r.kernel.org>, linux-kernel@...r.kernel.org
Cc: Rich Felker <dalias@...c.org>, Andreas Larsson <andreas@...sler.com>, 
 guoren <guoren@...nel.org>, Christophe Leroy <christophe.leroy@...roup.eu>,
 "H. Peter Anvin" <hpa@...or.com>, sparclinux@...r.kernel.org,
 linux-s390@...r.kernel.org, Helge Deller <deller@....de>,
 linux-sh@...r.kernel.org, "linux-csky@...r.kernel.org"
 <linux-csky@...r.kernel.org>, "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
  Heiko Carstens <hca@...ux.ibm.com>, "musl@...ts.openwall.com"
 <musl@...ts.openwall.com>, Nicholas Piggin <npiggin@...il.com>, Alexander
 Viro <viro@...iv.linux.org.uk>, LTP List <ltp@...ts.linux.it>, Brian Cain
 <bcain@...cinc.com>, Christian Brauner <brauner@...nel.org>, Thomas
 Bogendoerfer <tsbogend@...ha.franken.de>, Xi Ruoyao
 <libc-alpha@...rceware.org>, linux-parisc@...r.kernel.org,
 linux-mips@...r.kernel.org,  stable@...r.kernel.org,
 linux-hexagon@...r.kernel.org,  linux-fsdevel@...r.kernel.org,
 linuxppc-dev@...ts.ozlabs.org, "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH 09/15] sh: rework sync_file_range ABI

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@...db.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

Powered by blists - more mailing lists