times(2) sys call bug?

times(2) sys call bug?

[Joakim Tjernlund]

Why does the below program end up reporting -1
multiple seconds when times() wrap:

#include <sys/times.h>
#include <stdio.h>

main()
{
        unsigned long t1;
        clock_t t2;
        while(1){
                t1 =3D times(NULL);
                t2 =3D times(NULL);
                sleep(1);
                printf("unsigned t1:%u, clock_t t2:%d\n", t1, t2);
                if (t2 > 1000)
                        break;
        }
}
and got:

unsigned t1:4294966339, clock_t t2:-957
unsigned t1:4294966439, clock_t t2:-857
unsigned t1:4294966539, clock_t t2:-757
unsigned t1:4294966639, clock_t t2:-657
unsigned t1:4294966739, clock_t t2:-557
unsigned t1:4294967295, clock_t t2:-1
unsigned t1:4294967295, clock_t t2:-1
unsigned t1:4294967295, clock_t t2:-1
unsigned t1:4294967295, clock_t t2:-1
unsigned t1:4294967295, clock_t t2:-1
unsigned t1:43, clock_t t2:43
unsigned t1:143, clock_t t2:143
unsigned t1:243, clock_t t2:243
unsigned t1:343, clock_t t2:343
unsigned t1:443, clock_t t2:443
unsigned t1:543, clock_t t2:543
unsigned t1:643, clock_t t2:643
unsigned t1:743, clock_t t2:743
unsigned t1:843, clock_t t2:843
unsigned t1:943, clock_t t2:943
unsigned t1:1043, clock_t t2:1043

Re: times(2) sys call bug?

[Josh Boyer]

On Thu, 20 Nov 2008 16:09:16 +0100
"Joakim Tjernlund" <joakim.tjernlund@transmode.se> wrote:

> Why does the below program end up reporting -1
> multiple seconds when times() wrap:

http://sources.redhat.com/bugzilla/show_bug.cgi?id=5209

josh

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Thu, 2008-11-20 at 10:37 -0500, Josh Boyer wrote:
> On Thu, 20 Nov 2008 16:09:16 +0100
> "Joakim Tjernlund" <joakim.tjernlund@transmode.se> wrote:
> 
> > Why does the below program end up reporting -1
> > multiple seconds when times() wrap:
> 
> http://sources.redhat.com/bugzilla/show_bug.cgi?id=5209
> 
> josh

I see, but this is a new ppc kernel bug I think.
This little hack changes the kernel sys call handling in an crude
way and then it works. Apperently the kernel thinks is an error if the
syscall returns a value between -_LAST_ERRNO and -1.
Perhaps a known bug?

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 1cbbf70..72effde 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -278,7 +278,8 @@ ret_from_syscall:
        SYNC
        MTMSRD(r10)
        lwz     r9,TI_FLAGS(r12)
-       li      r8,-_LAST_ERRNO
+       //li    r8,-_LAST_ERRNO
+       li      r8,-2
        andi.   r0,r9,(_TIF_SYSCALL_T_OR_A|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
        bne-    syscall_exit_work
        cmplw   0,r3,r8

Re: times(2) sys call bug?

[Paul Mackerras]

Joakim Tjernlund writes:

> This little hack changes the kernel sys call handling in an crude
> way and then it works. Apperently the kernel thinks is an error if the
> syscall returns a value between -_LAST_ERRNO and -1.

Try this patch and let me if it fixes it.  If it does I'll push it
upstream.

Paul.

diff --git a/kernel/sys.c b/kernel/sys.c
index 31deba8..1bf8c5c 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -33,6 +33,7 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/seccomp.h>
 #include <linux/cpu.h>
+#include <linux/ptrace.h>
 
 #include <linux/compat.h>
 #include <linux/syscalls.h>
@@ -878,6 +879,7 @@ asmlinkage long sys_times(struct tms __user * tbuf)
 		if (copy_to_user(tbuf, &tmp, sizeof(struct tms)))
 			return -EFAULT;
 	}
