Re: [PATCH] x86: use enum instead of literals for trap values

[Kees Cook]

On Fri, Mar 9, 2012 at 1:28 AM, Ingo Molnar <mingo@xxxxxxx> wrote:
>
> * Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> The traps are referred to by their numbers and it can be difficult to
>> understand them while reading the code without context. This patch adds
>> enumeration of the trap numbers and replaces the numbers with the correct
>> enum for x86.
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>>
>> ---
>> I've updated Aditya Kali's earlier patch:
>> https://lkml.org/lkml/2011/4/22/328
>> ---
>>  arch/x86/include/asm/traps.h |   25 +++++++++
>>  arch/x86/kernel/irqinit.c    |    2 +-
>>  arch/x86/kernel/traps.c      |  117 ++++++++++++++++++++++--------------------
>>  3 files changed, 88 insertions(+), 56 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/traps.h b/arch/x86/include/asm/traps.h
>> index 0012d09..768afb2 100644
>> --- a/arch/x86/include/asm/traps.h
>> +++ b/arch/x86/include/asm/traps.h
>> @@ -89,4 +89,29 @@ asmlinkage void smp_thermal_interrupt(void);
>>  asmlinkage void mce_threshold_interrupt(void);
>>  #endif
>>
>> +/* Interrupts/Exceptions */
>> +enum {
>> +     INTR_DIV_BY_ZERO = 0,   /*  0 */
>> +     INTR_DEBUG,             /*  1 */
>> +     INTR_NMI,               /*  2 */
>> +     INTR_BREAKPOINT,        /*  3 */
>> +     INTR_OVERFLOW,          /*  4 */
>> +     INTR_BOUNDS_CHECK,      /*  5 */
>> +     INTR_INVALID_OP,        /*  6 */
>> +     INTR_NO_DEV,            /*  7 */
>> +     INTR_DBL_FAULT,         /*  8 */
>> +     INTR_SEG_OVERRUN,       /*  9 */
>> +     INTR_INVALID_TSS,       /* 10 */
>> +     INTR_NO_SEG,            /* 11 */
>> +     INTR_STACK_FAULT,       /* 12 */
>> +     INTR_GPF,               /* 13 */
>> +     INTR_PAGE_FAULT,        /* 14 */
>> +     INTR_SPURIOUS,          /* 15 */
>> +     INTR_COPROCESSOR,       /* 16 */
>> +     INTR_ALIGNMENT,         /* 17 */
>> +     INTR_MCE,               /* 18 */
>> +     INTR_SIMD_COPROCESSOR,  /* 19 */
>> +     INTR_IRET = 32,         /* 32 */
>> +};
>
>> @@ -453,14 +458,15 @@ dotraplinkage void __kprobes do_debug(struct pt_regs *regs, long error_code)
>>  /*
>>   * Note that we play around with the 'TS' bit in an attempt to get
>>   * the correct behaviour even in the presence of the asynchronous
>> - * IRQ13 behaviour
>> + * INTR_GPF behaviour
>>   */
>
>> @@ -529,8 +535,9 @@ void math_error(struct pt_regs *regs, int error_code, int trapnr)
>>               info.si_code = FPE_FLTRES;
>>       } else {
>>               /*
>> -              * If we're using IRQ 13, or supposedly even some trap 16
>> -              * implementations, it's possible we get a spurious trap...
>> +              * If we're using INTR_GPF, or supposedly even some trap
>> +              * INTR_COPROCESSOR implementations, it's possible we get a
>> +              * spurious trap...
>
> There's confusion in this patch between legacy IRQ #13 [vector
> 0x20 + 13 ] and #GPF general protection fault [vector 13] - they
> are not the same.
>
> Furthermore, the INTR_ naming is not ideal either for (most of)
> these entries: for example we don't think of a page fault as an
> asynchronous interrupt entity - we think of it as a more or less
> synchronous fault/exception.
>
> Thus a X86_*_FAULT_VEC naming pattern might be better:
>
>        X86_PAGE_FAULT_VEC
>        X86_DOUBLE_FAULT_VEC
>
> (With X86_*_EXCEPTION_VEC applied where appropriate.)

Oh, hrm, my v2 missed this bit about EXCEPTION. What should I use as
the canonical source for "FAULT" vs "EXCEPTION" for this enum?

> I don't disagree with the general principle of the cleanup
> otherwise, the numeric literals are often ambiguous and
> confusing - as the trap 13 - irq 13 mixup above shows.

Right, and leaves me a bit confused too. :)

-Kees

-- 
Kees Cook
ChromeOS Security
--
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/