Date:   Mon, 22 Nov 2021 08:14:17 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>
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()
Message-ID: <20211122161417.p3yopenukxbts4gm@treble>
References: <55c7b316-e03d-9e91-d74c-fea63c469b3b@applied-asynchrony.com>
 <CAHk-=wjHbKfck1Ws4Y0pUZ7bxdjU9eh2WK0EFsv65utfeVkT9Q@mail.gmail.com>
 <20211118080627.GH174703@worktop.programming.kicks-ass.net>
 <20211118081852.GM174730@worktop.programming.kicks-ass.net>
 <YZYfYOcqNqOyZ8Yo@hirez.programming.kicks-ass.net>
 <YZZC3Shc0XA/gHK9@hirez.programming.kicks-ass.net>
 <20211119020427.2y5esq2czquwmvwc@treble>
 <YZduix64h64cDa7R@hirez.programming.kicks-ass.net>
 <20211119183544.sragh42cn2liu3pw@treble>
 <YZtjsPXjEsxOU0Zv@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YZtjsPXjEsxOU0Zv@hirez.programming.kicks-ass.net>
Precedence: bulk

On Mon, Nov 22, 2021 at 10:32:32AM +0100, Peter Zijlstra wrote:
> 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.

Realistically there are a limited number of callers to the unwinder.  If
anybody calls with refcount < 2 then we can WARN() and root them out.

It would have found this bug far before any weird races would have been
found.

True, it's not bulletproof, but task unwinder usage is (to some degree)
less critical than oopses.

Now I'm off to disappear for turkey week.  Cheers.

-- 
Josh