[PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

I see a panic in early boot when building with a recent gcc toolchain.
The issue is a divide by zero, which is undefined. Older toolchains
let us get away with it:

int foo(int a) { return a / 0; }

foo:
	li 9,0
	divw 3,3,9
	extsw 3,3
	blr

But newer ones catch it:

foo:
	trap

Add a check to avoid the divide by zero.

Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache line")
Signed-off-by: Anton Blanchard <anton@samba.org>
---
 arch/powerpc/kernel/setup_64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index adf2084..afd1c26 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -408,7 +408,8 @@ static void init_cache_info(struct ppc_cache_info *info, u32 size, u32 lsize,
 	info->line_size = lsize;
 	info->block_size = bsize;
 	info->log_block_size = __ilog2(bsize);
-	info->blocks_per_page = PAGE_SIZE / bsize;
+	if (bsize)
+		info->blocks_per_page = PAGE_SIZE / bsize;
 
 	if (sets == 0)
 		info->assoc = 0xffff;
-- 
2.7.4

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Benjamin Herrenschmidt]

On Sun, 2017-03-05 at 10:54 +1100, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> I see a panic in early boot when building with a recent gcc
> toolchain.
> The issue is a divide by zero, which is undefined. Older toolchains
> let us get away with it:

Maybe we should panic though ... not having a valid cache block size is
going to be fatal in other areas...

> int foo(int a) { return a / 0; }
> 
> foo:
> 	li 9,0
> 	divw 3,3,9
> 	extsw 3,3
> 	blr
> 
> But newer ones catch it:
> 
> foo:
> 	trap
> 
> Add a check to avoid the divide by zero.
> 
> Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache
> line")
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>  arch/powerpc/kernel/setup_64.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/setup_64.c
> b/arch/powerpc/kernel/setup_64.c
> index adf2084..afd1c26 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -408,7 +408,8 @@ static void init_cache_info(struct ppc_cache_info
> *info, u32 size, u32 lsize,
>  	info->line_size = lsize;
>  	info->block_size = bsize;
>  	info->log_block_size = __ilog2(bsize);
> -	info->blocks_per_page = PAGE_SIZE / bsize;
> +	if (bsize)
> +		info->blocks_per_page = PAGE_SIZE / bsize;
>  
>  	if (sets == 0)
>  		info->assoc = 0xffff;

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Benjamin Herrenschmidt]

On Sun, 2017-03-05 at 11:25 +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2017-03-05 at 10:54 +1100, Anton Blanchard wrote:
> > From: Anton Blanchard <anton@samba.org>
> > 
> > I see a panic in early boot when building with a recent gcc
> > toolchain.
> > The issue is a divide by zero, which is undefined. Older toolchains
> > let us get away with it:
> 
> Maybe we should panic though ... not having a valid cache block size
> is going to be fatal in other areas...

 ... Unless it's for L2/L3 caches. Of course...

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

> > int foo(int a) { return a / 0; }
> > 
> > foo:
> > 	li 9,0
> > 	divw 3,3,9
> > 	extsw 3,3
> > 	blr
> > 
> > But newer ones catch it:
> > 
> > foo:
> > 	trap
> > 
> > Add a check to avoid the divide by zero.
> > 
> > Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs.
> > cache
> > line")
> > Signed-off-by: Anton Blanchard <anton@samba.org>
> > ---
> >  arch/powerpc/kernel/setup_64.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kernel/setup_64.c
> > b/arch/powerpc/kernel/setup_64.c
> > index adf2084..afd1c26 100644
> > --- a/arch/powerpc/kernel/setup_64.c
> > +++ b/arch/powerpc/kernel/setup_64.c
> > @@ -408,7 +408,8 @@ static void init_cache_info(struct
> > ppc_cache_info
> > *info, u32 size, u32 lsize,
> >  	info->line_size = lsize;
> >  	info->block_size = bsize;
> >  	info->log_block_size = __ilog2(bsize);
> > -	info->blocks_per_page = PAGE_SIZE / bsize;
> > +	if (bsize)
> > +		info->blocks_per_page = PAGE_SIZE / bsize;
> >  
> >  	if (sets == 0)
> >  		info->assoc = 0xffff;

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Michael Ellerman]

Anton Blanchard <anton@ozlabs.org> writes:

> From: Anton Blanchard <anton@samba.org>
>
> I see a panic in early boot when building with a recent gcc toolchain.
> The issue is a divide by zero, which is undefined. Older toolchains
> let us get away with it:
>
> int foo(int a) { return a / 0; }
>
> foo:
> 	li 9,0
> 	divw 3,3,9
> 	extsw 3,3
> 	blr
>
> But newer ones catch it:
>
> foo:
> 	trap
>
> Add a check to avoid the divide by zero.

Erk sorry. One of the static checkers spotted it, but I hadn't got
around to fixing it because it seemed to not actually blow up, guess
not.

cheers

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Segher Boessenkool]

