compat syscall args

Arnd asked:

Here is the issue. In the sparc64 C calling conventions, it is assumed that 32-bit signed values are sign extended by the caller.

This means that, at syscall invocation time, we have to choose between either:

1) sign extending all syscall args for the C code, then explicitly zero-extending all non-signed syscall args. This would require the most amount of compat layer code help.

2) zero extending all syscall args for the C code, then expliticly sign-extending all signed syscall args.

3) some mixture of 1 and 2

#3 is what sparc64 does, it hits the highest number of system call arguments correctly. Specifically we:

arg0: zero-extend
arg1: zero-extend
arg2: zero-extend
arg3: zero-extend
arg4: leave as-is
arg5: leave as-is

I remember discussing this with Andi Kleen before.

Each platform is going to behave differently in this area, so I suppose the right thing to do really is to have the arch specific code use little zero/sign extender stubs when necessary so that the compat layer can assume that the args are properly sign/zero extended already. I guess this is how I'll fix this up on sparc64 for now.

Comments?

As it turns out we're taking care of this already via stubs in arch/sparc64/kernel/sys32.S, I just need to add them for select and futex.

Here is how I'm going to fix this on sparc64 therefore.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/05/29 12:27:54-07:00 [email protected]
# [SPARC64]: compat select and futex need %o4 zero-extended.
#
# arch/sparc64/kernel/systbls.S
# 2004/05/29 12:27:36-07:00 [email protected] +3 -3
# [SPARC64]: compat select and futex need %o4 zero-extended.
#
# arch/sparc64/kernel/sys32.S
# 2004/05/29 12:27:36-07:00 [email protected] +13 -0
# [SPARC64]: compat select and futex need %o4 zero-extended.
#

diff -Nru a/arch/sparc64/kernel/sys32.S b/arch/sparc64/kernel/sys32.S
@@ -6,6 +6,7 @@
  * Copyright (C) 1998 Jakub Jelinek   ([email protected])
  */
 
+#include <linux/config.h>
 #include <asm/errno.h>
 
 /* NOTE: call as jump breaks return stack, we have to avoid that */

@@ -77,6 +78,18 @@
 sys32_mq_timedreceive:
 	sethi		%hi(compat_sys_mq_timedreceive), %g1
 	jmpl		%g1 + %lo(compat_sys_mq_timedreceive), %g0
+	 srl		%o4, 0, %o4
+
+	.globl		sys32_select
+sys32_select:
+	sethi		%hi(compat_sys_select), %g1
+	jmpl		%g1 + %lo(compat_sys_select), %g0
+	 srl		%o4, 0, %o4
+
+	.globl		sys32_futex
+sys32_futex:
+	sethi		%hi(compat_sys_futex), %g1
+	jmpl		%g1 + %lo(compat_sys_futex), %g0
 	 srl		%o4, 0, %o4
 
 	.align		32

diff -Nru a/arch/sparc64/kernel/systbls.S b/arch/sparc64/kernel/systbls.S
@@ -37,7 +37,7 @@
 	.word sys_madvise, sys_vhangup, sys32_truncate64, sys_mincore, sys32_getgroups16
 /*80*/	.word sys32_setgroups16, sys_getpgrp, sys_setgroups, compat_sys_setitimer, sys32_ftruncate64
 	.word sys_swapon, compat_sys_getitimer, sys_setuid, sys_sethostname, sys_setgid
-/*90*/	.word sys_dup2, sys_setfsuid, compat_sys_fcntl, compat_sys_select, sys_setfsgid
+/*90*/	.word sys_dup2, sys_setfsuid, compat_sys_fcntl, sys32_select, sys_setfsgid
 	.word sys_fsync, sys_setpriority32, sys_nis_syscall, sys_nis_syscall, sys_nis_syscall
 /*100*/ .word sys_getpriority, sys32_rt_sigreturn, sys32_rt_sigaction, sys32_rt_sigprocmask, sys32_rt_sigpending
 	.word sys32_rt_sigtimedwait, sys32_rt_sigqueueinfo, sys32_rt_sigsuspend, sys_setresuid, sys_getresuid

@@ -47,7 +47,7 @@
 	.word sys_nis_syscall, sys32_setreuid16, sys32_setregid16, sys_rename, sys_truncate
 /*130*/	.word sys_ftruncate, sys_flock, sys_lstat64, sys_nis_syscall, sys_nis_syscall
 	.word sys_nis_syscall, sys_mkdir, sys_rmdir, sys32_utimes, sys_stat64
