Re: [PATCH] x86: Pin task-stack in __get_wchan()

From: Peter Zijlstra <peterz@infradead.org>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Holger Hoffst??tte <holger@applied-asynchrony.com>,
	Qi Zheng <zhengqi.arch@bytedance.com>,
	Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Justin Forbes <jmforbes@linuxtx.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Guenter Roeck <linux@roeck-us.net>, Shuah Khan <shuah@kernel.org>,
	patches@kernelci.org, lkft-triage@lists.linaro.org,
	Pavel Machek <pavel@denx.de>, Jon Hunter <jonathanh@nvidia.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH] x86: Pin task-stack in __get_wchan()
Date: Mon, 22 Nov 2021 10:32:32 +0100	[thread overview]
Message-ID: <YZtjsPXjEsxOU0Zv@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20211119183544.sragh42cn2liu3pw@treble>

On Fri, Nov 19, 2021 at 10:35:44AM -0800, Josh Poimboeuf wrote:
> On Fri, Nov 19, 2021 at 10:29:47AM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 18, 2021 at 06:04:27PM -0800, Josh Poimboeuf wrote:
> > > On Thu, Nov 18, 2021 at 01:11:09PM +0100, Peter Zijlstra wrote:
> > 
> > > > I now have the below, the only thing missing is that there's a
> > > > user_mode() call on a stack based regs. Now on x86_64 we can
> > > > __get_kernel_nofault() regs->cs and call it a day, but on i386 we have
> > > > to also fetch regs->flags.
> > > > 
> > > > Is this really the way to go?
> > > 
> > > Please no.  Can we just add a check in unwind_start() to ensure the
> > > caller did try_get_task_stack()?
> > 
> > I tried; but at best it's fundamentally racy and in practise its worse
> > because init_task doesn't seem to believe in refcounts and kthreads are
> > odd for some raisin. Now those are fixable, but given the fundamental
> > races, I don't see how it's ever going to be reliable.
> 
> I'm probably out of the loop here, but I wonder what races you're
> referring to.

We can do the warn as you suggest, however, it can become 0 right after
we test and then still make the unwder explode.

That is, the test is not sufficient.

> And I assume 'stack_refcount > 0' only needs to be asserted when
> unwinding other tasks, not current.  So it shouldn't affect unwinds
> during boot, or oopses.
> 
> Yes, the unwinder has to be rock solid, but if the caller can't even
> ensure the given task's memory exists, it sounds like a bug in the
> caller that needs a warning.

Well, yes. Still it would be nice if the unwinder would not itself burn
the house, even in that case.