On Sun, Mar 05, 2017 at 09:26:47PM +1100, Michael Ellerman wrote:
> > I see a panic in early boot when building with a recent gcc toolchain.
> > The issue is a divide by zero, which is undefined. Older toolchains
> > let us get away with it:
> >
> > int foo(int a) { return a / 0; }
> >
> > foo:
> > 	li 9,0
> > 	divw 3,3,9
> > 	extsw 3,3
> > 	blr
> >
> > But newer ones catch it:
> >
> > foo:
> > 	trap
> >
> > Add a check to avoid the divide by zero.
> 
> Erk sorry. One of the static checkers spotted it, but I hadn't got
> around to fixing it because it seemed to not actually blow up, guess
> not.

The PowerPC divw etc. instructions do not trap by themselves, but recent
GCC inserts trap instructions on code paths that are always undefined
behaviour (like, dividing by zero).


Segher

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Gabriel Paubert]

On Sun, Mar 05, 2017 at 06:37:37AM -0600, Segher Boessenkool wrote:
> On Sun, Mar 05, 2017 at 09:26:47PM +1100, Michael Ellerman wrote:
> > > I see a panic in early boot when building with a recent gcc toolchain.
> > > The issue is a divide by zero, which is undefined. Older toolchains
> > > let us get away with it:
> > >
> > > int foo(int a) { return a / 0; }
> > >
> > > foo:
> > > 	li 9,0
> > > 	divw 3,3,9
> > > 	extsw 3,3
> > > 	blr
> > >
> > > But newer ones catch it:
> > >
> > > foo:
> > > 	trap
> > >
> > > Add a check to avoid the divide by zero.
> > 
> > Erk sorry. One of the static checkers spotted it, but I hadn't got
> > around to fixing it because it seemed to not actually blow up, guess
> > not.
> 
> The PowerPC divw etc. instructions do not trap by themselves, but recent
> GCC inserts trap instructions on code paths that are always undefined
> behaviour (like, dividing by zero).


Is it systematic or does it depend from, e.g., optimization levels?

Is there anything in the standards about this feature?

    Gabriel

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Segher Boessenkool]

On Sun, Mar 05, 2017 at 05:58:37PM +0100, Gabriel Paubert wrote:
> > > Erk sorry. One of the static checkers spotted it, but I hadn't got
> > > around to fixing it because it seemed to not actually blow up, guess
> > > not.
> > 
> > The PowerPC divw etc. instructions do not trap by themselves, but recent
> > GCC inserts trap instructions on code paths that are always undefined
> > behaviour (like, dividing by zero).
> 
> Is it systematic or does it depend from, e.g., optimization levels?

In this case it needs -fisolate-erroneous-paths-dereference which is
default at -O2 and higher.

> Is there anything in the standards about this feature?

The compiler can do whatever it likes with code that has undefined
behaviour.  With this optimisation it a) can compile the conforming
code to something better; and b) undefined behaviour will trap instead
of doing something random (which often is exploitable).


Segher

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Benjamin Herrenschmidt]

