Re: [PATCH 18/19] move _set_gate and its users to a common location

Ingo Molnar
2007-12-22T02:13:43Z

* Glauber de Oliveira Costa <gcosta@redhat.com> wrote:

> This patch moves _set_gate and its users to desc.h. We can now use
> common code for x86_64 and i386.

a few days ago i started seeing weird crashes on 64-bit x86 in the
random-kernel-bootup tests. Nothing truly reproducable to be bisectable,
but quality of x86.git went down drastically. Double faults, triple
faults, crashes all around the place, with every few dozen kernel
bootups.

Today i found a config and a workload that triggered the crashes a bit
more reliably. I still needed a 6 hours bisection marathon to pinpoint
the patch - the one in this thread. A review of it with H. Peter Anvin
pinpointed the breakage:

> +static inline void set_system_gate(unsigned int n, void *addr)
> +{
> + BUG_ON((unsigned)n > 0xFF);
> + _set_gate(n, GATE_TRAP, addr, 0x3, 0, __KERNEL_CS);
> +}

> -static inline void set_system_gate(int nr, void *func)
> -{
> - BUG_ON((unsigned)nr > 0xFF);
> - _set_gate(nr, GATE_INTERRUPT, (unsigned long) func, 3, 0);
> -}

you changed the type of system gates on 64-bit from GATE_INTERRUPT to
GATE_TRAP. The effect of this is that these gates enter with interrupts
enabled - instead of interrupts disabled. This, amongst others, affects
the following key gate:

set_system_gate(IA32_SYSCALL_VECTOR, ia32_syscall);

which relies on disabled interrupts to fix up its stack. If an interrupt
comes in the wrong moment then we get a kernel stack corruption that is
not survivable.

the reason for this bug was that you tried to do too many changes in a
single patch. You did a cleanup, you did unification and you moved code
around. It was totally non-obvious what you did and the resulting patch
was not reviewable at all - even after the bisection poinpointed the
patch, it took us almost 30 minutes to figure out where the bug was. If
this unstructured, careless mess of patches continues then we are not
going to be able to accept any more 64-bit paravirt patches into
x86.git.

Ingo