Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()

From: Thomas Gleixner <tglx@linutronix.de>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Dan Williams <dan.j.williams@intel.com>
Cc: Borislav Petkov <bp@alien8.de>,
	LKML <linux-kernel@vger.kernel.org>,
	the arch/x86 maintainers <x86@kernel.org>
Subject: Re: [patch 01/10] x86/fpu/signal: Clarify exception handling in restore_fpregs_from_user()
Date: Tue, 31 Aug 2021 00:01:35 +0200	[thread overview]
Message-ID: <87r1eafv7k.ffs@tglx> (raw)
In-Reply-To: <CAHk-=wgbeNyFV3pKh+hvh-ZON3UqQfkCWnfLYAXXA9cX2iqsyg@mail.gmail.com>

On Mon, Aug 30 2021 at 14:26, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> Incidentally, why do we bother with negation in those?  Why not have
>> user_insn(), XSTATE_OP() and kernel_insn_err() return 0 or trap
>> number...

Correct.

> I really wish we didn't have that odd _ASM_EXTABLE_FAULT/
> ex_handler_fault() special case at all.
>
> It's *very* confusing, and it actually seems to be mis-used. It looks
> like the "copy_mc_fragile" code uses it by mistake, and doesn't
> actually want that "modify %%rax" behavior of that exception handler
> AT ALL.
>
> If I read that code correctly, it almost by mistake doesn't actually
> care, and will overwrite %%rax with the right result, but it doesn't
> look like the "fault code in %eax" was ever *intentional*. There's no
> mention of it.
>
> Maybe I'm misreading that code, but I look at it and just go "Whaa?"

Ooops. I never looked at that usage site. It indeed does not make use of
that information. The original __mcsafe_copy() made use of it, but that
got removed/replaced long ago.

The other user is SGX which actually uses the trap number in EAX for
failure analysis.

> The code in user_insn() clearly *does* use that fault number (and, as
> you say, inverts it for some reason), but I wonder how much it really
> cares? Could we get rid of it, and just set a fixed error code?
>
> I only checked one user, but that one didn't actually care which fault
> it was, it only cared about fault-vs-no-fault.

The usage sites of user_insn() and XSTATE_OP() need to distinguish:

   - success
   - fail due to #PF (which can be tried to handle)
   - fail due to some other exception (#GP, #MC)

I found that _ASM_EXTABLE_FAULT() mechanism pretty conveniant for this
and the negation was just me being lazy after I discovered that
X86_TRAP_PF == EFAULT. It turned out not to be a brilliant idea, but at
the time it looked great...

So yes, the negation does not matter, but the ability to check whether
the fail was caused by #PF or not matters.

Thanks,

        tglx