-/*140*/	.word sys32_sendfile64, sys_nis_syscall, compat_sys_futex, sys_gettid, compat_sys_getrlimit
+/*140*/	.word sys32_sendfile64, sys_nis_syscall, sys32_futex, sys_gettid, compat_sys_getrlimit
 	.word compat_sys_setrlimit, sys_pivot_root, sys32_prctl, sys32_pciconfig_read, sys32_pciconfig_write
 /*150*/	.word sys_nis_syscall, sys_nis_syscall, sys_nis_syscall, sys_poll, sys_getdents64
 	.word compat_sys_fcntl64, sys_ni_syscall, compat_sys_statfs, compat_sys_fstatfs, sys_oldumount

@@ -65,7 +65,7 @@
 	.word sys32_ipc, sys32_sigreturn, sys_clone, sys_nis_syscall, sys32_adjtimex
 /*220*/	.word compat_sys_sigprocmask, sys_ni_syscall, sys32_delete_module, sys_ni_syscall, sys_getpgid
 	.word sys32_bdflush, sys32_sysfs, sys_nis_syscall, sys32_setfsuid16, sys32_setfsgid16
-/*230*/	.word compat_sys_select, sys_time, sys_nis_syscall, sys_stime, compat_statfs64
+/*230*/	.word sys32_select, sys_time, sys_nis_syscall, sys_stime, compat_statfs64
 	.word compat_fstatfs64, sys_llseek, sys_mlock, sys_munlock, sys_mlockall
 /*240*/	.word sys_munlockall, sys_sched_setparam, sys_sched_getparam, sys_sched_setscheduler, sys_sched_getscheduler
 	.word sys_sched_yield, sys_sched_get_priority_max, sys_sched_get_priority_min, sys32_sched_rr_get_interval, compat_sys_nanosleep

Ah, thanks for solving this mystery.

select and futex may not be the only ones that need to be fixed for sparc64. I grepped through the list of syscalls that you call without a sys32.S wrapper and found these that take at least five arguments:

compat_sys_io_getevents, compat_sys_mount, sys_fsetxattr, sys_setsockopt, sys_llseek, sys_mremap, sys_remap_file_pages, sys_setxattr, sys_lsetxattr, sys_fsetxattr, sys32_fadvise64, sys32_fadvise64_64, sys32_ipc, sys32_mremap, sys32_pciconfig_read, sys32_pciconfig_write, sys32_prctl, sys32_pread64, sys32_pwrite64, sys32_rt_sigaction

I did not check which ones are safe anyway, but you probably want to go through that list.

	Arnd <><

Hi Dave,

Yeah, you and Andi and I (and others, I think) had this discussion, but it ended like this:

--------------------
Date: Thu, 13 Jun 2002 22:56:52 -0700 (PDT)
Message-Id: <[email protected]>
To: [email protected]
Cc: [email protected], [email protected], [email protected],
[email protected], [email protected], [email protected]
Subject: Re: [PATCH] Consolidate sys32_utime
From: "David S. Miller" <[email protected]>

From: Andi Kleen <[email protected]>
Date: Fri, 14 Jun 2002 07:53:10 +0200

I've had good experiences with zero extension at least so far. Would be probably good just for consistency.

So be it, the convention is that all arguments are zero extended from 32-bits to 64-bits when the syscall is invoked.
----------------------------------------------------------------------

Did something change along the way?

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

I wasn't aware of the convention but it absolutely makes sense. If I find the time, I'll go through the existing compat_sys_* handlers to see if they all do sign-extension correctly. Note that on s390, we also need 31-bit zero-extension for pointers, which is done in architecture-specific code. Otherwise every compat_* syscall would need to use compat_uptr_t arguments which appears unnecessarily ugly to me.

Also, what should be the conversion for positive signed arguments like the futex 'op' value? Sign-extension would be the formally correct solution, but simply using the zero-extended value (like we do in most places) works just as well.

	Arnd <><

Personally I think it makes more sense to do what sparc64 does which is:

1) The syscall32 entry code extends each arg in a fixed way. Ie. arg0-3 are zero-extended, arg4-5 are sign-extended or whatever. I posted the choices we use on sparc64 just the other day.

2) For each syscall where this default set of extensions is not correct, little assembler or C stubs are used to correct the extensions made by the default code.

It is the most optimal solution. We only need 13 or so stubs on sparc64 with the defaults we've choosen.