+	force_successful_syscall_return();
 	return (long) jiffies_64_to_clock_t(get_jiffies_64());
 }
 

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 10:52 +1100, Paul Mackerras wrote:
> Joakim Tjernlund writes:
> 
> > This little hack changes the kernel sys call handling in an crude
> > way and then it works. Apperently the kernel thinks is an error if the
> > syscall returns a value between -_LAST_ERRNO and -1.
> 
> Try this patch and let me if it fixes it.  If it does I'll push it
> upstream.
> 

It does fix the problem, thanks. You might want to do the same
to time(2)?
This workaround lets you get around the times(2) problem. Perhaps
you want to mention it in the commit msg:

static clock_t
our_times(void)        /* Make times(2) behave rationally on Linux */
{
	clock_t ret;

	errno   = 0;
	ret     = times(NULL);
	if (errno != 0)
		ret = (clock_t) (-errno);
	return ret;
}

Re: times(2) sys call bug?

[Gabriel Paubert]

On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> Joakim Tjernlund writes:
> 
> > This little hack changes the kernel sys call handling in an crude
> > way and then it works. Apperently the kernel thinks is an error if the
> > syscall returns a value between -_LAST_ERRNO and -1.
> 
> Try this patch and let me if it fixes it.  If it does I'll push it
> upstream.

With your patch, you won't get EFAULT if you pass a bad
address, but a constant, time independent value, unless
I miss something. 

Of course there are peoaple who claim that EFAULT is a
bad idea to start with and that you should send a SIGSEGV
instead, and I can see their point. 

But with the current implementation, it is a game that 
you can't win: any syscall that wants to return an
arbitrary integer multiplexed with an error value is
broken beyond repair, by design.

Oh, well. 

	Gabriel

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 09:41 +0100, Gabriel Paubert wrote:
> On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> > Joakim Tjernlund writes:
> > 
> > > This little hack changes the kernel sys call handling in an crude
> > > way and then it works. Apperently the kernel thinks is an error if the
> > > syscall returns a value between -_LAST_ERRNO and -1.
> > 
> > Try this patch and let me if it fixes it.  If it does I'll push it
> > upstream.
> 
> With your patch, you won't get EFAULT if you pass a bad
> address, but a constant, time independent value, unless
> I miss something. 

Not so, look again:

asmlinkage long sys_times(struct tms __user * tbuf)
{
	...
	if (tbuf) {
		...
		if (copy_to_user(tbuf, &tmp, sizeof(struct tms)))
			return -EFAULT;
	}
	force_successful_syscall_return();
	return (long) jiffies_64_to_clock_t(get_jiffies_64());
}

Re: times(2) sys call bug?

[Paul Mackerras]

Gabriel Paubert writes:

> On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> > Joakim Tjernlund writes:
> > 
> > > This little hack changes the kernel sys call handling in an crude
> > > way and then it works. Apperently the kernel thinks is an error if the
> > > syscall returns a value between -_LAST_ERRNO and -1.
> > 
> > Try this patch and let me if it fixes it.  If it does I'll push it
> > upstream.
> 
> With your patch, you won't get EFAULT if you pass a bad
> address, but a constant, time independent value, unless
> I miss something. 

I think you are missing something, namely that I put the call to
force_successful_syscall_return() AFTER the return -EFAULT.

