Re: [PATCHV5 3/3] x86, ras: Add __mcsafe_copy() function to recover from machine checks

[Andy Lutomirski]

On Sat, Dec 26, 2015 at 6:08 PM, Tony Luck <tony.luck@xxxxxxxxx> wrote:
> On Sat, Dec 26, 2015 at 6:54 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>> On Dec 26, 2015 6:33 PM, "Borislav Petkov" <bp@xxxxxxxxx> wrote:
>>> Andy, why is that? It makes the exception handling much simpler this way...
>>>
>>
>> I like the idea of moving more logic into C, but I don't like
>> splitting the logic across files and adding nasty special cases like
>> this.
>>
>> But what if we generalized it?  An extable entry gives a fault IP and
>> a landing pad IP.  Surely we can squeeze a flag bit in there.
>
> The clever squeezers have already been here. Instead of a pair
> of 64-bit values for fault_ip and fixup_ip they managed with a pair
> of 32-bit values that are each the relative offset of the desired address
> from the table location itself.
>
> We could make one of them 31-bits (since even an "allyesconfig" kernel
> is still much smaller than a gigabyte) to free a bit for a flag. But there
> are those external tools to pre-sort exception tables that would all
> need to be fixed too.
>
> Or we could direct the new fixups into a .fixup2 ELF section and put
> begin/end labels around that ... so we could check the address of the
> fixup to see whether it is a legacy or new format entry.
>

Either of those sounds good to me.

>> set the bit, you get an extended extable entry.  Instead of storing a
>> landing pad, it stores a pointer to a handler descriptor:
>>
>> struct extable_handler {
>>   bool (*handler)(struct pt_regs *, struct extable_handler *, ...):
>> };
>>
>> handler returns true if it handled the error and false if it didn't.
>
> It may be had to call that from the machine check handler ... the
> beauty of just patching the IP and returning from the handler was
> that it got us out of machine check context.

Your handler will need to know that it's in machine check context :)
In most cases (e.g. yours), the handler should just modify regs and
return.

>
>> The "..." encodes the fault number, error code, cr2, etc.  Maybe it
>> would be "unsigned long exception, const struct extable_info *info"
>> where extable_info contains a union?  I really wish C would grow up
>> and learn about union types.
>
> All this is made more difficult because the h/w doesn't give us
> all the things we might want to know (e.g. the virtual address).
> We just have a physical address (which may be missing some
> low order bits).

True.  I'm afraid that nothing I suggest can possibly help you there.

Anyhow, this could be a decent signature:

bool (*handler)(struct pt_regs *, struct extable_handler *, unsigned
int exception, unsigned long error_code, unsigned long extra):

If exception is X86_TRAP_PF, then extra is CR2.  If exception is
X86_TRAP_MC, then extra is however much of the PA you know.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/