[PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Tony Breeds]

Hi Paul,
	It'd be nice if this made it into 2.6.22, esp. now that more
distros are shipping glibc's that will use the VDSO.

I think I got the asm right, it works on pSerie, either way feedback
welcome.

From: Tony Breeds <tony@bakeyournoodle.com>

Fix VDSO gettimeofday() when called with NULL struct timeval.

Consider the prototype for gettimeofday():
	int gettimofday(struct timeval *tv, struct timezone *tz);

AFAICT it is valid to call with /either/ tv or tz being NULL, the C version
of sys_gettimeofday() supports this, the current version of gettimeofday() in
the VDSO will SEGV if called with a NULL tv.

This patch fixes this.

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---

 arch/powerpc/kernel/vdso32/gettimeofday.S |   13 +++++++------
 arch/powerpc/kernel/vdso64/gettimeofday.S |    8 +++++---
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: working/arch/powerpc/kernel/vdso32/gettimeofday.S
===================================================================
--- working.orig/arch/powerpc/kernel/vdso32/gettimeofday.S	2007-06-25 19:35:47.000000000 +1000
+++ working/arch/powerpc/kernel/vdso32/gettimeofday.S	2007-06-25 19:40:34.000000000 +1000
@@ -32,8 +32,10 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mr	r11,r4			/* r11 saves tz */
 	bl	__get_datapage@local	/* get data page */
 	mr	r9, r3			/* datapage ptr in r9 */
+	cmpli	cr0,r10,0		/* check if tv is NULL */
+	beq	1f
 	bl	__do_get_xsec@local	/* get xsec from tb & kernel */
-	bne-	2f			/* out of line -> do syscall */
+	bne-	3f			/* out of line -> do syscall */
 
 	/* seconds are xsec >> 20 */
 	rlwinm	r5,r4,12,20,31
@@ -50,20 +52,19 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mulhwu	r5,r5,r6
 	stw	r5,TVAL32_TV_USEC(r10)
 
-	cmpli	cr0,r11,0		/* check if tz is NULL */
-	beq	1f
+1:	cmpli	cr0,r11,0		/* check if tz is NULL */
+	beq	2f
 	lwz	r4,CFG_TZ_MINUTEWEST(r9)/* fill tz */
 	lwz	r5,CFG_TZ_DSTTIME(r9)
 	stw	r4,TZONE_TZ_MINWEST(r11)
 	stw	r5,TZONE_TZ_DSTTIME(r11)
 
-1:	mtlr	r12
+2:	mtlr	r12
 	crclr	cr0*4+so
 	li	r3,0
 	blr
 
-2:
-	mtlr	r12
+3:	mtlr	r12
 	mr	r3,r10
 	mr	r4,r11
 	li	r0,__NR_gettimeofday
Index: working/arch/powerpc/kernel/vdso64/gettimeofday.S
===================================================================
--- working.orig/arch/powerpc/kernel/vdso64/gettimeofday.S	2007-06-25 19:35:52.000000000 +1000
+++ working/arch/powerpc/kernel/vdso64/gettimeofday.S	2007-06-25 19:42:45.000000000 +1000
@@ -32,6 +32,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mr	r11,r3			/* r11 holds tv */
 	mr	r10,r4			/* r10 holds tz */
 	bl	V_LOCAL_FUNC(__get_datapage)	/* get data page */
+	cmpldi	cr0,r11,0		/* check if tv is NULL */
+	beq	1f
 	bl	V_LOCAL_FUNC(__do_get_xsec)	/* get xsec from tb & kernel */
 	lis     r7,15			/* r7 = 1000000 = USEC_PER_SEC */
 	ori     r7,r7,16960
@@ -43,14 +45,14 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 					 * XSEC_PER_SEC
 					 */
 	rldicl  r0,r0,44,20
-	cmpldi	cr0,r10,0		/* check if tz is NULL */
+1:	cmpldi	cr0,r10,0		/* check if tz is NULL */
 	std	r0,TVAL64_TV_USEC(r11)	/* store usec in tv */
-	beq	1f
+	beq	2f
 	lwz	r4,CFG_TZ_MINUTEWEST(r3)/* fill tz */
 	lwz	r5,CFG_TZ_DSTTIME(r3)
 	stw	r4,TZONE_TZ_MINWEST(r10)
 	stw	r5,TZONE_TZ_DSTTIME(r10)
-1:	mtlr	r12
+2:	mtlr	r12
 	crclr	cr0*4+so
 	li	r3,0			/* always success */
 	blr

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Benjamin Herrenschmidt]

