[tile] sys_llseek() can *not* be used as compat_sys_llseek() there

[tile] sys_llseek() can *not* be used as compat_sys_llseek() there

[Al Viro]

	Unless I'm seriously misreading your code, you have all arguments of
compat syscall sign-extended by asm glue.  If that's the case, consider
what will happen to 32bit binary doing
	llseek(fd, 0, 0xffffffff, &pos, SEEK_CUR)
on 32bit and 64bit hosts resp.  The former will move the current position
by 4Gb forward; the latter - by one byte backwards...

	sys_llseek() will do the right thing for compat on architectures that
zero-extend the arguments of compat syscall; x86/sparc/ppc/arm64 fall into
that category.  mips and (AFAICS) tile do not (and s390 is just plain weird
and needs wrappers for just about everything).  mips has sys_32_llseek() for
a good reason; we probably want to take that into fs/read_write.c, but
on such architectures something *is* needed - plain sys_llseek() won't work...

	FWIW, I suspect that we want __ARCH_COMPAT_ZERO_EXTENDS or
__ARCH_COMPAT_SIGN_EXTENDS defined in asm/compat.h; the need of sys_llseek()
would be "it's 32bit or it has compat and it's zero-extending", while
"has compat and it's not zero-extending" would pick compat_sys_llseek()
instead.  __ARCH_WANT_SYS_LLSEEK shouldn't exist...

	And things like "do we need compat_sys_lseek(), compat_sys_truncate()
and their ilk" could be expressed via those as well - compat && !sign_extends.

	Comments?

Re: [tile] sys_llseek() can *not* be used as compat_sys_llseek() there

[Chris Metcalf]

On 2/24/2013 7:29 PM, Al Viro wrote:
> 	Unless I'm seriously misreading your code, you have all arguments of
> compat syscall sign-extended by asm glue.  If that's the case, consider
> what will happen to 32bit binary doing
> 	llseek(fd, 0, 0xffffffff, &pos, SEEK_CUR)
> on 32bit and 64bit hosts resp.  The former will move the current position
> by 4Gb forward; the latter - by one byte backwards...

