RE: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()
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 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. 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()
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 , the numeric_limits::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 , 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()
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()
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()
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()
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()
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()
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()
Anton Blanchardwrites: > 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 = 0x; cheers
Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()
Anton Blanchardwrites: > From: Anton Blanchard > > 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()
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> > > > 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 > > 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 > > --- > > 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 = 0x;
Re: [PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()
On Sun, 2017-03-05 at 10:54 +1100, Anton Blanchard wrote: > From: Anton Blanchard> > 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 > --- > 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 = 0x;
[PATCH] powerpc: Avoid panic during boot due to divide by zero in init_cache_info()
From: Anton BlanchardI 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 --- 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 = 0x; -- 2.7.4