> Index: working/arch/powerpc/kernel/vdso32/gettimeofday.S
> ===================================================================
> --- working.orig/arch/powerpc/kernel/vdso32/gettimeofday.S	2007-06-25 19:35:47.000000000 +1000
> +++ working/arch/powerpc/kernel/vdso32/gettimeofday.S	2007-06-25 19:40:34.000000000 +1000
> @@ -32,8 +32,10 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
>  	mr	r11,r4			/* r11 saves tz */
>  	bl	__get_datapage@local	/* get data page */
>  	mr	r9, r3			/* datapage ptr in r9 */
> +	cmpli	cr0,r10,0		/* check if tv is NULL */
> +	beq	1f

Please use cmplwi here.

>  	bl	__do_get_xsec@local	/* get xsec from tb & kernel */
> -	bne-	2f			/* out of line -> do syscall */
> +	bne-	3f			/* out of line -> do syscall */
>  
>  	/* seconds are xsec >> 20 */
>  	rlwinm	r5,r4,12,20,31
> @@ -50,20 +52,19 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
>  	mulhwu	r5,r5,r6
>  	stw	r5,TVAL32_TV_USEC(r10)
>  
> -	cmpli	cr0,r11,0		/* check if tz is NULL */
> -	beq	1f
> +1:	cmpli	cr0,r11,0		/* check if tz is NULL */
> +	beq	2f

And fix that one while at it :-) It's the same thing, but at least one
is explicit on the size of the comparison. Also, I don't think you need
to renumber the labels, or did I miss something ?

Cheers,
Ben.

Re: [PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Segher Boessenkool]

> 	It'd be nice if this made it into 2.6.22, esp. now that more
> distros are shipping glibc's that will use the VDSO.

Yeah.  Not really vital though, no sane program would call
gettimeofday() with a NULL timeval (since that doesn't do
anything).  Or did you find such a program (other than a
testsuite)?

> +++ working/arch/powerpc/kernel/vdso32/gettimeofday.S	2007-06-25 
> 19:40:34.000000000 +1000
> @@ -32,8 +32,10 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
>  	mr	r11,r4			/* r11 saves tz */
>  	bl	__get_datapage@local	/* get data page */
>  	mr	r9, r3			/* datapage ptr in r9 */
> +	cmpli	cr0,r10,0		/* check if tv is NULL */

cmplwi r10,0

cmpli with two or three arguments is not an existing PowerPC
instruction.  Also, don't write cr0 if you don't do the same
in the accompanying branch insn, it only confuses things.

> +	beq	1f
>  	bl	__do_get_xsec@local	/* get xsec from tb & kernel */
> -	bne-	2f			/* out of line -> do syscall */
> +	bne-	3f			/* out of line -> do syscall */

No need to renumber the labels.  Either pick a higher
number for your new label (nothing says they need to be
in order); or reuse some other number (both your new and
the original 1: can be 1: without any problem, these are
local labels after all); or you can use 0: since that's
a valid local label as well.


Segher

Re: [PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Tony Breeds]

On Mon, Jun 25, 2007 at 10:39:42PM +1000, Benjamin Herrenschmidt wrote:
 
> Please use cmplwi here.

Will do.
 
> And fix that one while at it :-) It's the same thing, but at least one
> is explicit on the size of the comparison. Also, I don't think you need
> to renumber the labels, or did I miss something ?

No I did.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Tony Breeds]

On Mon, Jun 25, 2007 at 03:35:51PM +0200, Segher Boessenkool wrote:

> Yeah.  Not really vital though, no sane program would call
> gettimeofday() with a NULL timeval (since that doesn't do
> anything).  Or did you find such a program (other than a
> testsuite)?

No real program just me testing things.  I wanted to see what the
kernel had in sys_tz ;P
 
> cmplwi r10,0

Will do.  BenH also suggested this change.
 
> No need to renumber the labels.  Either pick a higher
> number for your new label (nothing says they need to be
> in order); or reuse some other number (both your new and
> the original 1: can be 1: without any problem, these are
> local labels after all); or you can use 0: since that's
> a valid local label as well.

Thanks.

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

[PATCH v2] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Tony Breeds]

Updated to include feedback from Ben and Segher, also reposition the
compare in the 64bit VDSO to catch all the references to tv.

From: Tony Breeds <tony@bakeyournoodle.com>

Fix VDSO gettimeofday() when called with NULL struct timeval.

