Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early

From: Andy Lutomirski <luto@amacapital.net>
To: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>,
	Andy Lutomirski <luto@kernel.org>, X86 ML <x86@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	KVM list <kvm@vger.kernel.org>,
	Arjan van de Ven <arjan@linux.intel.com>,
	xen-devel <Xen-devel@lists.xen.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v5 4/9] x86/traps: Enable all exception handler callbacks early
Date: Sun, 3 Apr 2016 06:22:10 -0700	[thread overview]
Message-ID: <CALCETrU7LAGXtx-duRJuLYbMtYK=YXGxpV44jKcXWZuyTOQh+Q@mail.gmail.com> (raw)
In-Reply-To: <20160403080737.GA19007@pd.tnic>

On Sun, Apr 3, 2016 at 1:07 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Sat, Apr 02, 2016 at 10:52:48PM +0200, Borislav Petkov wrote:
>> On Sat, Apr 02, 2016 at 01:16:07PM -0700, Andy Lutomirski wrote:
>> > I have no idea why it was explicitly unsupported, but I'm guessing it
>> > was just to avoid duplicating the code.  Early "ext" uaccess failures
>> > are certainly not going to work, but I don't think this is a problem
>> > -- there's no userspace before trap_init runs, so how exactly is an
>> > "ext" uaccess going to happen in the first place?
>> >
>> > In any event, if it did happen in older kernels, it would have
>> > immediately panicked due to that code.  At least with my code it just
>> > might manage to EFAULT correctly.
>>
>> Yeah, I was wondering what that early thing meant.
>>
>> Linus or tip guys probably remember what this whole deal with early
>> uaccess was about. I'll try to do some git archeology tomorrow.
>
> Yep, just as I suspected:
>
> 6a1ea279c210 ("x86, extable: Add early_fixup_exception()")
>
> Apparently, thread_info might not have been setup yet. I'm guessing the
> intention behind this no-uaccess-fixup-early is to not even attempt any
> fixup due to stuff *probably* not initialized yet and so the safer thing
> would be to panic instead.
>
> I'm wondering whether making it try to EFAULT correctly is the right
> thing to do... We're certainly more conservative if we panic and not
> allow some silently failed attempt at recovery which looks successful,
> to continue.
>
> hpa, thoughts?

I don't think this matters much.  There aren't many users of this
mechanism in the tree:

./arch/x86/kernel/signal.c:    get_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    put_user_try {
./arch/x86/kernel/signal_compat.c:    get_user_try {
./arch/x86/kernel/vm86_32.c:    put_user_try {
./arch/x86/kernel/vm86_32.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    get_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/ia32/ia32_signal.c:    put_user_try {
./arch/x86/include/asm/uaccess.h: * {get|put}_user_try and catch
./arch/x86/include/asm/uaccess.h: * get_user_try {
./arch/x86/include/asm/uaccess.h:#define get_user_try        uaccess_try
./arch/x86/include/asm/uaccess.h:#define put_user_try        uaccess_try

I don't see how we could get to that code in the first place without
current_thread_info() working.

If we can ever convince gcc to do jump labels properly for uaccess, it
would probably be better to just delete all that code.

--Andy