linux-kernel - Re: [PATCH] i386: per-CPU double fault TSS and stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 1 Sep 2007 12:33:12 +0200
From:	Andi Kleen <ak@...e.de>
To:	"Jan Beulich" <jbeulich@...ell.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i386: per-CPU double fault TSS and stack


Can you cc the next version to Linus please? He's probably best qualified
to review the i386 double fault handler because he wrote it originally.
I must admit the code always scared me a bit.

> +#ifdef CONFIG_HOTPLUG_CPU
> +static void *noinline __init_refok
> +#else
> +static inline void *__init
> +#endif

I really wonder if there isn't a cleaner way to do that :-( These init reference checks
are starting to become a major annoyance.

> +do_alloc_bootmem(unsigned long size, unsigned long align, unsigned long goal)
> +{
> +	return __alloc_bootmem(size, align, goal);
> +}
> +
>  /*
>   * cpu_init() initializes state that is per-CPU. Some data is already
>   * initialized (naturally) in the bootstrap process, such as the GDT
> @@ -659,6 +669,9 @@ void switch_to_new_gdt(void)
>  void __cpuinit cpu_init(void)
>  {
>  	int cpu = smp_processor_id();
> +#if N_EXCEPTION_TSS
> +	unsigned i;
> +#endif

Would it be that bad to have the TSS even around without CONFIG_DOUBLEFAULT?

In fact I would prefer to just eliminate CONFIG_DOUBLEFAULT (imho 
it always a bad idea because the amount of code it saves is miniscule) instead of 
adding such a ifdef maze.




> -#ifdef CONFIG_DOUBLEFAULT
> -	/* Set up doublefault TSS pointer in the GDT */
> -	__set_tss_desc(cpu, GDT_ENTRY_DOUBLEFAULT_TSS, &doublefault_tss);
> +#if N_EXCEPTION_TSS
> +#if EXCEPTION_STACK_ORDER > THREAD_ORDER
> +#error Assertion failed: EXCEPTION_STACK_ORDER <= THREAD_ORDER
> +#endif

BUILD_BUG_ON would look nicer


> +
> +		/* Set up exception handling stacks */
> +#ifdef CONFIG_SMP
> +		if (cpu) {

If you move the code after the gs pda setup you could use smp_processor_id() and
avoid the ifdefs (on UP it expands to 0 so the optimizer would do it cleanly)


> +				BUG_ON(page_count(page));
> +				init_page_count(page);
> +				free_pages(stack, j);
> +				stack += (PAGE_SIZE << j);

In 2.4-aa I added a alloc_pages_exact() for this. I don't think such games should
be played outside page_alloc.c. I would recommend to readd alloc_pages_exact()
and then use it.


> -#define DOUBLEFAULT_STACKSIZE (1024)
> -static unsigned long doublefault_stack[DOUBLEFAULT_STACKSIZE];
> -#define STACK_START (unsigned long)(doublefault_stack+DOUBLEFAULT_STACKSIZE)
> +extern unsigned long max_low_pfn;

No externs in .c

> +#define ptr_ok(x, l) ((x) >= PAGE_OFFSET \
> +                      && (x) + (l) <= PAGE_OFFSET + max_low_pfn * PAGE_SIZE - 1)
>  
> -#define ptr_ok(x) ((x) > PAGE_OFFSET && (x) < PAGE_OFFSET + MAXMEM)
> +#define THREAD_INFO_FROM(x) ((struct thread_info *)((x) & ~(THREAD_SIZE - 1)))
>  
> -static void doublefault_fn(void)
> +register const struct i386_hw_tss *self __asm__("ebx");

Can't you just move that to a proper argument register in assembler code?


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