bug: Exclude non-BUG/WARN exceptions from report_bug()

Commit:

b8347c219649 ("x86/debug: Handle warnings before the notifier chain, to fix KGDB crash")

changed the ordering of fixups, and did not take into account the case of x86 processing non-WARN() and non-BUG() exceptions. This would lead to output of a false BUG line with no other information. In the case of a refcount exception, it would be immediately followed by the refcount WARN(), producing very strange double-"cut here":

lkdtm: attempting bad refcount_inc() overflow

------------[ cut here ]------------
Kernel BUG at 0000000065f29de5 [verbose debug info unavailable]
------------[ cut here ]------------
refcount_t overflow at lkdtm_REFCOUNT_INC_OVERFLOW+0x6b/0x90 in cat[3065], uid/euid: 0/0
WARNING: CPU: 0 PID: 3065 at kernel/panic.c:657 refcount_error_report+0x9a/0xa4

...

In the prior ordering, exceptions were searched first:

do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
...
if (fixup_exception(regs, trapnr))
return 0;

- if (fixup_bug(regs, trapnr))
- return 0;
-

As a result, fixup_bugs()'s is_valid_bugaddr() didn't take into account needing to search the exception list first, since that had already happened.

So, instead of searching the exception list twice (once in is_valid_bugaddr() and then again in fixup_exception()), just add a simple sanity check to report_bug() that will immediately bail out if a BUG() (or WARN()) entry is not found.

---
Resending through akpm since this technically isn't x86-specific.

---
 lib/bug.c | 2 ++
 1 file changed, 2 insertions(+)

index c1b0fad31b10..551e4d405307 100644
@@ -150,6 +150,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 		return BUG_TRAP_TYPE_NONE;
 
 	bug = find_bug(bugaddr);
+	if (!bug)
+		return BUG_TRAP_TYPE_NONE;
 
 	file = NULL;
 	line = 0;

2.7.4


-- 
Kees Cook
Pixel Security

Just a note on this part, because I think this is a general issue (and has nothing to do with the particular patch itself).

Saying who you expect things to go through can definitely be a good idea, because I generally ignore patches that get sent to me from people I take pull requests from - I assume that I'm on the participants list as a heads-up for for comments, rather than applying. In fact, even if you don't send me pull requests, that's generally what I assume, because the bulk of patches should be going through some submaintainer that _does_ send me pull requests (or, like Andrew, is known to send patches).

I'd expect that many other people face the same issue - particularly when a patch doesn't have a very clear area, and there are multiple people on the participants list, it's easy to assume that the intended target is "somebody else". I do not, for example, tend to look at my emails so closely that I care who is in the "To:" field, and who is in the "Cc:" fields. I'd have to be very curious indeed to care that deeply.

Which brings me to my point: maybe we should encourage people to make this "for whom the patch tolls" information more obvious and more explicit. It wasn't obvious in the first submission (yes, I saw the patch then too), and even in this second one I actually initially didn't notice this one line in between the commit message and the actual patch. Maybe I get too much email, but I bet _that_ is very true of others too.

The obvious place to encourage people to do it is in the [PATCH] part in the subject, ie something like [PATCH/mm] or similar if you expect it to go through Andrew's mm tree, or [PATCH/x86] it you expect the x86 maintainers to pick it up. Or [PATCH/linus] if it's something that you really expect to bypass all maintainers (why? I'd prefer for that to then be explained).

But maybe other potential patch recipients would hate that kind of extra mess in the subject line?

Linus

Yeah, a lot of people miss the "comment" line. I try to use it sparingly, but really that only contributes to it not getting noticed. ;)

This may be redundant for some cases where the target is already in the commit subject prefix: "x86/locking: Fixes foo", but I regularly touch code without super-clear maintainership, or end up in places where it spans multiple areas (e.g. this patch was a fix for an x86-tree commit but the fix only touches the generic bug area, whee).

But for non-"obvious" cases, I like this idea; it could help me when I'm sending to lots of different trees.

My question would be, will the existing automated systems that parse the "PATCH" subject deal with a non-whitespaced suffix like this?

-Kees

Kees Cook
Pixel Security

Hmm. Maybe just space them out. That's what networking already does, ie you'll see things like

[PATCH net-next v3 0/5] patch description here

[PATCH net] some-patch-description

in subject lines. Maybe we can just encourage that format in general.

And yes, I agree, for when the targets are obvious, this clearly isn't needed. And often they are.

So this would still likely be the exception rather than the rule, but it would be a lot more obvious than hiding a one-liner commentary deep in the middle of the email.

Linus

At least for me (as the wireless-drivers maintainer) this would be a major improvement as it's not always clear to which to tree a patch should be applied and it would save unnecessary ping pong when I need to ask which tree is the patch going to. I think few times I have even accidentally applied a patch which Dave has already applied to the net tree because of the target tree was not clearly marked.

So at least I would very much welcome having this documented somewhere in Documentation so that I can start convincing people to use it more :)

Kalle Valo