Consider the prototype for gettimeofday():
	int gettimofday(struct timeval *tv, struct timezone *tz);

AFAICT it is valid to call with /either/ tv or tz being NULL, the C version
of sys_gettimeofday() supports this, the current version of gettimeofday() in
the VDSO will SEGV if called with a NULL tv.

This patch fixes this.

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---
Also fixes some whitespace damage at the top of vdso64/gettimeofday.S

 arch/powerpc/kernel/vdso32/gettimeofday.S |   13 +++++++------
 arch/powerpc/kernel/vdso64/gettimeofday.S |    8 +++++---
 2 files changed, 12 insertions(+), 9 deletions(-)

Index: working/arch/powerpc/kernel/vdso32/gettimeofday.S
===================================================================
--- working.orig/arch/powerpc/kernel/vdso32/gettimeofday.S
+++ working/arch/powerpc/kernel/vdso32/gettimeofday.S
@@ -32,6 +32,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mr	r11,r4			/* r11 saves tz */
 	bl	__get_datapage@local	/* get data page */
 	mr	r9, r3			/* datapage ptr in r9 */
+	cmplwi	r10,0			/* check if tv is NULL */
+	beq	3f
 	bl	__do_get_xsec@local	/* get xsec from tb & kernel */
 	bne-	2f			/* out of line -> do syscall */
 
@@ -50,7 +52,7 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mulhwu	r5,r5,r6
 	stw	r5,TVAL32_TV_USEC(r10)
 
-	cmpli	cr0,r11,0		/* check if tz is NULL */
+3:	cmplwi	r11,0			/* check if tz is NULL */
 	beq	1f
 	lwz	r4,CFG_TZ_MINUTEWEST(r9)/* fill tz */
 	lwz	r5,CFG_TZ_DSTTIME(r9)
Index: working/arch/powerpc/kernel/vdso64/gettimeofday.S
===================================================================
--- working.orig/arch/powerpc/kernel/vdso64/gettimeofday.S
+++ working/arch/powerpc/kernel/vdso64/gettimeofday.S
@@ -1,5 +1,4 @@
-
-	/*
+/*
  * Userland implementation of gettimeofday() for 64 bits processes in a
  * ppc64 kernel for use in the vDSO
  *
@@ -32,6 +31,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mr	r11,r3			/* r11 holds tv */
 	mr	r10,r4			/* r10 holds tz */
 	bl	V_LOCAL_FUNC(__get_datapage)	/* get data page */
+	cmpldi	r10,0			/* check if tv is NULL */
+	beq	2f
 	bl	V_LOCAL_FUNC(__do_get_xsec)	/* get xsec from tb & kernel */
 	lis     r7,15			/* r7 = 1000000 = USEC_PER_SEC */
 	ori     r7,r7,16960
@@ -43,8 +44,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 					 * XSEC_PER_SEC
 					 */
 	rldicl  r0,r0,44,20
-	cmpldi	cr0,r10,0		/* check if tz is NULL */
 	std	r0,TVAL64_TV_USEC(r11)	/* store usec in tv */
+2:	cmpldi	r10,0			/* check if tz is NULL */
 	beq	1f
 	lwz	r4,CFG_TZ_MINUTEWEST(r3)/* fill tz */
 	lwz	r5,CFG_TZ_DSTTIME(r3)

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH v2] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Benjamin Herrenschmidt]

On Tue, 2007-06-26 at 09:50 +1000, Tony Breeds wrote:
> Updated to include feedback from Ben and Segher, also reposition the
> compare in the 64bit VDSO to catch all the references to tv.
> 
> From: Tony Breeds <tony@bakeyournoodle.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [PATCH v2] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Segher Boessenkool]

>> Updated to include feedback from Ben and Segher, also reposition the
>> compare in the 64bit VDSO to catch all the references to tv.
>>
>> From: Tony Breeds <tony@bakeyournoodle.com>
>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Yep fine, thanks Tony.


Segher

Re: [PATCH v2] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Will Schmidt]

On Tue, 2007-06-26 at 09:50 +1000, Tony Breeds wrote:
> Updated to include feedback from Ben and Segher, also reposition the
> compare in the 64bit VDSO to catch all the references to tv.


> --- working.orig/arch/powerpc/kernel/vdso64/gettimeofday.S
> +++ working/arch/powerpc/kernel/vdso64/gettimeofday.S

