Re: [PATCH 3/6] i386 virtualization - Make ldt a desc struct

[Chris Wright <chrisw@osdl.org>]

* zach@vmware.com (zach@vmware.com) wrote:
> Make the LDT a desc_struct pointer, since this is what it actually is.

I like that plan.

> There is code which relies on the fact that LDTs are allocated in page
> chunks, and it is both cleaner and more convenient to keep the rather
> poorly named "size" variable from the LDT in terms of LDT pages.

I noticed it's replaced by context.ldt and context.ldt_pages, which
appear to be decoupling the overloaded use from before.  Looks sane.
More comments below.

> Signed-off-by: Zachary Amsden <zach@vmware.com>
> Index: linux-2.6.13/include/asm-i386/mmu.h
> ===================================================================
> --- linux-2.6.13.orig/include/asm-i386/mmu.h	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/mmu.h	2005-08-15 11:19:49.000000000 -0700
> @@ -9,9 +9,9 @@
>   * cpu_vm_mask is used to optimize ldt flushing.
>   */
>  typedef struct { 
> -	int size;
>  	struct semaphore sem;
> -	void *ldt;
> +	struct desc_struct *ldt;
> +	int ldt_pages;
>  } mm_context_t;
>  
>  #endif
> Index: linux-2.6.13/include/asm-i386/desc.h
> ===================================================================
> --- linux-2.6.13.orig/include/asm-i386/desc.h	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/desc.h	2005-08-15 11:19:49.000000000 -0700
> @@ -6,6 +6,9 @@
>  
>  #define CPU_16BIT_STACK_SIZE 1024
>  
> +/* The number of LDT entries per page */
> +#define LDT_ENTRIES_PER_PAGE (PAGE_SIZE / LDT_ENTRY_SIZE)
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/preempt.h>
> @@ -30,7 +33,7 @@
>  static inline unsigned long get_desc_base(struct desc_struct *desc)
>  {
>  	unsigned long base;
> -	base = ((desc->a >> 16)  & 0x0000ffff) |
> +	base = (desc->a >> 16) |

Seemingly unrelated.

>  		((desc->b << 16) & 0x00ff0000) |
>  		(desc->b & 0xff000000);
>  	return base;
> @@ -171,7 +174,7 @@
>  static inline void load_LDT_nolock(mm_context_t *pc, int cpu)
>  {
>  	void *segments = pc->ldt;
> -	int count = pc->size;
> +	int count = pc->ldt_pages * LDT_ENTRIES_PER_PAGE;
>  
>  	if (likely(!count)) {
>  		segments = &default_ldt[0];
> Index: linux-2.6.13/include/asm-i386/mmu_context.h
> ===================================================================
> --- linux-2.6.13.orig/include/asm-i386/mmu_context.h	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/mmu_context.h	2005-08-15 11:19:49.000000000 -0700
> @@ -19,7 +19,7 @@
>  	memset(&mm->context, 0, sizeof(mm->context));
>  	init_MUTEX(&mm->context.sem);
>  	old_mm = current->mm;
> -	if (old_mm && unlikely(old_mm->context.size > 0)) {
> +	if (old_mm && unlikely(old_mm->context.ldt)) {
>  		retval = copy_ldt(&mm->context, &old_mm->context);
>  	}
>  	if (retval == 0)
> @@ -32,7 +32,7 @@
>   */
>  static inline void destroy_context(struct mm_struct *mm)
>  {
> -	if (unlikely(mm->context.size))
> +	if (unlikely(mm->context.ldt))
>  		destroy_ldt(mm);
>  	del_lazy_mm(mm);
>  }
> Index: linux-2.6.13/include/asm-i386/mach-default/mach_desc.h
> ===================================================================
> --- linux-2.6.13.orig/include/asm-i386/mach-default/mach_desc.h	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/include/asm-i386/mach-default/mach_desc.h	2005-08-15 11:19:49.000000000 -0700
> @@ -62,11 +62,10 @@
>  	_set_tssldt_desc(&get_cpu_gdt_table(cpu)[GDT_ENTRY_LDT], (int)addr, ((size << 3)-1), 0x82);
>  }
>  
> -static inline int write_ldt_entry(void *ldt, int entry, __u32 entry_a, __u32 entry_b)
> +static inline int write_ldt_entry(struct desc_struct *ldt, int entry, __u32 entry_a, __u32 entry_b)
>  {
> -	__u32 *lp = (__u32 *)((char *)ldt + entry*8);
> -	*lp = entry_a;
> -	*(lp+1) = entry_b;
> +	ldt[entry].a = entry_a;
> +	ldt[entry].b = entry_b;
>  	return 0;
>  }
>  
> Index: linux-2.6.13/arch/i386/kernel/ptrace.c
> ===================================================================
> --- linux-2.6.13.orig/arch/i386/kernel/ptrace.c	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/arch/i386/kernel/ptrace.c	2005-08-15 11:19:49.000000000 -0700
> @@ -164,18 +164,20 @@
>  	 * and APM bios ones we just ignore here.
>  	 */
>  	if (segment_from_ldt(seg)) {
> -		u32 *desc;
> +		mm_context_t *context;
> +		struct desc_struct *desc;
>  		unsigned long base;
>  
> -		down(&child->mm->context.sem);
> -		desc = child->mm->context.ldt + (seg & ~7);
> -		base = (desc[0] >> 16) | ((desc[1] & 0xff) << 16) | (desc[1] & 0xff000000);
> +		context = &child->mm->context;
> +		down(&context->sem);
> +		desc = &context->ldt[segment_index(seg)];
> +		base = get_desc_base(desc);
>  
>  		/* 16-bit code segment? */
> -		if (!((desc[1] >> 22) & 1))
> +		if (!get_desc_32bit(desc))
>  			addr &= 0xffff;
>  		addr += base;
> -		up(&child->mm->context.sem);
> +		up(&context->sem);
>  	}
>  	return addr;
>  }
> Index: linux-2.6.13/arch/i386/kernel/ldt.c
> ===================================================================
> --- linux-2.6.13.orig/arch/i386/kernel/ldt.c	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/arch/i386/kernel/ldt.c	2005-08-15 11:19:49.000000000 -0700
> @@ -28,28 +28,27 @@
>  }
>  #endif
>  
> -static inline int alloc_ldt(mm_context_t *pc, const int oldsize, int mincount, const int reload)
> +static inline int alloc_ldt(mm_context_t *pc, const int old_pages, int new_pages, const int reload)
>  {
> -	void *oldldt;
> -	void *newldt;
> +	struct desc_struct *oldldt;
> +	struct desc_struct *newldt;
>  

Not quite related here (since change was introduced in earlier patch),
but old alloc_ldt special cased when room was available.  This is gone,
so am I reading this correctly, each time through it will allocate a
new one, and free the old one (if it existed)?  Just double checking
that change doesn't introduce possible mem leak.

> -	mincount = (mincount+511)&(~511);
> -	if (mincount*LDT_ENTRY_SIZE > PAGE_SIZE)
> -		newldt = vmalloc(mincount*LDT_ENTRY_SIZE);
> +	if (new_pages > 1)
> +		newldt = vmalloc(new_pages*PAGE_SIZE);
>  	else
> -		newldt = kmalloc(mincount*LDT_ENTRY_SIZE, GFP_KERNEL);
> +		newldt = kmalloc(PAGE_SIZE, GFP_KERNEL);

If so, then full page is likely to be reusable in common case, no? (If
there's such a thing as LDT common case ;-)

>  	if (!newldt)
>  		return -ENOMEM;
>  
> -	if (oldsize)
> -		memcpy(newldt, pc->ldt, oldsize*LDT_ENTRY_SIZE);
> +	if (old_pages)
> +		memcpy(newldt, pc->ldt, old_pages*PAGE_SIZE);
>  	oldldt = pc->ldt;
>  	if (reload)
> -		memset(newldt+oldsize*LDT_ENTRY_SIZE, 0, (mincount-oldsize)*LDT_ENTRY_SIZE);
> +		memset(newldt+old_pages*LDT_ENTRIES_PER_PAGE, 0, (new_pages-old_pages)*PAGE_SIZE);

In fact, I _think_ this causes a problem.  Who says newldt is bigger
than old one?  This looks like user-triggerable oops to me.

>  	pc->ldt = newldt;
>  	wmb();
> -	pc->size = mincount;
> +	pc->ldt_pages = new_pages;
>  	wmb();
>  
>  	/*
> @@ -60,20 +59,20 @@
>  #ifdef CONFIG_SMP
>  		cpumask_t mask;
>  		preempt_disable();
> -		SetPagesLDT(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / PAGE_SIZE);
> +		SetPagesLDT(pc->ldt, pc->ldt_pages);
>  		load_LDT(pc);
>  		mask = cpumask_of_cpu(smp_processor_id());
>  		if (!cpus_equal(current->mm->cpu_vm_mask, mask))
>  			smp_call_function(flush_ldt, NULL, 1, 1);
>  		preempt_enable();
>  #else
> -		SetPagesLDT(pc->ldt, (pc->size * LDT_ENTRY_SIZE) / PAGE_SIZE);
> +		SetPagesLDT(pc->ldt, pc->ldt_pages);
>  		load_LDT(pc);
>  #endif
>  	}
> -	if (oldsize) {
> -		ClearPagesLDT(oldldt, (oldsize * LDT_ENTRY_SIZE) / PAGE_SIZE);
> -		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
> +	if (old_pages) {
> +		ClearPagesLDT(oldldt, old_pages);
> +		if (old_pages > 1)
>  			vfree(oldldt);
>  		else
>  			kfree(oldldt);
> @@ -86,10 +85,10 @@
>  	int err;
>  
>  	down(&old->sem);
> -	err = alloc_ldt(new, 0, old->size, 0);
> +	err = alloc_ldt(new, 0, old->ldt_pages, 0);
>  	if (!err) {
> -		memcpy(new->ldt, old->ldt, old->size*LDT_ENTRY_SIZE);
> -		SetPagesLDT(new->ldt, (new->size * LDT_ENTRY_SIZE) / PAGE_SIZE);
> +		memcpy(new->ldt, old->ldt, old->ldt_pages*PAGE_SIZE);
> +		SetPagesLDT(new->ldt, new->ldt_pages);
>  	}
>  	up(&old->sem);
>  	return err;
> @@ -97,14 +96,16 @@
>  
>  void destroy_ldt(struct mm_struct *mm)
>  {
> +	int pages = mm->context.ldt_pages;
> +
>  	if (mm == current->active_mm)
>  		clear_LDT();
> -	ClearPagesLDT(mm->context.ldt, (mm->context.size * LDT_ENTRY_SIZE) / PAGE_SIZE);
> -	if (mm->context.size*LDT_ENTRY_SIZE > PAGE_SIZE)
> +	ClearPagesLDT(mm->context.ldt, pages);
> +	if (pages > 1)
>  		vfree(mm->context.ldt);
>  	else
>  		kfree(mm->context.ldt);
> -	mm->context.size = 0;
> +	mm->context.ldt_pages = 0;
>  }
>  
>  static int read_ldt(void __user * ptr, unsigned long bytecount)
> @@ -113,13 +114,13 @@
>  	unsigned long size;
>  	struct mm_struct * mm = current->mm;
>  
> -	if (!mm->context.size)
> +	if (!mm->context.ldt_pages)
>  		return 0;
>  	if (bytecount > LDT_ENTRY_SIZE*LDT_ENTRIES)
>  		bytecount = LDT_ENTRY_SIZE*LDT_ENTRIES;
>  
>  	down(&mm->context.sem);
> -	size = mm->context.size*LDT_ENTRY_SIZE;
> +	size = mm->context.ldt_pages*PAGE_SIZE;
>  	if (size > bytecount)
>  		size = bytecount;

This now looks like you can leak data?  Since full page is unlikely
used, but accounting is done in page sizes.  Asking to read_ldt with
bytcount of PAGE_SIZE could give some uninitialzed data back to user.
Did I miss the spot where this is always zero-filled?

> @@ -166,6 +167,7 @@
>  	__u32 entry_1, entry_2;
>  	int error;
>  	struct user_desc ldt_info;
> +	int page_number;
>  
>  	error = -EINVAL;
>  	if (bytecount != sizeof(ldt_info))
> @@ -184,10 +186,11 @@
>  			goto out;
>  	}
>  
> +	page_number = ldt_info.entry_number / LDT_ENTRIES_PER_PAGE;
>  	down(&mm->context.sem);
> -	if (ldt_info.entry_number >= mm->context.size) {
> -		error = alloc_ldt(&current->mm->context, mm->context.size,
> -					ldt_info.entry_number+1, 1);
> +	if (page_number >= mm->context.ldt_pages) {
> +		error = alloc_ldt(&current->mm->context, mm->context.ldt_pages,
> +					page_number+1, 1);
>  		if (error < 0)
>  			goto out_unlock;
>  	}
> Index: linux-2.6.13/arch/i386/kernel/kprobes.c
> ===================================================================
> --- linux-2.6.13.orig/arch/i386/kernel/kprobes.c	2005-08-15 11:16:59.000000000 -0700
> +++ linux-2.6.13/arch/i386/kernel/kprobes.c	2005-08-15 11:19:49.000000000 -0700
> @@ -164,8 +164,7 @@
>  	 */
>  	if (segment_from_ldt(regs->xcs) && (current->mm)) {
>  		struct desc_struct *desc;
> -		desc = (struct desc_struct *) ((char *) current->mm->context.ldt +
> -						 (segment_index(regs->xcs) * 8));
> +		desc = &current->mm->context.ldt[segment_index(regs->xcs)];
>  		addr = (kprobe_opcode_t *) (get_desc_base(desc) + regs->eip -
>  						sizeof(kprobe_opcode_t));
>  	} else {
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/