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

[Thomas Gleixner]

On Mon, Aug 30 2021 at 14:26, Linus Torvalds wrote:
> On Mon, Aug 30, 2021 at 2:07 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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...
>
> 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?"

It took me a while to figure out why this is actually using that
specific fault handler. And yes, I had a major "Whaa?" moment as well.

ASM_EXTABLE_FAULT() was introduced with commit

  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

blessed by a guy named Torvalds :)

commit 92b0729c34ca ("x86/mm, x86/mce: Add memcpy_mcsafe()") made use of
this and it actually did not use the trap number either:

+       .section .fixup, "ax"
+       /* Return false for any failure */
+.L_memcpy_mcsafe_fail:
+       mov     $1, %rax
+       ret

commit b2f9d678e28c ("x86/mce: Check for faults tagged in
EXTABLE_CLASS_FAULT exception table entries") made use of this in MCE to
allow in kernel recovery. The only thing it uses is checking the
exception handler type.

Bah. I'll fix that up to make that less obscure.

The remaining two use cases (SGX and FPU) make use of the stored trap
number.

Thanks,

        tglx