[PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

For both system calls user space passes a signed long length parameter,
while the kernel side takes an unsigned long parameter and converts it
later to signed long again.
As far as I can see there is no point in doing the unsigned -> signed
conversion.
But it has led to bugs in compat wrappers see e.g.
dd90bbd5 "powerpc: Add compat_sys_truncate".
The s390 compat wrappers for these two system calls are broken as well
since they also perform zero extension instead of sign extension for
the length parameter.

In addition if hpa comes up with an automated way of generating
compat wrappers it would generate wrong ones for these two cases.

So change the length parameter from unsigned long to long.

Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/open.c                |    7 +++----
 include/linux/syscalls.h |    5 ++---
 2 files changed, 5 insertions(+), 7 deletions(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c
+++ linux-2.6/fs/open.c
@@ -290,10 +290,9 @@ out:
 	return error;
 }
 
-SYSCALL_DEFINE2(truncate, const char __user *, path, unsigned long, length)
+SYSCALL_DEFINE2(truncate, const char __user *, path, long, length)
 {
-	/* on 32-bit boxen it will cut the range 2^31--2^32-1 off */
-	return do_sys_truncate(path, (long)length);
+	return do_sys_truncate(path, length);
 }
 
 static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
@@ -342,7 +341,7 @@ out:
 	return error;
 }
 