You should get an EFAULT error if the address is bad, i.e. on return
to userspace with cr0.SO = 1 and r3 = EFAULT (note, not -EFAULT).  On
a non-error return you should get cr0.SO = 0 and r3 containing the
return value (even if it's -EFAULT).  It's possible that glibc will
stuff it up again after that but I hope not.

Paul.

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 20:03 +1100, Paul Mackerras wrote:
> Gabriel Paubert writes:
> 
> > On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> > > Joakim Tjernlund writes:
> > > 
> > > > This little hack changes the kernel sys call handling in an crude
> > > > way and then it works. Apperently the kernel thinks is an error if the
> > > > syscall returns a value between -_LAST_ERRNO and -1.
> > > 
> > > Try this patch and let me if it fixes it.  If it does I'll push it
> > > upstream.
> > 
> > With your patch, you won't get EFAULT if you pass a bad
> > address, but a constant, time independent value, unless
> > I miss something. 
> 
> I think you are missing something, namely that I put the call to
> force_successful_syscall_return() AFTER the return -EFAULT.
> 
> You should get an EFAULT error if the address is bad, i.e. on return
> to userspace with cr0.SO = 1 and r3 = EFAULT (note, not -EFAULT).  On
> a non-error return you should get cr0.SO = 0 and r3 containing the
> return value (even if it's -EFAULT).  It's possible that glibc will
> stuff it up again after that but I hope not.

With your patch:

	t1 = times((void*) 1);
	if (t1 == -1) {
		my_err = errno;
 		printf("Errno:%d, %s\n", my_err, strerror(my_err));
	}

prints:
	Errno:14, Bad address

Re: times(2) sys call bug?

[Gabriel Paubert]

On Fri, Nov 21, 2008 at 08:03:06PM +1100, Paul Mackerras wrote:
> Gabriel Paubert writes:
> 
> > On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> > > Joakim Tjernlund writes:
> > > 
> > > > This little hack changes the kernel sys call handling in an crude
> > > > way and then it works. Apperently the kernel thinks is an error if the
> > > > syscall returns a value between -_LAST_ERRNO and -1.
> > > 
> > > Try this patch and let me if it fixes it.  If it does I'll push it
> > > upstream.
> > 
> > With your patch, you won't get EFAULT if you pass a bad
> > address, but a constant, time independent value, unless
> > I miss something. 
> 
> I think you are missing something, namely that I put the call to
> force_successful_syscall_return() AFTER the return -EFAULT.
> 

Indeed, it may be time to update the syscall documentation, saying
that you need to clear errno before the syscall and check errno
and not the return value since -1 is valid. 

Who does this? I have spotted some errors in other places on 
my man pages too, especially in the networking area (they were
correct once upon a time, but have not been updated).

	Regards,
	Gabriel

Re: times(2) sys call bug?

[Paul Mackerras]

Gabriel Paubert writes:

> Who does this? I have spotted some errors in other places on 
> my man pages too, especially in the networking area (they were
> correct once upon a time, but have not been updated).

Michael Kerrisk <mtk.manpages@googlemail.com>, I believe.

Paul.

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 10:50 +0100, Gabriel Paubert wrote:
> On Fri, Nov 21, 2008 at 08:03:06PM +1100, Paul Mackerras wrote:
> > Gabriel Paubert writes:
> > 
> > > On Fri, Nov 21, 2008 at 10:52:14AM +1100, Paul Mackerras wrote:
> > > > Joakim Tjernlund writes:
> > > > 
> > > > > This little hack changes the kernel sys call handling in an crude
> > > > > way and then it works. Apperently the kernel thinks is an error if the
> > > > > syscall returns a value between -_LAST_ERRNO and -1.
> > > > 
> > > > Try this patch and let me if it fixes it.  If it does I'll push it
> > > > upstream.
> > > 
> > > With your patch, you won't get EFAULT if you pass a bad
> > > address, but a constant, time independent value, unless
> > > I miss something. 
> > 
> > I think you are missing something, namely that I put the call to
> > force_successful_syscall_return() AFTER the return -EFAULT.
> > 
> 
> Indeed, it may be time to update the syscall documentation, saying
> that you need to clear errno before the syscall and check errno
> and not the return value since -1 is valid. 

And perhaps mention that times(NULL) never returns an error.
And that times() is broken in 2.6.27 and earlier and needs the
workaround posted earlier.

 Jocke

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 10:52 +1100, Paul Mackerras wrote:
> Joakim Tjernlund writes:
> 
> > This little hack changes the kernel sys call handling in an crude
> > way and then it works. Apperently the kernel thinks is an error if the
> > syscall returns a value between -_LAST_ERRNO and -1.
> 
> Try this patch and let me if it fixes it.  If it does I'll push it
> upstream.
> 
> Paul.
[SNIP]
> +	force_successful_syscall_return();
>  	return (long) jiffies_64_to_clock_t(get_jiffies_64());

Why is 64 bits ops used here when you only use 32 bits? 

BTW, I think time(2) needs this:

diff --git a/kernel/time.c b/kernel/time.c
index 6a08660..1627910 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -37,6 +37,7 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/math64.h>
+#include <linux/ptrace.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -65,8 +66,9 @@ asmlinkage long sys_time(time_t __user * tloc)
 
 	if (tloc) {
 		if (put_user(i,tloc))
-			i = -EFAULT;
+			return -EFAULT;
 	}
+	force_successful_syscall_return();
 	return i;
 }
 

Re: times(2) sys call bug?

[Paul Mackerras]

Joakim Tjernlund writes:

> > +	force_successful_syscall_return();
> >  	return (long) jiffies_64_to_clock_t(get_jiffies_64());
> 
> Why is 64 bits ops used here when you only use 32 bits? 

If HZ is 1000, jiffies_64_to_clock_t is going to divide jiffies by 10,
so we need to start with 64 bits in order to get the top few bits
of a 32-bit result correct.

> BTW, I think time(2) needs this:

In principle you are correct, but it's only going to matter for a
little over an hour some time in the year 2106. :)

