Re: [PATCH] ARM: Convert BUG() to use unreachable()
Russell King - ARM Linux wrote: > [ ... ] this thread should probably die until some problem has actually > been identified. > > If it ain't broke, don't fix. Couldn't agree more. Happy Christmas! cheers, DaveK
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Tue, Dec 22, 2009 at 02:09:02PM +, Dave Korn wrote: > Russell King - ARM Linux wrote: > > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: > >>> How is "size-optimal trap" defined? > >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the > >> condition codes, and so we eliminate the branch. That's the only > >> optimization we apply with __builtin_trap. > >> > >>> Let me put it another way: I want this function to terminate with an > >>> explicit NULL pointer dereference in every case. > >> Then just use that. > > > > That's precisely what we have been using for many years. > > I don't understand. It should still work just fine; the original version > posted appears to simply lack 'volatile' on the (int *) cast. Neither do I - AFAIK the existing code works fine. I think this is just a noisy thread about people wanting to use the latest and greated compiler "features" whether they make sense to or not, and this thread should probably die until some problem has actually been identified. If it ain't broke, don't fix.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
Russell King - ARM Linux wrote: > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: >>> How is "size-optimal trap" defined? >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the >> condition codes, and so we eliminate the branch. That's the only >> optimization we apply with __builtin_trap. >> >>> Let me put it another way: I want this function to terminate with an >>> explicit NULL pointer dereference in every case. >> Then just use that. > > That's precisely what we have been using for many years. I don't understand. It should still work just fine; the original version posted appears to simply lack 'volatile' on the (int *) cast. cheers, DaveK
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On 12/17/2009 06:17 PM, Richard Guenther wrote: It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure use __builtin_trap (); instead for the whole sequence (the unreachable is implied then). GCC choses a size-optimal trap representation for your target then. Agree that it shouldn't but just to be sure I'd use *(volatile int *)0 = 0; unreachable (); Paolo
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: >> How is "size-optimal trap" defined? > > E.g. Sparc and MIPS have "tcc" instructions that trap based on the > condition codes, and so we eliminate the branch. That's the only > optimization we apply with __builtin_trap. > >> Let me put it another way: I want this function to terminate with an >> explicit NULL pointer dereference in every case. > > Then just use that. That's precisely what we have been using for many years.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: How is "size-optimal trap" defined? E.g. Sparc and MIPS have "tcc" instructions that trap based on the condition codes, and so we eliminate the branch. That's the only optimization we apply with __builtin_trap. Let me put it another way: I want this function to terminate with an explicit NULL pointer dereference in every case. Then just use that. r~
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 07:48:37PM +, Russell King - ARM Linux wrote: > Given the lack of progress/bug reporting on ARM uclinux, the lack of > platform support and the lack of configurations, my view is that there > is no one actually using it. I know that I don't particularly take any > care with respect to uclinux when making changes to the MMU based kernels. > Why bother if apparantly no one's using it? Jamie, As you seem to be a user of uclinux on ARM, could you help ensure that the support there is bug fixed and we at least have a configuration file which kautobuild can use to provide feedback on the build status of uclinux on ARM please?
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 07:38:26PM +, Jamie Lokier wrote: > Joe Buck wrote: > > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > > Besides, didn't I see a whole bunch of kernel security patches related > > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > > suddenly won't get your trap. > > > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > > protection is independent of the generic kernel. > > > > > > Milage may vary on other architectures, but that's not a concern here. > > It does not trap on at least one ARM-nommu kernel... I was going to say the following in a different reply but discarded it because it wasn't relevant to the GCC list. I regard ARM nommu as highly experimental, especially as the maintainer vanished half way through merging it into mainline. I know that there are some parts of ARM nommu that are highly buggy - such as ARM940 support invalidating the entire data cache on any DMA transaction... say goodbye stacked return addresses. As such, I would not be surprised if the ARM nommu kernel has _lots_ of weird and wonderful bugs. I am not surprised that NULL pointer dereferences don't work - its actually something I'd expect given that they have a protection unit which the kernel doesn't apparently touch. Maybe the protection unit code never got merged? I've no idea. As I say, uclinux support got as far as being half merged and that's roughly the state it's remained in ever since. We don't even have any no-MMU configurations which the kernel builder automatically tests for us. Given the lack of progress/bug reporting on ARM uclinux, the lack of platform support and the lack of configurations, my view is that there is no one actually using it. I know that I don't particularly take any care with respect to uclinux when making changes to the MMU based kernels. Why bother if apparantly no one's using it?
Re: [PATCH] ARM: Convert BUG() to use unreachable()
Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > Besides, didn't I see a whole bunch of kernel security patches related > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > suddenly won't get your trap. > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > protection is independent of the generic kernel. > > > > Milage may vary on other architectures, but that's not a concern here. It does not trap on at least one ARM-nommu kernel... > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. The patch under discussion, which led to these questions and Cc gcc@gcc.gnu.org, is specific to the ARM architecture and that is Russell's focus, as ARM kernel maintainer. But yes, the whole principle of how to trap and whether it's safe to follow a null pointer dereference with __builtin_unreachable() applies to all the other architectures too. -- Jamie
Re: [PATCH] ARM: Convert BUG() to use unreachable()
Joe Buck wrote: On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: Besides, didn't I see a whole bunch of kernel security patches related to null pointer dereferences lately? If page 0 can be mapped, you suddenly won't get your trap. Page 0 can not be mapped on ARM kernels since the late 1990s, and this protection is independent of the generic kernel. Milage may vary on other architectures, but that's not a concern here. I don't understand, though, why you would want to implement a generally useful facility (make the kernel trap so you can do a post-mortem analysis) in a way that's only safe for the ARM port. Each Linux kernel architecture has in its architecture specific bug.h an implementation that is deemed by the architecture maintainers to work. As far as I know, few if any of these use __builtin_trap(). Some could be converted to __builtin_trap(), others cannot (x86 for example). If we enhanced __builtin_trap() to take an argument for the trap code, MIPS could be converted. But as it stands now __builtin_trap() is not very useful. As more architectures start adding funky tables that get generated by the inline asm (as in x86), __builtin_trap() becomes less useful. David Daney
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 11:14:01AM -0800, Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > Besides, didn't I see a whole bunch of kernel security patches related > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > suddenly won't get your trap. > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > protection is independent of the generic kernel. > > > > Milage may vary on other architectures, but that's not a concern here. > > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. The discussion at hand is about code contained in the ARM supporting files (arch/arm/kernel/traps.c), rather than the generic kernel. As such, it is not relevant to any architecture other than ARM.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > Besides, didn't I see a whole bunch of kernel security patches related > > to null pointer dereferences lately? If page 0 can be mapped, you > > suddenly won't get your trap. > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > protection is independent of the generic kernel. > > Milage may vary on other architectures, but that's not a concern here. I don't understand, though, why you would want to implement a generally useful facility (make the kernel trap so you can do a post-mortem analysis) in a way that's only safe for the ARM port.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > Besides, didn't I see a whole bunch of kernel security patches related > to null pointer dereferences lately? If page 0 can be mapped, you > suddenly won't get your trap. Page 0 can not be mapped on ARM kernels since the late 1990s, and this protection is independent of the generic kernel. Milage may vary on other architectures, but that's not a concern here.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
Russell King - ARM Linux wrote: > Let me put it another way: I want this function to terminate with an > explicit NULL pointer dereference in every case. __builtin_trap cannot be used because the GCC manual says "The mechanism used may vary from release to release so you should not rely on any particular implementation". It includes calling abort() as a possible implementation - not ideal. This is not related to GCC, but I have an ARM system here where dereferencing NULL does not trap. You guessed it, it doesn't have a regular MMU. But there are other addresses which do trap. They would be a much better choice for BUG(). (Aha! Maybe that's why the kernel just behaves weirdly when it runs out of memory and eventually triggers a watchdog reboot, with no panic message. I'd better try changing BUG() :-) Even with an MMU, sometimes userspace maps page zero. For example, Wine on x86 does that. (It might be possible to change Wine and kernel vm86 to avoid it, but it has not happened). Bug-free userspace behaviour should not stop kernel's BUG() from doing it's basic job of trapping in the kernel! It would be quite messy if userspace maps page zero, and then a kernel BUG() ploughs ahead into __builtin_unreachable() and completely undefined behaviour, possibly leading to worse behaviour than omitting the check which called BUG(). Under those circumstances I'd rather see it use __builtin_trap() if that really does trap :-) The point of the exercise with __builtin_unreachable() is to reduce the kernel size by removing the for(;;) loop. *(int *)0 = 0 isn't the smallest trapping sequence. (When it works :-) So the most compact _and_ reliable sequence for the kernel, on all architectures, is probably: __asm__ volatile("smallest undefined or always-trapping instruction") followed by __builtin_unreachable(), because GCC documentation _does_ say that asm followed by that will execute the asm and assume the asm doesn't return. -- Jamie
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 10:17:18AM -0800, Russell King - ARM Linux wrote: > > It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure > > use > >__builtin_trap (); > > instead for the whole sequence (the unreachable is implied then). > > GCC choses a size-optimal trap representation for your target then. > > How is "size-optimal trap" defined? The point of "*(int *)0 = 0;" is > to cause a NULL pointer dereference which is trapped by the kernel to > produce a full post mortem and backtrace which is easily recognised > as a result of this code. With something like __builtin_trap, the compiler knows that your intent is to cause a trap. But it's asking for trouble, and for future flame wars between kernel developers and gcc developers, to put in sequences that happen to do the right thing if the compiler does no optimizations whatsoever, but that might be messed up by optimizations because they are code sequences whose behavior is undefined. Besides, didn't I see a whole bunch of kernel security patches related to null pointer dereferences lately? If page 0 can be mapped, you suddenly won't get your trap.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 06:17:11PM +0100, Richard Guenther wrote: > On Thu, Dec 17, 2009 at 6:09 PM, David Daney > wrote: > > Jamie Lokier wrote: > >> > >> Uwe Kleine-König wrote: > >>> > >>> Use the new unreachable() macro instead of for(;;); > >>> *(int *)0 = 0; > >>> /* Avoid "noreturn function does return" */ > >>> - for (;;); > >>> + unreachable(); > >> > >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it > >> knows the branch of the code leading to unreachable can never be reached? > >> > > > > I don't know the definitive answer, so I am sending to g...@... > > > > FYI: #define unreachable() __builtin_unreachable() > > It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure > use >__builtin_trap (); > instead for the whole sequence (the unreachable is implied then). > GCC choses a size-optimal trap representation for your target then. How is "size-optimal trap" defined? The point of "*(int *)0 = 0;" is to cause a NULL pointer dereference which is trapped by the kernel to produce a full post mortem and backtrace which is easily recognised as a result of this code. Having gcc decide on, maybe, an undefined instruction instead would be confusing. Let me put it another way: I want this function to terminate with an explicit NULL pointer dereference in every case.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 6:09 PM, David Daney wrote: > Jamie Lokier wrote: >> >> Uwe Kleine-König wrote: >>> >>> Use the new unreachable() macro instead of for(;;); >>> *(int *)0 = 0; >>> /* Avoid "noreturn function does return" */ >>> - for (;;); >>> + unreachable(); >> >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it >> knows the branch of the code leading to unreachable can never be reached? >> > > I don't know the definitive answer, so I am sending to g...@... > > FYI: #define unreachable() __builtin_unreachable() It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure use __builtin_trap (); instead for the whole sequence (the unreachable is implied then). GCC choses a size-optimal trap representation for your target then. Richard. > >> If GCC-4.5 does not, are you sure a future version of GCC will never >> remove it? In other words, is __builtin_unreachable() _defined_ in >> such a way that it cannot remove the previous assignment? >> >> We have seen problems with GCC optimising away important tests for >> NULL pointers in the kernel, due to similar propagation of "impossible >> to occur" conditions, so it's worth checking with GCC people what the >> effect of this one would be. >> >> In C, there is a general theoretical problem with back-propagation of >> optimisations from code with undefined behaviour. In the case of >> __builtin_unreachable(), it would depend on all sorts of unclearly >> defined semantics whether it can remove a preceding *(int *)0 = 0. >> >> I'd strongly suggest asking on the GCC list. (I'd have mentioned this >> earlier, if I'd known about the patch for other architectures). >> >> The documentation for __builtin_unreachable() only says the program is >> undefined if control flow reaches it. In other words, it does not say >> what effect it can have on previous instructions, and I think it's >> quite likely that it has not been analysed in a case like this. >> >> One thing that would give me a lot more confidence, because the GCC >> documentation does mention asm(), is this: >> >>> *(int *)0 = 0; >>> /* Ensure unreachableness optimisations cannot propagate back. *I/ >>> __asm__ volatile(""); >>> /* Avoid "noreturn function does return" */ >>> unreachable(); >> >> -- Jamie > >
Re: [PATCH] ARM: Convert BUG() to use unreachable()
Jamie Lokier wrote: Uwe Kleine-König wrote: Use the new unreachable() macro instead of for(;;); *(int *)0 = 0; /* Avoid "noreturn function does return" */ - for (;;); + unreachable(); Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it knows the branch of the code leading to unreachable can never be reached? I don't know the definitive answer, so I am sending to g...@... FYI: #define unreachable() __builtin_unreachable() If GCC-4.5 does not, are you sure a future version of GCC will never remove it? In other words, is __builtin_unreachable() _defined_ in such a way that it cannot remove the previous assignment? We have seen problems with GCC optimising away important tests for NULL pointers in the kernel, due to similar propagation of "impossible to occur" conditions, so it's worth checking with GCC people what the effect of this one would be. In C, there is a general theoretical problem with back-propagation of optimisations from code with undefined behaviour. In the case of __builtin_unreachable(), it would depend on all sorts of unclearly defined semantics whether it can remove a preceding *(int *)0 = 0. I'd strongly suggest asking on the GCC list. (I'd have mentioned this earlier, if I'd known about the patch for other architectures). The documentation for __builtin_unreachable() only says the program is undefined if control flow reaches it. In other words, it does not say what effect it can have on previous instructions, and I think it's quite likely that it has not been analysed in a case like this. One thing that would give me a lot more confidence, because the GCC documentation does mention asm(), is this: *(int *)0 = 0; /* Ensure unreachableness optimisations cannot propagate back. *I/ __asm__ volatile(""); /* Avoid "noreturn function does return" */ unreachable(); -- Jamie