[PATCH] compat: fix compat_sys_openat and friends

[PATCH] compat: fix compat_sys_openat and friends

[Stephen Rothwell]

Most of the 64 bit architectures will zero extend the first argument to
compat_sys_{openat,newfstatat,futimesat} which will fail if the 32 bit
syscall was passed AT_FDCWD (which is a small negative number).  Declare
the first argument to be an unsigned int which will force the correct
sign extension when the internal functions are called in each case.

Also, do some small white space cleanups in fs/compat.c.

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---

 fs/compat.c              |   12 ++++++------
 include/linux/syscalls.h |    6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

This has been built on powerpc (and does the neede sign extension)
but not tested as we have yet to wire up the syscalls there.

Also, we will need compat wrappers for the other *at syscalls for
the same reason (sign extension of the first argument).

Also, I am wondering if we should move the declarations of the compat
syscalls to linux/compat.h (where all previous compat syscalls are
declated).
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

13be12a8986f1e178e20f80b94e2fab7c46bc5fe
diff --git a/fs/compat.c b/fs/compat.c
index cc58a20..70c5af4 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -73,17 +73,17 @@ asmlinkage long compat_sys_utime(char __
 	return do_utimes(AT_FDCWD, filename, t ? tv : NULL);
 }
 
-asmlinkage long compat_sys_futimesat(int dfd, char __user *filename, struct compat_timeval __user *t)
+asmlinkage long compat_sys_futimesat(unsigned int dfd, char __user *filename, struct compat_timeval __user *t)
 {
 	struct timeval tv[2];
 
-	if (t) { 
+	if (t) {
 		if (get_user(tv[0].tv_sec, &t[0].tv_sec) ||
 		    get_user(tv[0].tv_usec, &t[0].tv_usec) ||
 		    get_user(tv[1].tv_sec, &t[1].tv_sec) ||
 		    get_user(tv[1].tv_usec, &t[1].tv_usec))
-			return -EFAULT; 
-	} 
+			return -EFAULT;
+	}
 	return do_utimes(dfd, filename, t ? tv : NULL);
 }
 
@@ -114,7 +114,7 @@ asmlinkage long compat_sys_newlstat(char
 	return error;
 }
 
