Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1495189 > unrolled thread
| Started by | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| First post | 2016-10-04 06:10 +0200 |
| Last post | 2016-10-06 04:20 +0200 |
| Articles | 12 on this page of 32 — 9 participants |
Back to article view | Back to linux.kernel
BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-04 06:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Greg KH <greg@kroah.com> - 2016-10-04 10:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Johannes Weiner <hannes@cmpxchg.org> - 2016-10-04 11:40 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 03:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Johannes Weiner <hannes@cmpxchg.org> - 2016-10-05 11:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Johannes Weiner <hannes@cmpxchg.org> - 2016-10-05 11:40 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Jan Kara <jack@suse.cz> - 2016-10-05 12:50 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 18:20 +0200
Re: [PATCH] checkpatch: extend BUG warning Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 19:10 +0200
[PATCH] checkpatch: extend BUG warning Joe Perches <joe@perches.com> - 2016-10-05 19:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-10-05 04:50 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 05:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Willy Tarreau <w@1wt.eu> - 2016-10-05 07:50 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 18:00 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Willy Tarreau <w@1wt.eu> - 2016-10-05 21:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 21:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Willy Tarreau <w@1wt.eu> - 2016-10-05 23:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-05 23:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-05 23:50 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-06 00:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-06 00:40 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-07 00:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-07 00:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-07 01:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-07 02:00 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Willy Tarreau <w@1wt.eu> - 2016-10-07 08:00 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-07 19:20 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-07 19:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Kees Cook <keescook@chromium.org> - 2016-10-07 19:40 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Willy Tarreau <w@1wt.eu> - 2016-10-07 20:30 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Dave Chinner <david@fromorbit.com> - 2016-10-06 04:10 +0200
Re: BUG_ON() in workingset_node_shadows_dec() triggers Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-06 04:20 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-10-06 00:40 +0200 |
| Message-ID | <sp310-8gt-13@gated-at.bofh.it> |
| In reply to | #1496094 |
On Wed, Oct 5, 2016 at 3:17 PM, Kees Cook <keescook@chromium.org> wrote:
>
> With my more paranoid desires, I would prefer to keep "stop kernel
> execution with the state set up by this process", not just "make the
> process never return to user-space".
Quite honestly, I think the answer to that is: "No. Not by default".
So with some kind of kernel command line option, yes, kind of like
"reboot_on_oops" (or whatever it is - I've never used it ;)
>> And *if* we make BUG() actually do something sane (non-trapping), we
>> can easily make it be generic, not arch-specific. In fact, I'd
>> implement it by just adding a "handle_bug()" in kernel/panic.c...
>
> Yeah, I'm not sure what the right next step would be. Do we need a new
> set of functions between WARN and BUG? Or maybe extract the
> process-killing logic on a per-arch level and make it a specific API
> so that it can be explicitly called as part of error-handling? Hmm
So the process-killing logic actually used to historically just be
"call do_exit()". In fact, that's what most architectures still do in
their error paths. And it's what a lot of people who just want to kill
the current code do.
So calling "do_exit()" is actually perfectly fine. It's just that
calling do_exit() from BUG_ON() is a major pain, because of the
asynchronous nature of BUG_ON(). But if you are in a regular system
call and don't hold any locks, do_exit() is still fine.
In fact, all that x86 really does differently from do_exit() in the
fault path is to reset the stack pointer first, so that you don't get
stack smashers when you have recursive faults (which used to be one
really nasty failure case, not just with BUG_ON(), but with any kernel
oops in general). So on x86, the crash code actually calls a function
called "rewind_stack_do_exit()" instead.
But the name gives it away: it's the exact same thing.
So you can actually do a generic BUG_ON() (even with the current
semantics) pretty much today by just having a config option that the
architecture can set to specify whether you should just call
"do_exit()" or "rewind_stack_do_exit()" to do that final killing
action.
There's a few other possible gotcha's (the code is hard to follow
because the normal implementation uses a trapping instruction and
hides the BUG() information in the text, so you get the whole fault
path), but on the whole I think it should be fairly straightforward do
just get rid of all the arch code, and replace it with a generic
function that can then decide internally whether it wants to just
warn, whether it wants to SIGKILL, or whether it wants to do the
traditional thing and just force do_exit(). Or do new things like
reboot or just halt.
But it really would be very nice to never have do_exit() have to worry
about odd callers. We've had a *lot* of trouble over the years with
deadlocks on critical locks in do_exit(), for example.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-10-07 00:10 +0200 |
| Message-ID | <spp1v-72A-21@gated-at.bofh.it> |
| In reply to | #1496104 |
On Wed, Oct 5, 2016 at 3:29 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 5, 2016 at 3:17 PM, Kees Cook <keescook@chromium.org> wrote: >> >> With my more paranoid desires, I would prefer to keep "stop kernel >> execution with the state set up by this process", not just "make the >> process never return to user-space". > > Quite honestly, I think the answer to that is: "No. Not by default". > So with some kind of kernel command line option, yes, kind of like > "reboot_on_oops" (or whatever it is - I've never used it ;) > >>> And *if* we make BUG() actually do something sane (non-trapping), we >>> can easily make it be generic, not arch-specific. In fact, I'd >>> implement it by just adding a "handle_bug()" in kernel/panic.c... >> >> Yeah, I'm not sure what the right next step would be. Do we need a new >> set of functions between WARN and BUG? Or maybe extract the >> process-killing logic on a per-arch level and make it a specific API >> so that it can be explicitly called as part of error-handling? Hmm > > So the process-killing logic actually used to historically just be > "call do_exit()". In fact, that's what most architectures still do in > their error paths. And it's what a lot of people who just want to kill > the current code do. > > So calling "do_exit()" is actually perfectly fine. It's just that > calling do_exit() from BUG_ON() is a major pain, because of the > asynchronous nature of BUG_ON(). But if you are in a regular system > call and don't hold any locks, do_exit() is still fine. Well, that's the problem I repeatedly ran into: locks. And actually, more than that. Even in seccomp when trying to do a clean death of a process (without the paranoid requirement that kernel execution for that syscall stop), I've run into problems with do_exit() -- in that case it was bad ptrace state assumptions during signal delivery. I originally used do_exit with the hardened usercopy stuff and that would fall all over itself under lkdtm testing, since locks were held. The "cleanest" way to handle it seemed to be the lock-busting logic already built into BUG, so I moved to that. > In fact, all that x86 really does differently from do_exit() in the > fault path is to reset the stack pointer first, so that you don't get > stack smashers when you have recursive faults (which used to be one > really nasty failure case, not just with BUG_ON(), but with any kernel > oops in general). So on x86, the crash code actually calls a function > called "rewind_stack_do_exit()" instead. > > But the name gives it away: it's the exact same thing. > > So you can actually do a generic BUG_ON() (even with the current > semantics) pretty much today by just having a config option that the > architecture can set to specify whether you should just call > "do_exit()" or "rewind_stack_do_exit()" to do that final killing > action. > > There's a few other possible gotcha's (the code is hard to follow > because the normal implementation uses a trapping instruction and > hides the BUG() information in the text, so you get the whole fault > path), but on the whole I think it should be fairly straightforward do > just get rid of all the arch code, and replace it with a generic > function that can then decide internally whether it wants to just > warn, whether it wants to SIGKILL, or whether it wants to do the > traditional thing and just force do_exit(). Or do new things like > reboot or just halt. > > But it really would be very nice to never have do_exit() have to worry > about odd callers. We've had a *lot* of trouble over the years with > deadlocks on critical locks in do_exit(), for example. Right, so I see a number of "features" that are mixed together between do_exit, WARN, BUG, and panic, and it seems code wants various combinations of them: - report details about an unexpected condition occurring (BUG, actually, does NOT do this: it doesn't take any kind of format argument, but arch-specific internal traps DO, before calling the rest of the internals that BUG is wired up to.) - dump CPU and stack state - kill current - kill current's entire thread group - stop kernel execution from continuing - halt the system - reboot the system By far the most problematic is "stop kernel execution from continuing", but that's currently the behavior that BUG depends on, so replacing BUG with anything needs to either fix the surrounding logic to fail sanely or we have the keep the feature. I remain convinced, though, that "stop this thread of kernel execution" is not the same as "reboot the system". -Kees -- Kees Cook Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-10-07 00:30 +0200 |
| Message-ID | <sppkS-7h3-25@gated-at.bofh.it> |
| In reply to | #1496881 |
On Thu, Oct 6, 2016 at 3:07 PM, Kees Cook <keescook@chromium.org> wrote:
> The "cleanest" way to handle it seemed to be the lock-busting logic
> already built into BUG, so I moved to that.
Heh. The lock-busting logic in BUG() has always been broken. It's been
random hacks. It doesn't actually work in any general case, it just
occasionally happens to get things right. Mostly it tries to handle
the console locking (the whole "oops_in_progress" magic) so that if
you have a BUG_ON() in bad areas, at least you still end up getting
output.
But no, it's not reliable in any way, shape or form. That's really why
you want to continue after a BUG().
> By far the most problematic is "stop kernel execution from
> continuing", but that's currently the behavior that BUG depends on, so
> replacing BUG with anything needs to either fix the surrounding logic
> to fail sanely or we have the keep the feature.
Well, I'm not sure how much we actually end up depending on it,
considering that we now have two examples of BUG() implementations
that actually do _not_ depend on stopping execution: both the sound
subsystem and the XFS version of BUG_ON() end up not actually doing
the BUG() thing.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-10-07 01:10 +0200 |
| Message-ID | <sppXA-7O8-9@gated-at.bofh.it> |
| In reply to | #1496889 |
On Thu, Oct 6, 2016 at 3:29 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Thu, Oct 6, 2016 at 3:07 PM, Kees Cook <keescook@chromium.org> wrote: >> The "cleanest" way to handle it seemed to be the lock-busting logic >> already built into BUG, so I moved to that. > > Heh. The lock-busting logic in BUG() has always been broken. It's been > random hacks. It doesn't actually work in any general case, it just > occasionally happens to get things right. Mostly it tries to handle > the console locking (the whole "oops_in_progress" magic) so that if > you have a BUG_ON() in bad areas, at least you still end up getting > output. It seems to handle other things too, file descriptors, I think? Some giant warning, I think about fds, went away when I switched from do_exit() to BUG(). I'd have to go look more closely. > But no, it's not reliable in any way, shape or form. That's really why > you want to continue after a BUG(). Yeah, agreed about the unreliability. It's why I'm a fan of panic_on_oops. :P (Except when doing lots of tests under lkdtm, then I like having multiple Oopses without rebooting, but perhaps that is literally the only use-case...) >> By far the most problematic is "stop kernel execution from >> continuing", but that's currently the behavior that BUG depends on, so >> replacing BUG with anything needs to either fix the surrounding logic >> to fail sanely or we have the keep the feature. > > Well, I'm not sure how much we actually end up depending on it, > considering that we now have two examples of BUG() implementations > that actually do _not_ depend on stopping execution: both the sound > subsystem and the XFS version of BUG_ON() end up not actually doing > the BUG() thing. Yeah, for sure. I didn't mean to imply they all depended on it, just that finding those that do will require manual inspection. We'll not be able to do a flag-day on BUG until we fix everything. -Kees -- Kees Cook Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-10-07 02:00 +0200 |
| Message-ID | <spqK2-85w-7@gated-at.bofh.it> |
| In reply to | #1496913 |
On Thu, Oct 6, 2016 at 4:05 PM, Kees Cook <keescook@chromium.org> wrote:
>
> It seems to handle other things too, file descriptors, I think? Some
> giant warning, I think about fds, went away when I switched from
> do_exit() to BUG(). I'd have to go look more closely.
I think you must have changed something else too. I can't think of
what else there is than the crazy "oops_in+_progress" hacks. We used
to reset the preempt counter too, but with that being per-thread I
don't think that even matters.
So I think you may have some voodoo programming there.
An an oops (but not a do_exit()) will add a taint, and there's the
notifications that might do random things (mainly kgdb and tracing).
But that should be pretty much it.
> Yeah, for sure. I didn't mean to imply they all depended on it, just
> that finding those that do will require manual inspection. We'll not
> be able to do a flag-day on BUG until we fix everything.
Not true. That kind of thinking just says "we can never change BUG at all".
You'll never fix anything that way.
We should just switch BUG() over and be done with it. The whole point
it that since it should never trigger in the first place, the
semantics on BUG() should never matter.
And if you have some code that depends on the semantics of BUG(), that
code is buggy crap *by*definition*.
And there's no way we'll let that kind of shit determine kernel
development. That would be insane.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Willy Tarreau <w@1wt.eu> |
|---|---|
| Date | 2016-10-07 08:00 +0200 |
| Message-ID | <spwmm-416-3@gated-at.bofh.it> |
| In reply to | #1496943 |
On Thu, Oct 06, 2016 at 04:59:20PM -0700, Linus Torvalds wrote: > We should just switch BUG() over and be done with it. The whole point > it that since it should never trigger in the first place, the > semantics on BUG() should never matter. > > And if you have some code that depends on the semantics of BUG(), that > code is buggy crap *by*definition*. I totally agree with this. If a developer writes BUG() somewhere, it means he doesn't see how it is possible to end up in this situation. Thus we cannot hope that the BUG() call is doing anything right to fix what the code author didn't expect to happen. It just means "try to limit the risks but I don't really know which ones". Also we won't make things worse. Where people currently have an oops, they'll get one or more warnings. The side effects (lockups, panic, etc) will more or less be the same, but many of us already don't want to continue after an oops and despite this our systems work fine, so I don't see why anyone would suffer from such a change. However some developers may get more details about issues than what they could get in the past. Willy
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-10-07 19:20 +0200 |
| Message-ID | <spGYq-3AS-31@gated-at.bofh.it> |
| In reply to | #1497028 |
On Thu, Oct 6, 2016 at 10:48 PM, Willy Tarreau <w@1wt.eu> wrote: > On Thu, Oct 06, 2016 at 04:59:20PM -0700, Linus Torvalds wrote: >> We should just switch BUG() over and be done with it. The whole point >> it that since it should never trigger in the first place, the >> semantics on BUG() should never matter. >> >> And if you have some code that depends on the semantics of BUG(), that >> code is buggy crap *by*definition*. > > I totally agree with this. If a developer writes BUG() somewhere, it > means he doesn't see how it is possible to end up in this situation. > Thus we cannot hope that the BUG() call is doing anything right to > fix what the code author didn't expect to happen. It just means > "try to limit the risks but I don't really know which ones". > > Also we won't make things worse. Where people currently have an oops, > they'll get one or more warnings. The side effects (lockups, panic, > etc) will more or less be the same, but many of us already don't want > to continue after an oops and despite this our systems work fine, so > I don't see why anyone would suffer from such a change. However some > developers may get more details about issues than what they could get > in the past. Fair enough. I'll put something together for at least my use-cases and see how ugly it gets in testing. :) I actually started on something like this for the CONFIG_DEBUG_LIST, which had to deal with the logic of "continue after WARN or abort after BUG" etc... Regardless, I still think that we can't let BUG continue kernel execution though, since it may lead to entirely unexpected behavior (possibly security-sensitive) by still running. Upgrading BUG to panic(), though, I'd be fine with, as a way to get people to convert to WARN. -Kees -- Kees Cook Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-10-07 19:30 +0200 |
| Message-ID | <spH86-3EG-51@gated-at.bofh.it> |
| In reply to | #1497379 |
On Fri, Oct 7, 2016 at 10:16 AM, Kees Cook <keescook@chromium.org> wrote:
>
> Regardless, I still think that we can't let BUG continue kernel
> execution though, since it may lead to entirely unexpected behavior
> (possibly security-sensitive) by still running. Upgrading BUG to
> panic(), though, I'd be fine with, as a way to get people to convert
> to WARN.
No. Really. You can upgrade BUG() to "panic()" with a kernel command
line. But not by default.
I'm not going to take any patches that make BUG() even *worse*. That
would be insane. I'm not insane.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2016-10-07 19:40 +0200 |
| Message-ID | <spHhL-3I7-11@gated-at.bofh.it> |
| In reply to | #1497406 |
On Fri, Oct 7, 2016 at 10:21 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Oct 7, 2016 at 10:16 AM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Regardless, I still think that we can't let BUG continue kernel
>> execution though, since it may lead to entirely unexpected behavior
>> (possibly security-sensitive) by still running. Upgrading BUG to
>> panic(), though, I'd be fine with, as a way to get people to convert
>> to WARN.
>
> No. Really. You can upgrade BUG() to "panic()" with a kernel command
> line. But not by default.
>
> I'm not going to take any patches that make BUG() even *worse*. That
> would be insane. I'm not insane.
I'll quit debating how to change things, but I'll just try to point
out that the "stop execution" logic, currently, is not an accident.
Without CONFIG_BUG, BUG is defined as "do {} while (1)", and without
CONFIG_HAVE_ARCH_BUG, BUG is defined as "printk(...); panic(...);".
-Kees
--
Kees Cook
Nexus Security
[toc] | [prev] | [next] | [standalone]
| From | Willy Tarreau <w@1wt.eu> |
|---|---|
| Date | 2016-10-07 20:30 +0200 |
| Message-ID | <spI49-4kf-1@gated-at.bofh.it> |
| In reply to | #1497409 |
Hi Kees,
On Fri, Oct 07, 2016 at 10:33:33AM -0700, Kees Cook wrote:
> I'll quit debating how to change things, but I'll just try to point
> out that the "stop execution" logic, currently, is not an accident.
> Without CONFIG_BUG, BUG is defined as "do {} while (1)", and without
> CONFIG_HAVE_ARCH_BUG, BUG is defined as "printk(...); panic(...);".
I think we're all convinced about this *initial* intent. However among
the 3197 BUG() and 9594 BUG_ON() that are present in v4.8, how many
should *really* be of them ? I'm seeing that during 4.8 development
cycle alone, we managed to add 81 BUG() and 55 BUG_ON(). I doubt we
found so many valid reasons to kill the system. 38 of them were added
to drivers/. The problem is that this "style" has accumulated over the
years. We only had 1739 BUG() and 1801 BUG_ON() in 2.6.12. So we
roughly multiplied that by 4 in 11 years.
The current trend seems to actually be to remove some of them, 3 were
removed from lib/, 4 from include/, 29 removed from fs/, one removed
from mm/ but two added to kernel/ and 3 other ones to net/.
Maybe changing only kernel/ and mm/'s BUG() occurrences to something
like "I_KNOW_I_WILL_BE_BLAMED_FOR_THIS_BUG()" and letting them kill
until they're properly audited, and leaving the other ones non-fatal
could be a reasonable tradeoff to start with ?
Willy
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-10-06 04:10 +0200 |
| Message-ID | <sp6id-20i-1@gated-at.bofh.it> |
| In reply to | #1495681 |
On Tue, Oct 04, 2016 at 08:29:00PM -0700, Linus Torvalds wrote:
> On Tue, Oct 4, 2016 at 7:43 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> >
> > A couple years ago Ingo had an idea to kill BUG_ON abuse that I made
> > a 1st pass at. Back then it seemed nobody cared. Maybe that has since
> > changed?
> >
> > https://lkml.org/lkml/2014/4/30/359
>
> So we actually have the checkpatch warning already:
>
> # avoid BUG() or BUG_ON()
> if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
> my $msg_type = \&WARN;
> $msg_type = \&CHK if ($file);
> &{$msg_type}("AVOID_BUG",
> "Avoid crashing the kernel - try
> using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" .
> $herecurr);
> }
>
> but it doesn't trigger on VM_BUG_ON().
>
> And I'm not convinced about replacing things with BUG_ON_AND_HALT(),
> it simply doesn't fix the existing issue we have: people use BUG_ON(),
> and worse, _when_ they use BUG_ON(), they use it instead of error
> handling, so the code _around_ the BUG_ON() tends to then very much
> depend on what the BUG_ON() checks.
>
> This is actually one way that VM_BUG_ON() is better: it's very much by
> design something that can be compiled away, so at least hopefully
> nobody thinks of it as a security measure. So we could just say that
> we will treat VM_BUG_ON() as a WARN_ON_ONCE(), and just not kill the
> machine.
In XFS, we use ASSERT() (could be XFS_BUG_ON() for all
that the name matters) but we only define that to BUG_ON if
CONFIG_XFS_DEBUG=y.
For "production debug" kernels we have CONFIG_XFS_WARN=y, which
turns ASSERT() into WARN_ON(). We get the warnings, but none of the
crashiness that are desirable in a development context. This is what
distro debug kernels should be using, as it also ensures
we don't build in the real debug code that does things that would
affect prodution systems adversely, like randomly take different
allocator paths to ensure we get code coverage of all the allocator
algorithms...
i.e. production kernels ship with neither set, the debug kernel
ships with CONFIG_XFS_WARN=y, and we do all our development with
CONFIG_XFS_DEBUG=y.
I think this case falls into the "production debug" classification;
we want a warning, but we don't want the system to be taken down....
> But apart from the checkpatch thing, it's actually a pretty big change.
Yeah, that's why we added CONFIG_XFS_WARN=y to do this - it was a 20
line change to add XFS_CONFIG_WARN instead of having to audit and
modify ~1800 call sites to do something differently. And because we
know that ASSERT() is not present in all kernels, it isn't ever used
as a replacement for error handling. Perhaps that's the simplest
solution here as well....
Just my 2c worth.
-Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2016-10-06 04:20 +0200 |
| Message-ID | <sp6rT-2cz-7@gated-at.bofh.it> |
| In reply to | #1496137 |
On Wed, Oct 5, 2016 at 6:59 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> In XFS, we use ASSERT() (could be XFS_BUG_ON() for all
> that the name matters) but we only define that to BUG_ON if
> CONFIG_XFS_DEBUG=y.
>
> For "production debug" kernels we have CONFIG_XFS_WARN=y, which
> turns ASSERT() into WARN_ON(). We get the warnings, but none of the
> crashiness that are desirable in a development context.
Yes. that sounds very much like the right kind of decision.
Forcing crashes can be very useful for the actual developer that is
doing development on the code itself, kind of a "fail fast, fail
hard".
But users (or developers that are developing something _else_ than XFS
;) don't tend to like it.
Linus
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web