diff mbox series

Re:Re: [PATCH] powerpc: Fix a bug in __div64_32 if divisor is zero

Message ID 20200822165433.58228-1-zhongguohua1@huawei.com
State New
Headers show
Series Re:Re: [PATCH] powerpc: Fix a bug in __div64_32 if divisor is zero | expand

Commit Message

Guohua Zhong Aug. 22, 2020, 4:54 p.m. UTC
>In generic version in lib/math/div64.c, there is no checking of 'base' 
>either.
>Do we really want to add this check in the powerpc version only ?

>The only user of __div64_32() is do_div() in 
>include/asm-generic/div64.h. Wouldn't it be better to do the check there ?

>Christophe

Yet, I have noticed that there is no checking of 'base' in these functions.
But I am not sure how to check is better.As we know that the result is 
undefined when divisor is zero. It maybe good to print error and dump stack.
 Let the process to know that the divisor is zero by sending SIGFPE. 

div64_s64(), div64_u64(), div64_u64_rem(), div_s64_rem and div_u64_rem () 
in include/linux/math64.h

+ if (unlikely(divisor == 0)) {
+         pr_err("%s divisor=0\n",__func__);
+         dump_stack();
+         force_sig(SIGFPE);
+ }

Guohua

>>  	lwz	r5,0(r3)	# get the dividend into r5/r6
>>  	lwz	r6,4(r3)
>>  	cmplw	r5,r4
>>@@ -52,6 +55,7 @@ __div64_32:
>>  4:	stw	r7,0(r3)	# return the quotient in *r3
>>  	stw	r8,4(r3)
>>  	mr	r3,r6		# return the remainder in r3
>>+5:					# return if divisor r4 is zero
>>  	blr
>>  
>>  /*
>>diff --git a/arch/powerpc/lib/div64.S b/arch/powerpc/lib/div64.S
>>index 3d5426e7dcc4..1cc9bcabf678 100644
>>--- a/arch/powerpc/lib/div64.S
>>+++ b/arch/powerpc/lib/div64.S
>>@@ -13,6 +13,9 @@
>>  #include <asm/processor.h>
>>  
>>  _GLOBAL(__div64_32)
>>+	li	r9,0
>>+	cmplw	r4,r9	# check if divisor r4 is zero
>>+	beq	5f			# jump to label 5 if r4(divisor) is zero
>>  	lwz	r5,0(r3)	# get the dividend into r5/r6
>>  	lwz	r6,4(r3)
>>  	cmplw	r5,r4
>>@@ -52,4 +55,5 @@ _GLOBAL(__div64_32)
>>  4:	stw	r7,0(r3)	# return the quotient in *r3
>>  	stw	r8,4(r3)
>>  	mr	r3,r6		# return the remainder in r3
>>+5:					# return if divisor r4 is zero
>>  	blr
>>

Comments

Gabriel Paubert Aug. 22, 2020, 5:25 p.m. UTC | #1
On Sun, Aug 23, 2020 at 12:54:33AM +0800, Guohua Zhong wrote:
> >In generic version in lib/math/div64.c, there is no checking of 'base' 
> >either.
> >Do we really want to add this check in the powerpc version only ?
> 
> >The only user of __div64_32() is do_div() in 
> >include/asm-generic/div64.h. Wouldn't it be better to do the check there ?
> 
> >Christophe
> 
> Yet, I have noticed that there is no checking of 'base' in these functions.
> But I am not sure how to check is better.As we know that the result is 
> undefined when divisor is zero. It maybe good to print error and dump stack.
>  Let the process to know that the divisor is zero by sending SIGFPE. 
> 
> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
> index a3b98c86f077..161c656ee3ee 100644
> --- a/include/asm-generic/div64.h
> +++ b/include/asm-generic/div64.h
> @@ -43,6 +43,11 @@
>  # define do_div(n,base) ({                                     \
>         uint32_t __base = (base);                               \
>         uint32_t __rem;                                         \
> + if (unlikely(base == 0)) {                          \
> +         pr_err("do_div base=%d\n",base);            \
> +         dump_stack();                               \
> +         force_sig(SIGFPE);                          \
> + }      
> 

I suspect this will generate a strong reaction. SIGFPE is for user space
instruction attempting a division by zero. A division by zero in the
kernel is a kernel bug, period, and you don't want to kill a user
process for this reason.

If it happens in an interrupt, the context of the kernel may not even be
related to the current process.

Many other architectures (x86 for example) already trigger an exception
on a division by zero but the handler will find that the exception
happened in kernel context and generate an Oops, not raise a signal in a
(possibly innocent) userland process.

	Gabriel

> Then it also needto add this checking in functions of
> div64_s64(), div64_u64(), div64_u64_rem(), div_s64_rem and div_u64_rem () 
> in include/linux/math64.h
> 
> + if (unlikely(divisor == 0)) {
> +         pr_err("%s divisor=0\n",__func__);
> +         dump_stack();
> +         force_sig(SIGFPE);
> + }
> 
> Guohua
> 
> >>  	lwz	r5,0(r3)	# get the dividend into r5/r6
> >>  	lwz	r6,4(r3)
> >>  	cmplw	r5,r4
> >>@@ -52,6 +55,7 @@ __div64_32:
> >>  4:	stw	r7,0(r3)	# return the quotient in *r3
> >>  	stw	r8,4(r3)
> >>  	mr	r3,r6		# return the remainder in r3
> >>+5:					# return if divisor r4 is zero
> >>  	blr
> >>  
> >>  /*
> >>diff --git a/arch/powerpc/lib/div64.S b/arch/powerpc/lib/div64.S
> >>index 3d5426e7dcc4..1cc9bcabf678 100644
> >>--- a/arch/powerpc/lib/div64.S
> >>+++ b/arch/powerpc/lib/div64.S
> >>@@ -13,6 +13,9 @@
> >>  #include <asm/processor.h>
> >>  
> >>  _GLOBAL(__div64_32)
> >>+	li	r9,0
> >>+	cmplw	r4,r9	# check if divisor r4 is zero
> >>+	beq	5f			# jump to label 5 if r4(divisor) is zero
> >>  	lwz	r5,0(r3)	# get the dividend into r5/r6
> >>  	lwz	r6,4(r3)
> >>  	cmplw	r5,r4
> >>@@ -52,4 +55,5 @@ _GLOBAL(__div64_32)
> >>  4:	stw	r7,0(r3)	# return the quotient in *r3
> >>  	stw	r8,4(r3)
> >>  	mr	r3,r6		# return the remainder in r3
> >>+5:					# return if divisor r4 is zero
> >>  	blr
> >>
>
Segher Boessenkool Aug. 23, 2020, 12:11 a.m. UTC | #2
On Sun, Aug 23, 2020 at 12:54:33AM +0800, Guohua Zhong wrote:
> Yet, I have noticed that there is no checking of 'base' in these functions.
> But I am not sure how to check is better.As we know that the result is 
> undefined when divisor is zero. It maybe good to print error and dump stack.
>  Let the process to know that the divisor is zero by sending SIGFPE. 

That is now what the PowerPC integer divide insns do: they just leave
the result undefined (and they can set the overflow flag then, but no
one uses that).


Segher
Guohua Zhong Aug. 24, 2020, 11:54 a.m. UTC | #3
>> Yet, I have noticed that there is no checking of 'base' in these functions.
>> But I am not sure how to check is better.As we know that the result is 
>> undefined when divisor is zero. It maybe good to print error and dump stack.
>>  Let the process to know that the divisor is zero by sending SIGFPE. 

> That is now what the PowerPC integer divide insns do: they just leave
> the result undefined (and they can set the overflow flag then, but no
> one uses that).

OK ,So just keep the patch as below. If this patch looks good for you, please
help to review. I will send the new patch later.

Thanks for your reply.

diff --git a/arch/powerpc/boot/div64.S b/arch/powerpc/boot/div64.S
index 4354928ed62e..1d3561cf16fa 100644
--- a/arch/powerpc/boot/div64.S
+++ b/arch/powerpc/boot/div64.S
@@ -13,8 +13,10 @@

        .globl __div64_32
        .globl __div64_32
 __div64_32:
+ cmplwi      r4,0    # check if divisor r4 is zero
        lwz     r5,0(r3)        # get the dividend into r5/r6
        lwz     r6,4(r3)
+ beq 5f                      # jump to label 5 if r4(divisor) is zero
        cmplw   r5,r4
        li      r7,0
        li      r8,0
@@ -52,7 +54,7 @@ __div64_32:
 4:     stw     r7,0(r3)        # return the quotient in *r3
        stw     r8,4(r3)
        mr      r3,r6           # return the remainder in r3
-   blr
+5:   blr                             # return if divisor r4 is zero

 /*
  * Extended precision shifts.
diff --git a/arch/powerpc/lib/div64.S b/arch/powerpc/lib/div64.S
index 3d5426e7dcc4..570774d9782d 100644
--- a/arch/powerpc/lib/div64.S
+++ b/arch/powerpc/lib/div64.S
@@ -13,8 +13,10 @@
 #include <asm/processor.h>

 _GLOBAL(__div64_32)
+ cmplwi      r4,0    # check if divisor r4 is zero
        lwz     r5,0(r3)        # get the dividend into r5/r6
        lwz     r6,4(r3)
+ beq 5f                      # jump to label 5 if r4(divisor) is zero
        cmplw   r5,r4
        li      r7,0
        li      r8,0
@@ -52,4 +54,4 @@ _GLOBAL(__div64_32)
 4:     stw     r7,0(r3)        # return the quotient in *r3
        stw     r8,4(r3)
        mr      r3,r6           # return the remainder in r3
-   blr
+5:   blr                             # return if divisor r4 is zero

Guohua
Guohua Zhong Aug. 24, 2020, 1:25 p.m. UTC | #4
>> >In generic version in lib/math/div64.c, there is no checking of 'base' 
>> >either.
>> >Do we really want to add this check in the powerpc version only ?
>> 
>> >The only user of __div64_32() is do_div() in 
>> >include/asm-generic/div64.h. Wouldn't it be better to do the check there ?
>> 
>> >Christophe
>> 
>> Yet, I have noticed that there is no checking of 'base' in these functions.
>> But I am not sure how to check is better.As we know that the result is 
>> undefined when divisor is zero. It maybe good to print error and dump stack.
>>  Let the process to know that the divisor is zero by sending SIGFPE. 
>> 
>> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
>> index a3b98c86f077..161c656ee3ee 100644
>> --- a/include/asm-generic/div64.h
>> +++ b/include/asm-generic/div64.h
>> @@ -43,6 +43,11 @@
>>  # define do_div(n,base) ({                                     \
>>         uint32_t __base = (base);                               \
>>         uint32_t __rem;                                         \
>> + if (unlikely(base == 0)) {                          \
>> +         pr_err("do_div base=%d\n",base);            \
>> +         dump_stack();                               \
>> +         force_sig(SIGFPE);                          \
>> + }      
>> 

> I suspect this will generate a strong reaction. SIGFPE is for user space
> instruction attempting a division by zero. A division by zero in the
> kernel is a kernel bug, period, and you don't want to kill a user
> process for this reason.

> If it happens in an interrupt, the context of the kernel may not even be
> related to the current process.

> Many other architectures (x86 for example) already trigger an exception
> on a division by zero but the handler will find that the exception
> happened in kernel context and generate an Oops, not raise a signal in a
> (possibly innocent) userland process.

OK. So just don't touch do_div functions in include/asm-generic/div64.h
But for powerpc it can not trigger an exception when divisor is 0 in __div64_32.


So the patch as below is still useful for powerpc. If this patch looks good for 
you, please help to review. I will send the new patch later.

Thanks for your reply.

diff --git a/arch/powerpc/boot/div64.S b/arch/powerpc/boot/div64.S
index 4354928ed62e..1d3561cf16fa 100644
--- a/arch/powerpc/boot/div64.S
+++ b/arch/powerpc/boot/div64.S
@@ -13,8 +13,10 @@

        .globl __div64_32
        .globl __div64_32
 __div64_32:
+ cmplwi      r4,0    # check if divisor r4 is zero
        lwz     r5,0(r3)        # get the dividend into r5/r6
        lwz     r6,4(r3)
+ beq 5f                      # jump to label 5 if r4(divisor) is zero
        cmplw   r5,r4
        li      r7,0
        li      r8,0
@@ -52,7 +54,7 @@ __div64_32:
 4:     stw     r7,0(r3)        # return the quotient in *r3
        stw     r8,4(r3)
        mr      r3,r6           # return the remainder in r3
-   blr
+5:   blr                             # return if divisor r4 is zero

 /*
  * Extended precision shifts.
diff --git a/arch/powerpc/lib/div64.S b/arch/powerpc/lib/div64.S
index 3d5426e7dcc4..570774d9782d 100644
--- a/arch/powerpc/lib/div64.S
+++ b/arch/powerpc/lib/div64.S
@@ -13,8 +13,10 @@
 #include <asm/processor.h>

 _GLOBAL(__div64_32)
+ cmplwi      r4,0    # check if divisor r4 is zero
        lwz     r5,0(r3)        # get the dividend into r5/r6
        lwz     r6,4(r3)
+ beq 5f                      # jump to label 5 if r4(divisor) is zero
        cmplw   r5,r4
        li      r7,0
        li      r8,0
@@ -52,4 +54,4 @@ _GLOBAL(__div64_32)
 4:     stw     r7,0(r3)        # return the quotient in *r3
        stw     r8,4(r3)
        mr      r3,r6           # return the remainder in r3
-   blr
+5:   blr                             # return if divisor r4 is zero

Guohua
David Laight Aug. 24, 2020, 3:05 p.m. UTC | #5
From: Guohua Zhong
> Sent: 24 August 2020 14:26
> 
> >> >In generic version in lib/math/div64.c, there is no checking of 'base'
> >> >either.
> >> >Do we really want to add this check in the powerpc version only ?
> >>
> >> >The only user of __div64_32() is do_div() in
> >> >include/asm-generic/div64.h. Wouldn't it be better to do the check there ?
> >>
> >> >Christophe
> >>
> >> Yet, I have noticed that there is no checking of 'base' in these functions.
> >> But I am not sure how to check is better.As we know that the result is
> >> undefined when divisor is zero. It maybe good to print error and dump stack.

I thought that the onus was put on the caller to avoid divide by zero.

On x86 divide by zero causes an exception which (I'm pretty sure)
leads to a oops/panic.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Segher Boessenkool Aug. 24, 2020, 6:17 p.m. UTC | #6
On Mon, Aug 24, 2020 at 07:54:07PM +0800, Guohua Zhong wrote:
> >> Yet, I have noticed that there is no checking of 'base' in these functions.
> >> But I am not sure how to check is better.As we know that the result is 
> >> undefined when divisor is zero. It maybe good to print error and dump stack.
> >>  Let the process to know that the divisor is zero by sending SIGFPE. 
> 
> > That is now what the PowerPC integer divide insns do: they just leave
> > the result undefined (and they can set the overflow flag then, but no
> > one uses that).
> 
> OK ,So just keep the patch as below. If this patch looks good for you, please
> help to review. I will send the new patch later.
> 
> Thanks for your reply.
> 
> diff --git a/arch/powerpc/boot/div64.S b/arch/powerpc/boot/div64.S
> index 4354928ed62e..1d3561cf16fa 100644
> --- a/arch/powerpc/boot/div64.S
> +++ b/arch/powerpc/boot/div64.S
> @@ -13,8 +13,10 @@
> 
>         .globl __div64_32
>         .globl __div64_32
>  __div64_32:
> + cmplwi      r4,0    # check if divisor r4 is zero
>         lwz     r5,0(r3)        # get the dividend into r5/r6
>         lwz     r6,4(r3)
> + beq 5f                      # jump to label 5 if r4(divisor) is zero

Just "beqlr".

This instruction scheduling hurts all CPUs that aren't 8xx, fwiw (but
likely only in the case where r4 *is* zero, so who cares :-) )

So...  What is the *goal* of this patch?  It looks like the routine
would not get into a loop if r4 is 0, just return the wrong result?
But, it *always* will, there *is* no right result?

No caller should call it with zero as divisor ever, so in that sense,
checking for it in the division routine is just pure wasted work.


Segher
diff mbox series

Patch

diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
index a3b98c86f077..161c656ee3ee 100644
--- a/include/asm-generic/div64.h
+++ b/include/asm-generic/div64.h
@@ -43,6 +43,11 @@ 
 # define do_div(n,base) ({                                     \
        uint32_t __base = (base);                               \
        uint32_t __rem;                                         \
+ if (unlikely(base == 0)) {                          \
+         pr_err("do_div base=%d\n",base);            \
+         dump_stack();                               \
+         force_sig(SIGFPE);                          \
+ }      


Then it also needto add this checking in functions of