linux-kernel - Re: [PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140402153713.18e3444e@gandalf.local.home>
Date:	Wed, 2 Apr 2014 15:37:13 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 0/2] x86: Fix perf deadlock caused by dumpstack cleanup

On Wed, 2 Apr 2014 11:46:38 -0700
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Wed, Apr 2, 2014 at 10:26 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > Here are two patches that fix the deadlock that you discovered.
> >
> > The first patch is the real culprit, but as I was looking at the code
> > I realized that the irq stack part was a bit off too. That part didn't
> > cause the lock up, but needs to be fixed regardless.
> >
> > Note, I'm hoping to use these clean ups to make x86_64 and i386 code
> > a bit closer to each other, so I hope the original change does not
> > get reverted.
> 
> I'll apply them.
> 
> That said, I'm not *AT*ALL* convinced that a "cleanup" that involves
> having helper functions with multiple bugs and now seven (7!)
> arguments passed into it is a "cleanup" at all.
> 

Yeah, I didn't like it after the second patch :-(

> The cleanup was claimed to help improve readability. Really? Somebody
> needs to reconsider their goals in life if they think that a
> 7-argument helper functions where the caller needs to pass in
> addresses to variables because the helper function will change them is
> a good idea. Just the *call* is spread out over two lines because
> calling that function is so complex.

The helper can be merged back to the main function as it doesn't seem
to be much of a helper anymore.

> 
> I suspect that maybe creating some kind of "struct stack_information"
> structure might help somewhat. Because right now I'm very doubtful
> about the whole "this clarifies/simplifies" argument. The very fact
> that the "cleaned-up" function was a buggy pile of sh*t should make
> you question the cleanup itself.

Well, actually, it proves that there's a clean up needed, as the reason
for the bugs was due to the subtle interactions of the original code.
That said, I agree, the cleanup itself isn't much of a cleanup. But I
intend on continuing to cleanup this code, and remove all the subtle
interactions that is going on here.

This is an ongoing project and what you see here is not the end result.
I am quite ashamed of the bugs though.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/