<snippage>
> @@ -32,6 +31,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
>  	mr	r11,r3			/* r11 holds tv */
>  	mr	r10,r4			/* r10 holds tz */
>  	bl	V_LOCAL_FUNC(__get_datapage)	/* get data page */
> +	cmpldi	r10,0			/* check if tv is NULL */

Whoops!    r10 holds tz value, not tv.   This should be r11.  

As is, a (64-bit) userspace app calling gettimeofday(&tv,NULL); will
"silently fail", and the application is left using whatever old data
happens to be in the tv structure.

This affected my G5 (gentoo), most noticable when apps like 'ping' and
nfs-mounts quit working properly.  :-) 

patch follows momentarily... 

> +	beq	2f
>  	bl	V_LOCAL_FUNC(__do_get_xsec)	/* get xsec from tb & kernel */
>  	lis     r7,15			/* r7 = 1000000 = USEC_PER_SEC */
>  	ori     r7,r7,16960
> @@ -43,8 +44,8 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
>  					 * XSEC_PER_SEC
>  					 */
>  	rldicl  r0,r0,44,20
> -	cmpldi	cr0,r10,0		/* check if tz is NULL */
>  	std	r0,TVAL64_TV_USEC(r11)	/* store usec in tv */
> +2:	cmpldi	r10,0			/* check if tz is NULL */
>  	beq	1f
>  	lwz	r4,CFG_TZ_MINUTEWEST(r3)/* fill tz */
>  	lwz	r5,CFG_TZ_DSTTIME(r3)
> 
> Yours Tony
> 
>   linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
>   Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

[PATCH v2.1 ] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Will Schmidt]

The vdso64 portion of patch 74609f4536f2b8fd6a48381bbbe3cd37da20a527 for
fixing problems with NULL gettimeofday input mistakenly checks for a
null tz field twice, when it should be checking for null tz once, and
null tv once; by way of a r10/r11 typo. 

Any application calling gettimeofday(&tv,NULL) will "fail".

This corrects that typo, and makes my G5 happy.

tested on G5. 


Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>
cc: Tony Breeds <tony@bakeyournoodle.com>
cc: Ben Herrenschmidt <benh@kernel.crashing.org>

---

diff --git a/arch/powerpc/kernel/vdso64/gettimeofday.S b/arch/powerpc/kernel/vdso64/gettimeofday.S
index 2d7a510..c6401f9 100644
--- a/arch/powerpc/kernel/vdso64/gettimeofday.S
+++ b/arch/powerpc/kernel/vdso64/gettimeofday.S
@@ -31,7 +31,7 @@ V_FUNCTION_BEGIN(__kernel_gettimeofday)
 	mr	r11,r3			/* r11 holds tv */
 	mr	r10,r4			/* r10 holds tz */
 	bl	V_LOCAL_FUNC(__get_datapage)	/* get data page */
-	cmpldi	r10,0			/* check if tv is NULL */
+	cmpldi	r11,0			/* check if tv is NULL */
 	beq	2f
 	bl	V_LOCAL_FUNC(__do_get_xsec)	/* get xsec from tb & kernel */
 	lis     r7,15			/* r7 = 1000000 = USEC_PER_SEC */

Re: [PATCH v2.1 ] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Segher Boessenkool]

> The vdso64 portion of patch 74609f4536f2b8fd6a48381bbbe3cd37da20a527 
> for
> fixing problems with NULL gettimeofday input mistakenly checks for a
> null tz field twice, when it should be checking for null tz once, and
> null tv once; by way of a r10/r11 typo.
>
> Any application calling gettimeofday(&tv,NULL) will "fail".
>
> This corrects that typo, and makes my G5 happy.
>
> tested on G5.
>
>
> Signed-off-by: Will Schmidt <will_schmidt@vnet.ibm.com>

Obviously correct, how could we all have missed this :-(

Thanks Will.


Segher

Re: [PATCH v2.1 ] Fix VDSO gettimeofday() when called with NULL struct timeval.

[Tony Breeds]

On Fri, Jun 29, 2007 at 03:49:50PM -0500, Will Schmidt wrote:
> 
> The vdso64 portion of patch 74609f4536f2b8fd6a48381bbbe3cd37da20a527 for
> fixing problems with NULL gettimeofday input mistakenly checks for a
> null tz field twice, when it should be checking for null tz once, and
> null tv once; by way of a r10/r11 typo. 
> 
> Any application calling gettimeofday(&tv,NULL) will "fail".
> 
> This corrects that typo, and makes my G5 happy.

Ooops thanks Will!

The /really/ sad thing is I got it right in the first version of the
patch :(

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!