-asmlinkage long compat_sys_newfstatat(int dfd, char __user *filename,
+asmlinkage long compat_sys_newfstatat(unsigned int dfd, char __user *filename,
 		struct compat_stat __user *statbuf, int flag)
 {
 	struct kstat stat;
@@ -1326,7 +1326,7 @@ compat_sys_open(const char __user *filen
  * O_LARGEFILE flag.
  */
 asmlinkage long
-compat_sys_openat(int dfd, const char __user *filename, int flags, int mode)
+compat_sys_openat(unsigned int dfd, const char __user *filename, int flags, int mode)
 {
 	return do_sys_open(dfd, filename, flags, mode);
 }
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index fdbd436..3877209 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -559,12 +559,12 @@ asmlinkage long sys_newfstatat(int dfd, 
 			       struct stat __user *statbuf, int flag);
 asmlinkage long sys_readlinkat(int dfd, const char __user *path, char __user *buf,
 			       int bufsiz);
-asmlinkage long compat_sys_futimesat(int dfd, char __user *filename,
+asmlinkage long compat_sys_futimesat(unsigned int dfd, char __user *filename,
 				     struct compat_timeval __user *t);
-asmlinkage long compat_sys_newfstatat(int dfd, char __user * filename,
+asmlinkage long compat_sys_newfstatat(unsigned int dfd, char __user * filename,
 				      struct compat_stat __user *statbuf,
 				      int flag);
-asmlinkage long compat_sys_openat(int dfd, const char __user *filename,
+asmlinkage long compat_sys_openat(unsigned int dfd, const char __user *filename,
 				   int flags, int mode);
 
 #endif
-- 
1.1.5

Re: [PATCH] compat: fix compat_sys_openat and friends

[Linus Torvalds]

On Thu, 2 Feb 2006, Stephen Rothwell wrote:
>
> Most of the 64 bit architectures will zero extend the first argument to
> compat_sys_{openat,newfstatat,futimesat} which will fail if the 32 bit
> syscall was passed AT_FDCWD (which is a small negative number).  Declare
> the first argument to be an unsigned int which will force the correct
> sign extension when the internal functions are called in each case.

Umm.

Wouldn't it be _much_ better to declare the argument as a "long", since 
some architectures (alpha, for example) may assume that 32-bit arguments 
have been _sign_extended, not zero-extended.

Then, when the "compat_sys_xxxx()" function passes the "long" down to the 
_real_ function (which takes an "int"), those architectures (and only 
those architectures) that actually have assumptions about high bits will 
have the compiler automatically do the right zero- or sign-extensions at 
that call-site.

		Linus

Re: [PATCH] compat: fix compat_sys_openat and friends

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Wed, 1 Feb 2006 21:36:40 -0800 (PST)

> Wouldn't it be _much_ better to declare the argument as a "long", since 
> some architectures (alpha, for example) may assume that 32-bit arguments 
> have been _sign_extended, not zero-extended.
> 
> Then, when the "compat_sys_xxxx()" function passes the "long" down to the 
> _real_ function (which takes an "int"), those architectures (and only 
> those architectures) that actually have assumptions about high bits will 
> have the compiler automatically do the right zero- or sign-extensions at 
> that call-site.

There is the convention that for the compat system calls all the args
will be 32-bit zero extended by the platform syscall entry code before
the C code is invoked.  This topic used to come up a lot and finally
we all decided that was the thing to do.

It's important (at least I think so :-) for all of this generic compat
code to be able to have a well defined argument environment.

Anyways, I think that's how Stephen arrived at his patch.

Re: [PATCH] compat: fix compat_sys_openat and friends

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

On Wed, 01 Feb 2006 21:56:44 -0800 (PST) "David S. Miller" <davem@davemloft.net> wrote:
>
> From: Linus Torvalds <torvalds@osdl.org>
> Date: Wed, 1 Feb 2006 21:36:40 -0800 (PST)
> 
> > Wouldn't it be _much_ better to declare the argument as a "long", since 
> > some architectures (alpha, for example) may assume that 32-bit arguments 
> > have been _sign_extended, not zero-extended.
> > 
> > Then, when the "compat_sys_xxxx()" function passes the "long" down to the 
> > _real_ function (which takes an "int"), those architectures (and only 
> > those architectures) that actually have assumptions about high bits will 
> > have the compiler automatically do the right zero- or sign-extensions at 
> > that call-site.
> 
> There is the convention that for the compat system calls all the args
> will be 32-bit zero extended by the platform syscall entry code before
> the C code is invoked.  This topic used to come up a lot and finally
> we all decided that was the thing to do.
> 
> It's important (at least I think so :-) for all of this generic compat
> code to be able to have a well defined argument environment.
> 
> Anyways, I think that's how Stephen arrived at his patch.

Yes, that is it.  I have tried using "long" and "unsigned int" for those
first parameters and it produces exactly the same assembler output on
ppc64 and x86_64.  Everywhere else that we have a file descriptor argument
to a compat syscall function it is declared "unsigned int".

And for these compat functions, alpha is irrelevent of course. :-)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] compat: fix compat_sys_openat and friends

[Linus Torvalds]

On Thu, 2 Feb 2006, Stephen Rothwell wrote:
> 
> Yes, that is it.  I have tried using "long" and "unsigned int" for those
> first parameters and it produces exactly the same assembler output on
> ppc64 and x86_64.  Everywhere else that we have a file descriptor argument
> to a compat syscall function it is declared "unsigned int".
> 
> And for these compat functions, alpha is irrelevent of course. :-)

Fair enough. Applied.

		Linus

Re: [PATCH] compat: fix compat_sys_openat and friends

[Ralf Baechle]

On Wed, Feb 01, 2006 at 09:36:40PM -0800, Linus Torvalds wrote:

> > Most of the 64 bit architectures will zero extend the first argument to
> > compat_sys_{openat,newfstatat,futimesat} which will fail if the 32 bit
> > syscall was passed AT_FDCWD (which is a small negative number).  Declare
> > the first argument to be an unsigned int which will force the correct
> > sign extension when the internal functions are called in each case.
> 
> Umm.
> 
> Wouldn't it be _much_ better to declare the argument as a "long", since 
> some architectures (alpha, for example) may assume that 32-bit arguments 
> have been _sign_extended, not zero-extended.

> Then, when the "compat_sys_xxxx()" function passes the "long" down to the 
> _real_ function (which takes an "int"), those architectures (and only 
> those architectures) that actually have assumptions about high bits will 
> have the compiler automatically do the right zero- or sign-extensions at 
> that call-site.

MIPS is one of those architectures where variables are always held
sign-extended to the full 32-bit or 64-bit size of the register.  The
signed-ness of the C data type doesn't matter at all here so Stephen's
change is a nop for MIPS.

  Ralf