On Sun, 2017-03-05 at 11:24 -0600, Segher Boessenkool wrote:
> On Sun, Mar 05, 2017 at 05:58:37PM +0100, Gabriel Paubert wrote:
> > > > Erk sorry. One of the static checkers spotted it, but I hadn't got
> > > > around to fixing it because it seemed to not actually blow up, guess
> > > > not.
> > > 
> > > The PowerPC divw etc. instructions do not trap by themselves, but recent
> > > GCC inserts trap instructions on code paths that are always undefined
> > > behaviour (like, dividing by zero).
> > 
> > Is it systematic or does it depend from, e.g., optimization levels?
> 
> In this case it needs -fisolate-erroneous-paths-dereference which is
> default at -O2 and higher.
> 
> > Is there anything in the standards about this feature?
> 
> The compiler can do whatever it likes with code that has undefined
> behaviour.  With this optimisation it a) can compile the conforming
> code to something better; and b) undefined behaviour will trap instead
> of doing something random (which often is exploitable).

I actually like that feature, except it did bite me once or twice in the past
adding traps to intentional NULL dereferences ;-) Ah the joys of writing
a firmware where you poke at stuff at fixed addresses in low memory :-)

Cheers,
Ben.

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Segher Boessenkool]

On Mon, Mar 06, 2017 at 10:09:01AM +1100, Benjamin Herrenschmidt wrote:
> > The compiler can do whatever it likes with code that has undefined
> > behaviour.  With this optimisation it a) can compile the conforming
> > code to something better; and b) undefined behaviour will trap instead
> > of doing something random (which often is exploitable).
> 
> I actually like that feature,

Yeah, me too -- it also (currently) makes *smaller* code than it would
without it.  Win-win-win.

> except it did bite me once or twice in the past
> adding traps to intentional NULL dereferences ;-) Ah the joys of writing
> a firmware where you poke at stuff at fixed addresses in low memory :-)

You cannot really have something at address 0, the way NULL pointers
are represented in GCC.  0 in firmware, so *fun*, especially before the
CFAR was invented.  "Something jumped to 0, CTR is 0 so it's probably
a BCTR, but which one of the 6000?"

What do you have at 0?  Not anything you need often I hope?


Segher

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Benjamin Herrenschmidt]

On Sun, 2017-03-05 at 18:10 -0600, Segher Boessenkool wrote:
> You cannot really have something at address 0, the way NULL pointers
> are represented in GCC.  0 in firmware, so *fun*, especially before
> the
> CFAR was invented.  "Something jumped to 0, CTR is 0 so it's probably
> a BCTR, but which one of the 6000?"
> 
> What do you have at 0?  Not anything you need often I hope?

I think it was some kind of boot flag. I've had cases also of
copying the 0..0x100 region over with the kexec gunk etc...

Ben.

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Gabriel Paubert]

On Sun, Mar 05, 2017 at 11:24:56AM -0600, Segher Boessenkool wrote:
> On Sun, Mar 05, 2017 at 05:58:37PM +0100, Gabriel Paubert wrote:
> > > > Erk sorry. One of the static checkers spotted it, but I hadn't got
> > > > around to fixing it because it seemed to not actually blow up, guess
> > > > not.
> > > 
> > > The PowerPC divw etc. instructions do not trap by themselves, but recent
> > > GCC inserts trap instructions on code paths that are always undefined
> > > behaviour (like, dividing by zero).
> > 
> > Is it systematic or does it depend from, e.g., optimization levels?
> 
> In this case it needs -fisolate-erroneous-paths-dereference which is
> default at -O2 and higher.

Great, another optimization-dependent behaviour. :-(

But this is not the most serious issue: on PPC, when you #include
<limits>, the numeric_limits<any_integer_type>::traps is false on PPC,
and on no other architecture that I know of (in practice this trap
reflects the hardware behaviour on division by zero).

By generating a trap in this case, I believe that the compiler violates
a contract given by <limits>, and the standard.

I'd certainly prefer a compile time warning, easily convertible to an
error.

> 
> > Is there anything in the standards about this feature?
> 
> The compiler can do whatever it likes with code that has undefined
> behaviour.  With this optimisation it a) can compile the conforming
> code to something better; and b) undefined behaviour will trap instead
> of doing something random (which often is exploitable).

It may be undefined, but I believe that the numeric_limits<>::traps
value clearly prohibits generating a trap in this case.

    Gabriel

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Segher Boessenkool]