Without testing this (I'm in an airport), I think you might be right.  But the fix seems like it might just be changing the parameters of the llseek syscall in read_write.c to be "unsigned int" for offset_high and offset_low.  That would let the usual syscall-wrappers code properly zero-extend the high bits.

> 	sys_llseek() will do the right thing for compat on architectures that
> zero-extend the arguments of compat syscall; x86/sparc/ppc/arm64 fall into
> that category.  mips and (AFAICS) tile do not (and s390 is just plain weird
> and needs wrappers for just about everything).  mips has sys_32_llseek() for
> a good reason; we probably want to take that into fs/read_write.c, but
> on such architectures something *is* needed - plain sys_llseek() won't work...
>
> 	FWIW, I suspect that we want __ARCH_COMPAT_ZERO_EXTENDS or
> __ARCH_COMPAT_SIGN_EXTENDS defined in asm/compat.h; the need of sys_llseek()
> would be "it's 32bit or it has compat and it's zero-extending", while
> "has compat and it's not zero-extending" would pick compat_sys_llseek()
> instead.  __ARCH_WANT_SYS_LLSEEK shouldn't exist...
>
> 	And things like "do we need compat_sys_lseek(), compat_sys_truncate()
> and their ilk" could be expressed via those as well - compat && !sign_extends.
>
> 	Comments?

Cleanups like these sound generally good.  For tilegx32, for good or bad, we emulated the register allocation conventions of our 32-bit tilepro architecture, so that for example truncate64 has r0 = filename, r1 = dummy, r2+r3 = offset.  This doesn't match what the compiler would generate for a simple "char *, long long" argument pair, since for tilegx32 we (obviously) represent long long as a 64-bit register value in general.  But for new architectures it might be good to look to make changes like those you are suggesting.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [tile] sys_llseek() can *not* be used as compat_sys_llseek() there

[Al Viro]

On Wed, Feb 27, 2013 at 01:46:09PM -0500, Chris Metcalf wrote:
> On 2/24/2013 7:29 PM, Al Viro wrote:
> > 	Unless I'm seriously misreading your code, you have all arguments of
> > compat syscall sign-extended by asm glue.  If that's the case, consider
> > what will happen to 32bit binary doing
> > 	llseek(fd, 0, 0xffffffff, &pos, SEEK_CUR)
> > on 32bit and 64bit hosts resp.  The former will move the current position
> > by 4Gb forward; the latter - by one byte backwards...
> 
> Without testing this (I'm in an airport), I think you might be right.  But the fix seems like it might just be changing the parameters of the llseek syscall in read_write.c to be "unsigned int" for offset_high and offset_low.  That would let the usual syscall-wrappers code properly zero-extend the high bits.

... and screw binary compatibility on 64bit architectures that have llseek(2).
I also thought that there's no such thing - after all, any such architecture
has perfectly usable lseek(2).  No such luck - parisc64, ppc64, sparc64, s390x
and sh64 all have both of those.  AFAICS, glibc does not use llseek(2) on any
of those and any userland code directly using that syscall would be rather
dumb, but then this is precisely the sort of userland code likely to screw
it up.

I don't know; it's a borderline case, but strictly speaking we would be
breaking syscall ABI compatibility on those architectures by doing that.
OTOH, the real rule is more squishy - "don't break real userland code",
so...  Hell knows.

Linus, do you have any comments on that proposal?  Basically, that would make
sys_llseek() ignore upper 32 bits of offset_low on 64bit targets.  Benefit:
fixes bug in tile compat, removes the need of compat wrapper on mips, slightly
simpler rules for populating compat syscall tables.  Cost: changes behaviour
of llseek(2) for 64bit binaries on parisc, ppc, sparc, s390, sh64.  AFAIK,
glibc ignores that syscall on those targets, so affected userland would have
to do it manually via syscall(__NR_llseek,...).

TBH, I'd rather solve it by providing a mips-style wrapper for tile, but...

[PATCH] tile: work around bug in the generic sys_llseek

[Chris Metcalf]

sys_llseek should specify the high and low 32-bit seek values as "unsigned
int" but instead it specifies "unsigned long".  Since compat syscall
arguments are always sign-extended on tile, this means that a seek value
of 0xffffffff will be incorrectly interpreted as a value of -1ULL.

To avoid the risk of breaking binary compatibility on architectures
that already use sys_llseek this way, we follow the same path as MIPS
and provide a wrapper override.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
Cc: stable@kernel.org [v3.6 onwards]
---
Al Viro suggested changing the generic implementation but there has
been no followup, and his preference was just fixing tile anyway,
so this is what this patch does.  Al, can you ack this if it looks OK?
The generated code for compat_sys_llseek just clears the high 32 bits
of the two offset parameters and jumps to sys_llseek.

 arch/tile/include/asm/compat.h |    3 +++
 arch/tile/kernel/compat.c      |   13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/tile/include/asm/compat.h b/arch/tile/include/asm/compat.h
index 001d418..78f1f2d 100644
--- a/arch/tile/include/asm/compat.h
+++ b/arch/tile/include/asm/compat.h
@@ -288,6 +288,9 @@ long compat_sys_sync_file_range2(int fd, unsigned int flags,
 long compat_sys_fallocate(int fd, int mode,
 			  u32 offset_lo, u32 offset_hi,
 			  u32 len_lo, u32 len_hi);
+long compat_sys_llseek(unsigned int fd, unsigned int offset_high,
+		       unsigned int offset_low, loff_t __user * result,
+		       unsigned int origin);
 
 /* Assembly trampoline to avoid clobbering r0. */
 long _compat_sys_rt_sigreturn(void);
diff --git a/arch/tile/kernel/compat.c b/arch/tile/kernel/compat.c
index 7f72401..69034e2 100644
--- a/arch/tile/kernel/compat.c
+++ b/arch/tile/kernel/compat.c
@@ -76,6 +76,18 @@ long compat_sys_fallocate(int fd, int mode,
 			     ((loff_t)len_hi << 32) | len_lo);
 }
 
+/*
+ * Avoid bug in generic sys_llseek() that specifies offset_high and
+ * offset_low as "unsigned long", thus making it possible to pass
+ * a sign-extended high 32 bits in offset_low.
+ */
+long compat_sys_llseek(unsigned int fd, unsigned int offset_high,
+		       unsigned int offset_low, loff_t __user * result,
+		       unsigned int origin)
+{
+	return sys_llseek(fd, offset_high, offset_low, result, origin);
+}
+ 
 /* Provide the compat syscall number to call mapping. */
 #undef __SYSCALL
 #define __SYSCALL(nr, call) [nr] = (call),
@@ -83,6 +95,7 @@ long compat_sys_fallocate(int fd, int mode,
 /* See comments in sys.c */
 #define compat_sys_fadvise64_64 sys32_fadvise64_64
 #define compat_sys_readahead sys32_readahead
+#define sys_llseek compat_sys_llseek
 
 /* Call the assembly trampolines where necessary. */
 #define compat_sys_rt_sigreturn _compat_sys_rt_sigreturn
-- 
1.7.10.3

Re: [PATCH] tile: work around bug in the generic sys_llseek

[Al Viro]

On Mon, Mar 04, 2013 at 11:19:09AM -0500, Chris Metcalf wrote:
> sys_llseek should specify the high and low 32-bit seek values as "unsigned
> int" but instead it specifies "unsigned long".  Since compat syscall
> arguments are always sign-extended on tile, this means that a seek value
> of 0xffffffff will be incorrectly interpreted as a value of -1ULL.
> 
> To avoid the risk of breaking binary compatibility on architectures
> that already use sys_llseek this way, we follow the same path as MIPS
> and provide a wrapper override.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> Cc: stable@kernel.org [v3.6 onwards]
> ---
> Al Viro suggested changing the generic implementation but there has
> been no followup, and his preference was just fixing tile anyway,
> so this is what this patch does.  Al, can you ack this if it looks OK?
> The generated code for compat_sys_llseek just clears the high 32 bits
> of the two offset parameters and jumps to sys_llseek.

That's not enough - the reason why mips variant works is that they
use SYSCALL_DEFINE, which casts the sucker down to unsigned int.  Simply
declaring it as something that takes unsigned int will just tell the
compiler that upper bits of register are all zero, so it's free to pass
the damn thing unchanged.

Explicit cast down (for old versions) or use of COMPAT_SYSCALL_DEFINE would
work; longer term I'd prefer to have something (select or explicit defines
in asm/compat.h) telling whether we have a sign-extending or zero-extending
biarch and have that COMPAT_SYSCALL_DEFINE in fs/read_write.c, under ifdef
for arch being a sign-extender, but that's the next cycle fodder and can
be done on top of minimal (and easier backportable) fix.

Re: [PATCH] tile: work around bug in the generic sys_llseek

[Chris Metcalf]

On 3/4/2013 11:54 AM, Al Viro wrote:
> On Mon, Mar 04, 2013 at 11:19:09AM -0500, Chris Metcalf wrote:
>> sys_llseek should specify the high and low 32-bit seek values as "unsigned
>> int" but instead it specifies "unsigned long".  Since compat syscall
>> arguments are always sign-extended on tile, this means that a seek value
>> of 0xffffffff will be incorrectly interpreted as a value of -1ULL.
>>
>> To avoid the risk of breaking binary compatibility on architectures
>> that already use sys_llseek this way, we follow the same path as MIPS
>> and provide a wrapper override.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
>> Cc: stable@kernel.org [v3.6 onwards]
>> ---
>> Al Viro suggested changing the generic implementation but there has
>> been no followup, and his preference was just fixing tile anyway,
>> so this is what this patch does.  Al, can you ack this if it looks OK?
>> The generated code for compat_sys_llseek just clears the high 32 bits
>> of the two offset parameters and jumps to sys_llseek.
> That's not enough - the reason why mips variant works is that they
> use SYSCALL_DEFINE, which casts the sucker down to unsigned int.  Simply
> declaring it as something that takes unsigned int will just tell the
> compiler that upper bits of register are all zero, so it's free to pass
> the damn thing unchanged.

No, it actually works correctly for tile.  The reason is that "unsigned int" is represented, counter-intuitively, as a sign-extended 32-bit value.  (This lets us reuse some 64-bit comparison instructions for compat mode.)  So the compiler knows that if it is passing an unsigned int value to a function taking unsigned long, it must zero the high 32 bits explicitly.  I had the same concern about bypassing the SYSCALL_WRAPPER stuff, so I double-checked the generated code, and it's correct.

> Explicit cast down (for old versions) or use of COMPAT_SYSCALL_DEFINE would
> work; longer term I'd prefer to have something (select or explicit defines
> in asm/compat.h) telling whether we have a sign-extending or zero-extending
> biarch and have that COMPAT_SYSCALL_DEFINE in fs/read_write.c, under ifdef
> for arch being a sign-extender, but that's the next cycle fodder and can
> be done on top of minimal (and easier backportable) fix.

Yes, all that said, we should be using COMPAT_SYSCALL_DEFINE here.  Unfortunately it didn't exist in the code base where we first did the compat work (2.6.38) and I wasn't aware it had been added.  I'll post a followup patch switching things over to use COMPAT_SYSCALL_DEFINE, but I'd like to leave this patch as it is, since it makes it easier to backport the bug fix to previous releases.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

[PATCH] tile: properly use COMPAT_SYSCALL_DEFINEx

[Chris Metcalf]

This was pointed out by Al Viro.  Using the correct wrappers
properly does sign extension as necessary on syscall arguments.

Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
---
 arch/tile/kernel/compat.c |   35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/arch/tile/kernel/compat.c b/arch/tile/kernel/compat.c
index 69034e2..6ea4cdb 100644
--- a/arch/tile/kernel/compat.c
+++ b/arch/tile/kernel/compat.c
@@ -32,45 +32,48 @@
  * adapt the usual convention.
  */
 
-long compat_sys_truncate64(char __user *filename, u32 dummy, u32 low, u32 high)
+COMPAT_SYSCALL_DEFINE4(truncate64, char __user *, filename, u32, dummy,
+                       u32, low, u32, high)
 {
 	return sys_truncate(filename, ((loff_t)high << 32) | low);
 }
 
-long compat_sys_ftruncate64(unsigned int fd, u32 dummy, u32 low, u32 high)
+COMPAT_SYSCALL_DEFINE4(ftruncate64, unsigned int, fd, u32, dummy,
+                       u32, low, u32, high)
 {
 	return sys_ftruncate(fd, ((loff_t)high << 32) | low);
 }
 
-long compat_sys_pread64(unsigned int fd, char __user *ubuf, size_t count,
-			u32 dummy, u32 low, u32 high)
+COMPAT_SYSCALL_DEFINE6(pread64, unsigned int, fd, char __user *, ubuf,
+                       size_t, count, u32, dummy, u32, low, u32, high)
 {
 	return sys_pread64(fd, ubuf, count, ((loff_t)high << 32) | low);
 }
 
-long compat_sys_pwrite64(unsigned int fd, char __user *ubuf, size_t count,
-			 u32 dummy, u32 low, u32 high)
+COMPAT_SYSCALL_DEFINE6(pwrite64, unsigned int, fd, char __user *, ubuf,
+                       size_t, count, u32, dummy, u32, low, u32, high)
 {
 	return sys_pwrite64(fd, ubuf, count, ((loff_t)high << 32) | low);
 }
 
-long compat_sys_lookup_dcookie(u32 low, u32 high, char __user *buf, size_t len)
+COMPAT_SYSCALL_DEFINE4(lookup_dcookie, u32, low, u32, high,
+                       char __user *, buf, size_t, len)
 {
 	return sys_lookup_dcookie(((loff_t)high << 32) | low, buf, len);
 }
 
-long compat_sys_sync_file_range2(int fd, unsigned int flags,
-				 u32 offset_lo, u32 offset_hi,
-				 u32 nbytes_lo, u32 nbytes_hi)
+COMPAT_SYSCALL_DEFINE6(sync_file_range2, int, fd, unsigned int, flags,
+                       u32, offset_lo, u32, offset_hi,
+                       u32, nbytes_lo, u32, nbytes_hi)
 {
 	return sys_sync_file_range(fd, ((loff_t)offset_hi << 32) | offset_lo,
 				   ((loff_t)nbytes_hi << 32) | nbytes_lo,
 				   flags);
 }
 
-long compat_sys_fallocate(int fd, int mode,
-			  u32 offset_lo, u32 offset_hi,
-			  u32 len_lo, u32 len_hi)
+COMPAT_SYSCALL_DEFINE6(fallocate, int, fd, int, mode,
+                       u32, offset_lo, u32, offset_hi,
+                       u32, len_lo, u32, len_hi)
 {
 	return sys_fallocate(fd, mode, ((loff_t)offset_hi << 32) | offset_lo,
 			     ((loff_t)len_hi << 32) | len_lo);
@@ -81,9 +84,9 @@ long compat_sys_fallocate(int fd, int mode,
  * offset_low as "unsigned long", thus making it possible to pass
  * a sign-extended high 32 bits in offset_low.
  */
-long compat_sys_llseek(unsigned int fd, unsigned int offset_high,
-		       unsigned int offset_low, loff_t __user * result,
-		       unsigned int origin)
+COMPAT_SYSCALL_DEFINE5(llseek, unsigned int, fd, unsigned int, offset_high,
+		       unsigned int, offset_low, loff_t __user *, result,
+		       unsigned int, origin)
 {
 	return sys_llseek(fd, offset_high, offset_low, result, origin);
 }
-- 
1.7.10.3