| Message ID | 1488671674-20833-1-git-send-email-anton@ozlabs.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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;
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;
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
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
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
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
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.
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
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.
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
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
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
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
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;