-SYSCALL_DEFINE2(ftruncate, unsigned int, fd, unsigned long, length)
+SYSCALL_DEFINE2(ftruncate, unsigned int, fd, long, length)
 {
 	long ret = do_sys_ftruncate(fd, length, 1);
 	/* avoid REGPARM breakage on x86: */
Index: linux-2.6/include/linux/syscalls.h
===================================================================
--- linux-2.6.orig/include/linux/syscalls.h
+++ linux-2.6/include/linux/syscalls.h
@@ -460,9 +460,8 @@ asmlinkage long sys_mount(char __user *d
 				void __user *data);
 asmlinkage long sys_umount(char __user *name, int flags);
 asmlinkage long sys_oldumount(char __user *name);
-asmlinkage long sys_truncate(const char __user *path,
-				unsigned long length);
-asmlinkage long sys_ftruncate(unsigned int fd, unsigned long length);
+asmlinkage long sys_truncate(const char __user *path, long length);
+asmlinkage long sys_ftruncate(unsigned int fd, long length);
 asmlinkage long sys_stat(char __user *filename,
 			struct __old_kernel_stat __user *statbuf);
 asmlinkage long sys_statfs(const char __user * path,

Re: [PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[Linus Torvalds]

On Wed, 23 Sep 2009, Heiko Carstens wrote:
> 
> For both system calls user space passes a signed long length parameter,
> while the kernel side takes an unsigned long parameter and converts it
> later to signed long again.

No it doesn't.

Look at sys_ftruncate() again. It doesn't convert it to signed long at 
all. It converts it to 'loff_t' which is something different entirely.

Now, it may be that we _should_ convert it to 'long' like your patch does, 
but this is definitely not a "no changes" patch as far as I can tell. It 
limits ftruncate to 31 bits on 32-bit architectures, in ways it didn't use 
to be limited.

[ Note the "small" logic and the interaction with O_LARGEFILE. On a 32-bit 
  architecture, if you open with O_LARGEFILE, ftruncate() gets the full 
  32-bit range, and that's the part your patch broke. ]

So NAK. Not without some serious explanations on why I'm wrong and am just 
being unnecessarily difficult and a pinhead.

			Linus

Re: [PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[Heiko Carstens]

On Wed, Sep 23, 2009 at 08:13:47AM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 23 Sep 2009, Heiko Carstens wrote:
> > 
> > For both system calls user space passes a signed long length parameter,
> > while the kernel side takes an unsigned long parameter and converts it
> > later to signed long again.
> 
> No it doesn't.
> 
> Look at sys_ftruncate() again. It doesn't convert it to signed long at 
> all. It converts it to 'loff_t' which is something different entirely.
> 
> Now, it may be that we _should_ convert it to 'long' like your patch does, 
> but this is definitely not a "no changes" patch as far as I can tell. It 
> limits ftruncate to 31 bits on 32-bit architectures, in ways it didn't use 
> to be limited.
> 
> [ Note the "small" logic and the interaction with O_LARGEFILE. On a 32-bit 
>   architecture, if you open with O_LARGEFILE, ftruncate() gets the full 
>   32-bit range, and that's the part your patch broke. ]
> 
> So NAK. Not without some serious explanations on why I'm wrong and am just 
> being unnecessarily difficult and a pinhead.

Yes, you're right. That would be a user space visible change. I was too much
focused on the man page + the power pc patch.
So it's only the truncate part that would make sense:

Subject: [PATCH] fs: change sys_truncate length parameter type

From: Heiko Carstens <heiko.carstens@de.ibm.com>

For this system call user space passes a signed long length parameter,
while the kernel side takes an unsigned long parameter and converts it
later to signed long again.
As far as I can see there is no point in doing the unsigned -> signed
conversion.
But it has led to bugs in compat wrappers see e.g.
dd90bbd5 "powerpc: Add compat_sys_truncate".
The s390 compat wrapper for this functions is broken as well
since it also performs zero extension instead of sign extension
for the length parameter.

In addition if hpa comes up with an automated way of generating
compat wrappers it would generate a wrong one here.

So change the length parameter from unsigned long to long.

Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/open.c                |    5 ++---
 include/linux/syscalls.h |    3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c
+++ linux-2.6/fs/open.c
@@ -290,10 +290,9 @@ out:
 	return error;
 }
 
-SYSCALL_DEFINE2(truncate, const char __user *, path, unsigned long, length)
+SYSCALL_DEFINE2(truncate, const char __user *, path, long, length)
 {
-	/* on 32-bit boxen it will cut the range 2^31--2^32-1 off */
-	return do_sys_truncate(path, (long)length);
+	return do_sys_truncate(path, length);
 }
 
 static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
Index: linux-2.6/include/linux/syscalls.h
===================================================================
--- linux-2.6.orig/include/linux/syscalls.h
+++ linux-2.6/include/linux/syscalls.h
@@ -460,8 +460,7 @@ asmlinkage long sys_mount(char __user *d
 				void __user *data);
 asmlinkage long sys_umount(char __user *name, int flags);
 asmlinkage long sys_oldumount(char __user *name);
-asmlinkage long sys_truncate(const char __user *path,
-				unsigned long length);
+asmlinkage long sys_truncate(const char __user *path, long length);
 asmlinkage long sys_ftruncate(unsigned int fd, unsigned long length);
 asmlinkage long sys_stat(char __user *filename,
 			struct __old_kernel_stat __user *statbuf);

Re: [PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[Heiko Carstens]

On Wed, Sep 23, 2009 at 08:13:47AM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 23 Sep 2009, Heiko Carstens wrote:
> > 
> > For both system calls user space passes a signed long length parameter,
> > while the kernel side takes an unsigned long parameter and converts it
> > later to signed long again.
> 
> No it doesn't.
> 
> Look at sys_ftruncate() again. It doesn't convert it to signed long at 
> all. It converts it to 'loff_t' which is something different entirely.
> 
> Now, it may be that we _should_ convert it to 'long' like your patch does, 
> but this is definitely not a "no changes" patch as far as I can tell. It 
> limits ftruncate to 31 bits on 32-bit architectures, in ways it didn't use 
> to be limited.
> 
> [ Note the "small" logic and the interaction with O_LARGEFILE. On a 32-bit 
>   architecture, if you open with O_LARGEFILE, ftruncate() gets the full 
>   32-bit range, and that's the part your patch broke. ]
> 
> So NAK. Not without some serious explanations on why I'm wrong and am just 
> being unnecessarily difficult and a pinhead.

Yes, you're right. That would be a user space visible change. I was too much
focused on the man page + the power pc patch.
So it's only the truncate part that would make sense:

Subject: [PATCH] fs: change sys_truncate length parameter type

From: Heiko Carstens <heiko.carstens@de.ibm.com>

For this system call user space passes a signed long length parameter,
while the kernel side takes an unsigned long parameter and converts it
later to signed long again.
As far as I can see there is no point in doing the unsigned -> signed
conversion.
But it has led to bugs in compat wrappers see e.g.
dd90bbd5 "powerpc: Add compat_sys_truncate".
The s390 compat wrapper for this functions is broken as well
since it also performs zero extension instead of sign extension
for the length parameter.

In addition if hpa comes up with an automated way of generating
compat wrappers it would generate a wrong one here.

So change the length parameter from unsigned long to long.

Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/open.c                |    5 ++---
 include/linux/syscalls.h |    3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c
+++ linux-2.6/fs/open.c
@@ -290,10 +290,9 @@ out:
 	return error;
 }
 
-SYSCALL_DEFINE2(truncate, const char __user *, path, unsigned long, length)
+SYSCALL_DEFINE2(truncate, const char __user *, path, long, length)
 {
-	/* on 32-bit boxen it will cut the range 2^31--2^32-1 off */
-	return do_sys_truncate(path, (long)length);
+	return do_sys_truncate(path, length);
 }
 
 static long do_sys_ftruncate(unsigned int fd, loff_t length, int small)
Index: linux-2.6/include/linux/syscalls.h
===================================================================
--- linux-2.6.orig/include/linux/syscalls.h
+++ linux-2.6/include/linux/syscalls.h
@@ -460,8 +460,7 @@ asmlinkage long sys_mount(char __user *d
 				void __user *data);
 asmlinkage long sys_umount(char __user *name, int flags);
 asmlinkage long sys_oldumount(char __user *name);
-asmlinkage long sys_truncate(const char __user *path,
-				unsigned long length);
+asmlinkage long sys_truncate(const char __user *path, long length);
 asmlinkage long sys_ftruncate(unsigned int fd, unsigned long length);
 asmlinkage long sys_stat(char __user *filename,
 			struct __old_kernel_stat __user *statbuf);

Re: [PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Look at sys_ftruncate() again. It doesn't convert it to signed long at 
> all. It converts it to 'loff_t' which is something different entirely.
> 
> Now, it may be that we _should_ convert it to 'long' like your patch does, 
> but this is definitely not a "no changes" patch as far as I can tell. It 
> limits ftruncate to 31 bits on 32-bit architectures, in ways it didn't use 
> to be limited.
> 
> [ Note the "small" logic and the interaction with O_LARGEFILE. On a 32-bit 
>   architecture, if you open with O_LARGEFILE, ftruncate() gets the full 
>   32-bit range, and that's the part your patch broke. ]
> 

I would argue that the current behavior is, if not incorrect (we define 
our own ABI after all) at least unwanted (we want our ABI to be as close 
as practical to the POSIX API.)

In particular, at the API level, both truncate() and ftruncate does take 
a signed (off_t) parameter, and the POSIX spec states:

        The ftruncate() function shall fail if:

        EINVAL The length argument was less than 0.

So yes, it's a change, but it seems to be one which probably should be done.

> In addition if hpa comes up with an automated way of generating
> compat wrappers it would generate wrong ones for these two cases.

I really mean to try to shake free some time and do that project.  I 
don't think it will happen before Kernel Summit, though :(

	-hpa

Re: [PATCH] fs: change sys_truncate/sys_ftruncate length parameter type

[Linus Torvalds]

On Wed, 23 Sep 2009, H. Peter Anvin wrote:
> 
> I would argue that the current behavior is, if not incorrect (we define our
> own ABI after all) at least unwanted (we want our ABI to be as close as
> practical to the POSIX API.)

I think we care a WHOLE LOT more about binary compatibility than we care 
about some POSIX case for a braindamaged and fundamentally broken system 
call anyway.

The thing is, ftruncate() should take loff_t these days, so the old POSIX 
language is about a pointless system call where the _only_ thing that 
matters is legacy - and that legacy trumps any POSIX concerns.

			Linus