Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190228145450.289603901@infradead.org> <20190228150152.540038736@infradead.org>
In-Reply-To: <20190228150152.540038736@infradead.org>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 28 Feb 2019 08:49:57 -0800
Message-ID: <CAHk-=wgsSXxL5c2Lbmbai=bcCgN0G=jPO+gWhQ3a+wx-LTaFkQ@mail.gmail.com>
Subject: Re: [PATCH 6/8] i915,uaccess: Fix redundant CLAC
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>, Peter Anvin <hpa@zytor.com>,
        Julien Thierry <julien.thierry@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Andy Lutomirski <luto@amacapital.net>,
        Ingo Molnar <mingo@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        James Morse <james.morse@arm.com>, valentin.schneider@arm.com,
        Brian Gerst <brgerst@gmail.com>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Andrew Lutomirski <luto@kernel.org>,
        Borislav Petkov <bp@alien8.de>,
        Denys Vlasenko <dvlasenk@redhat.com>,
        Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
        Chris Wilson <chris@chris-wilson.co.uk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 28, 2019 at 7:05 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x3c: redundant UACCESS disable
> drivers/gpu/drm/i915/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x66: redundant UACCESS disable
>
> AKA. you don't need user_access_end() if user_access_begin() fails.

NOOO!

This is complete garbage, and will end up running with AC set forever after.

PeterZ, you need to remove that "redundant UACCESS disable" check, or
make it a whole lot smarter. Because as it is, it's horribly horribly
wrong.

We absolutely _have_ to have that "user_access_end()" there.

Yes, it is redundant (but harmlessly so) if no fault occurs.

But if a fault occurs, that "user_access_end()" is what clears AC on
the faulting path. That's absolutely required, because we don't clear
it on return from exception (and we shouldn't - one common pattern for
user space exceptions is "try to do a big access, if that fails go
back to using small accesses until it fails again").

Your reachability clearly doesn't take exception handling into account.

That patch must die, and your incorrect reachability model must be fixed.

Right now the objtool rules seem to be worse than not using objtool,
if it causes these kinds of false positives.

              Linus