On Mon, Mar 06, 2017 at 01:03:19PM +0100, Gabriel Paubert wrote:
> > > > The PowerPC divw etc. instructions do not trap by themselves, but recent
> > > > GCC inserts trap instructions on code paths that are always undefined
> > > > behaviour (like, dividing by zero).
> > > 
> > > Is it systematic or does it depend from, e.g., optimization levels?
> > 
> > In this case it needs -fisolate-erroneous-paths-dereference which is
> > default at -O2 and higher.
> 
> Great, another optimization-dependent behaviour. :-(

It makes the "behaviour" for undefined behaviour *less* surprising.
It does not change anything else: malformed programs stay malformed,
correct programs do exactly what they did before, too.

> But this is not the most serious issue: on PPC, when you #include
> <limits>, the numeric_limits<any_integer_type>::traps is false on PPC,
> and on no other architecture that I know of (in practice this trap
> reflects the hardware behaviour on division by zero).
> 
> By generating a trap in this case, I believe that the compiler violates
> a contract given by <limits>, and the standard.

[ snip ]

I have no idea why you are bringing C++ into this.  Please open a PR
if you think there is a bug in the C++ library.

I'll note that this cannot violate the standard, see the "terms and
definitions":

[defns.undefined]
undefined behavior
behavior for which this International Standard imposes no requirements


Segher

RE: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[David Laight]

From: Segher Boessenkool
> Sent: 06 March 2017 14:18
> On Mon, Mar 06, 2017 at 01:03:19PM +0100, Gabriel Paubert wrote:
> > > > > The PowerPC divw etc. instructions do not trap by themselves, but=
 recent
> > > > > GCC inserts trap instructions on code paths that are always undef=
ined
> > > > > behaviour (like, dividing by zero).
> > > >
> > > > Is it systematic or does it depend from, e.g., optimization levels?
> > >
> > > In this case it needs -fisolate-erroneous-paths-dereference which is
> > > default at -O2 and higher.
> >
> > Great, another optimization-dependent behaviour. :-(
>=20
> It makes the "behaviour" for undefined behaviour *less* surprising.
> It does not change anything else: malformed programs stay malformed,
> correct programs do exactly what they did before, too.

Yep, 'undefined behaviour' is exactly that.
It doesn't mean 'undefined result', or 'maybe a signal'.
Wiping the disk and targeting the user with an ICBM are both valid.

	David

Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Michael Ellerman]

Anton Blanchard <anton@ozlabs.org> writes:
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index adf2084..afd1c26 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -408,7 +408,8 @@ static void init_cache_info(struct ppc_cache_info *info, u32 size, u32 lsize,
>  	info->line_size = lsize;
>  	info->block_size = bsize;
>  	info->log_block_size = __ilog2(bsize);
> -	info->blocks_per_page = PAGE_SIZE / bsize;
> +	if (bsize)
> +		info->blocks_per_page = PAGE_SIZE / bsize;
  
And we should probably initialise it to zero just in case, so I added:

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index afd1c2623459..9cfaa8b69b5f 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -410,6 +410,8 @@ static void init_cache_info(struct ppc_cache_info *info, u32 size, u32 lsize,
 	info->log_block_size = __ilog2(bsize);
 	if (bsize)
 		info->blocks_per_page = PAGE_SIZE / bsize;
+	else
+		info->blocks_per_page = 0;
 
 	if (sets == 0)
 		info->assoc = 0xffff;

cheers

Re: powerpc: Avoid panic during boot due to divide by zero in init_cache_info()

[Michael Ellerman]

On Sat, 2017-03-04 at 23:54:34 UTC, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> I see a panic in early boot when building with a recent gcc toolchain.
> The issue is a divide by zero, which is undefined. Older toolchains
> let us get away with it:
> 
> int foo(int a) { return a / 0; }
> 
> foo:
> 	li 9,0
> 	divw 3,3,9
> 	extsw 3,3
> 	blr
> 
> But newer ones catch it:
> 
> foo:
> 	trap
> 
> Add a check to avoid the divide by zero.
> 
> Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache line")
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/6ba422c75facb1b1e0e206c464ee12

cheers