Paul.

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 20:51 +1100, Paul Mackerras wrote:
> Joakim Tjernlund writes:
> 
> > > +	force_successful_syscall_return();
> > >  	return (long) jiffies_64_to_clock_t(get_jiffies_64());
> > 
> > Why is 64 bits ops used here when you only use 32 bits? 
> 
> If HZ is 1000, jiffies_64_to_clock_t is going to divide jiffies by 10,
> so we need to start with 64 bits in order to get the top few bits
> of a 32-bit result correct.

I see, thanks.

> 
> > BTW, I think time(2) needs this:
> 
> In principle you are correct, but it's only going to matter for a
> little over an hour some time in the year 2106. :)

I know, but I figured it should be fixed to serve as an template for
other similar sys calls(not that I know of any offhand). Perhaps add it
commented?

 Jocke

Re: times(2) sys call bug?

[Joakim Tjernlund]

On Fri, 2008-11-21 at 10:31 +0100, Joakim Tjernlund wrote:
> On Fri, 2008-11-21 at 10:52 +1100, Paul Mackerras wrote:
> > Joakim Tjernlund writes:
> > 
> > > This little hack changes the kernel sys call handling in an crude
> > > way and then it works. Apperently the kernel thinks is an error if the
> > > syscall returns a value between -_LAST_ERRNO and -1.
> > 
> > Try this patch and let me if it fixes it.  If it does I'll push it
> > upstream.
> > 
> > Paul.
> [SNIP]
> > +	force_successful_syscall_return();
> >  	return (long) jiffies_64_to_clock_t(get_jiffies_64());
> 
> Why is 64 bits ops used here when you only use 32 bits? 
> 
> BTW, I think time(2) needs this:
> 
> diff --git a/kernel/time.c b/kernel/time.c
[SNIP]

Oh, and compat needs fixing too. 

For both my patches:
Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

BTW, why is sys_time() impl. differently in compat:
  do_gettimeofday(&tv);
vs.
  get_seconds();

same for sys_times(): 
  compat_jiffies_to_clock_t(jiffies);
vs.
  jiffies_64_to_clock_t(get_jiffies_64());


diff --git a/kernel/compat.c b/kernel/compat.c
index 32c254a..c6346ec 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -23,6 +23,7 @@
 #include <linux/timex.h>
 #include <linux/migrate.h>
 #include <linux/posix-timers.h>
+#include <linux/ptrace.h>
 
 #include <asm/uaccess.h>
 
@@ -196,6 +197,7 @@ asmlinkage long compat_sys_times(struct compat_tms __user *tbuf)
 		if (copy_to_user(tbuf, &tmp, sizeof(tmp)))
 			return -EFAULT;
 	}
+	force_successful_syscall_return();
 	return compat_jiffies_to_clock_t(jiffies);
 }
 
@@ -850,8 +852,9 @@ asmlinkage long compat_sys_time(compat_time_t __user * tloc)
 
 	if (tloc) {
 		if (put_user(i,tloc))
-			i = -EFAULT;
+			return -EFAULT;
 	}
+	force_successful_syscall_return();
 	return i;
 }