[PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jan Beulich]

Instead of suppressing the change of .text to become readonly, make
the SMP locks patching code properly adjust/restore the page access
rights.

On x86-64 additionally remove all mappings past the kernel image, and
remove leftovers from the removal of the more general (but abandoned)
SMP alternatives.

Note that while I expect this to work properly on it own, it was only
tested together with the change_page_attr() patch submitted before,
which namely fixed the reference counting on split pages (which
affected earlier versions of this patch).

Assumes the respective KPROBES adjustment is also in place
(CONFIG_KPROBES being removed here at once).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

 arch/i386/kernel/alternative.c   |   67 +++++++++++++++++++++++++++++++++------
 arch/i386/mm/init.c              |   14 +-------
 arch/x86_64/kernel/vmlinux.lds.S |    9 -----
 arch/x86_64/mm/init.c            |   14 ++------
 4 files changed, 64 insertions(+), 40 deletions(-)

--- linux-2.6.22-rc7/arch/i386/kernel/alternative.c	2007-07-03 10:57:28.000000000 +0200
+++ 2.6.22-rc7-x86-alt-page-attr/arch/i386/kernel/alternative.c	2007-07-02 14:40:15.000000000 +0200
@@ -2,8 +2,11 @@
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
+#include <linux/pfn.h>
 #include <asm/alternative.h>
 #include <asm/sections.h>
+#include <asm/pgtable.h>
+#include <asm/cacheflush.h>
 
 static int noreplace_smp     = 0;
 static int smp_alt_once      = 0;
@@ -150,6 +153,46 @@ static void nop_out(void *insns, unsigne
 	}
 }
 
+#ifdef CONFIG_DEBUG_RODATA
+
+#ifdef CONFIG_X86_32
+#include <asm/highmem.h>
+#define MODULES_VADDR VMALLOC_START
+#endif
+
+static inline void make_writable(const void *instr, unsigned int len)
+{
+	unsigned long va = (unsigned long)instr;
+
+	if (va < MODULES_VADDR) {
+		change_page_attr(virt_to_page(instr),
+				 PFN_UP(va + len) - PFN_DOWN(va),
+				 PAGE_KERNEL_EXEC);
+		global_flush_tlb();
+	}
+}
+
+static inline void make_readonly(const void *instr, unsigned int len)
+{
+	unsigned long va = (unsigned long)instr;
+
+	if (va < MODULES_VADDR) {
+		change_page_attr(virt_to_page(instr),
+				 PFN_UP(va + len) - PFN_DOWN(va),
+#ifdef CONFIG_X86_64
+				 PAGE_KERNEL_RO);
+#else
+				 PAGE_KERNEL_RX);
+#endif
+		global_flush_tlb();
+	}
+}
+
+#else /* !CONFIG_DEBUG_RODATA */
+#define make_writable (void)
+#define make_readonly (void)
+#endif
+
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
 extern u8 *__smp_locks[], *__smp_locks_end[];
 
@@ -196,7 +239,9 @@ static void alternatives_smp_lock(u8 **s
 			continue;
 		if (*ptr > text_end)
 			continue;
+		make_writable(*ptr, 1);
 		**ptr = 0xf0; /* lock prefix */
+		make_readonly(*ptr, 1);
 	};
 }
 
@@ -212,7 +257,9 @@ static void alternatives_smp_unlock(u8 *
 			continue;
 		if (*ptr > text_end)
 			continue;
+		make_writable(*ptr, 1);
 		nop_out(*ptr, 1);
+		make_readonly(*ptr, 1);
 	};
 }
 
@@ -239,7 +286,6 @@ void alternatives_smp_module_add(struct 
 				 void *text,  void *text_end)
 {
 	struct smp_alt_module *smp;
-	unsigned long flags;
 
 	if (noreplace_smp)
 		return;
@@ -265,39 +311,37 @@ void alternatives_smp_module_add(struct 
 		__FUNCTION__, smp->locks, smp->locks_end,
 		smp->text, smp->text_end, smp->name);
 
-	spin_lock_irqsave(&smp_alt, flags);
+	spin_lock(&smp_alt);
 	list_add_tail(&smp->next, &smp_alt_modules);
 	if (boot_cpu_has(X86_FEATURE_UP))
 		alternatives_smp_unlock(smp->locks, smp->locks_end,
 					smp->text, smp->text_end);
-	spin_unlock_irqrestore(&smp_alt, flags);
+	spin_unlock(&smp_alt);
 }
 
 void alternatives_smp_module_del(struct module *mod)
 {
 	struct smp_alt_module *item;
-	unsigned long flags;
 
 	if (smp_alt_once || noreplace_smp)
 		return;
 
-	spin_lock_irqsave(&smp_alt, flags);
+	spin_lock(&smp_alt);
 	list_for_each_entry(item, &smp_alt_modules, next) {
 		if (mod != item->mod)
 			continue;
 		list_del(&item->next);
-		spin_unlock_irqrestore(&smp_alt, flags);
+		spin_unlock(&smp_alt);
 		DPRINTK("%s: %s\n", __FUNCTION__, item->name);
 		kfree(item);
 		return;
 	}
-	spin_unlock_irqrestore(&smp_alt, flags);
+	spin_unlock(&smp_alt);
 }
 
 void alternatives_smp_switch(int smp)
 {
 	struct smp_alt_module *mod;
-	unsigned long flags;
 
 #ifdef CONFIG_LOCKDEP
 	/*
@@ -313,7 +357,7 @@ void alternatives_smp_switch(int smp)
 		return;
 	BUG_ON(!smp && (num_online_cpus() > 1));
 
-	spin_lock_irqsave(&smp_alt, flags);
+	spin_lock(&smp_alt);
 	if (smp) {
 		printk(KERN_INFO "SMP alternatives: switching to SMP code\n");
 		clear_bit(X86_FEATURE_UP, boot_cpu_data.x86_capability);
@@ -329,7 +373,7 @@ void alternatives_smp_switch(int smp)
 			alternatives_smp_unlock(mod->locks, mod->locks_end,
 						mod->text, mod->text_end);
 	}
-	spin_unlock_irqrestore(&smp_alt, flags);
+	spin_unlock(&smp_alt);
 }
 
 #endif
@@ -369,6 +413,7 @@ void __init alternative_instructions(voi
 
 	local_irq_save(flags);
 	apply_alternatives(__alt_instructions, __alt_instructions_end);
+	local_irq_restore(flags);
 
 	/* switch to patch-once-at-boottime-only mode and free the
 	 * tables in case we know the number of CPUs will never ever
@@ -399,6 +444,8 @@ void __init alternative_instructions(voi
 		alternatives_smp_switch(0);
 	}
 #endif
+
+	local_irq_save(flags);
  	apply_paravirt(__parainstructions, __parainstructions_end);
 	local_irq_restore(flags);
 }
--- linux-2.6.22-rc7/arch/i386/mm/init.c	2007-07-03 10:57:29.000000000 +0200
+++ 2.6.22-rc7-x86-alt-page-attr/arch/i386/mm/init.c	2007-07-02 14:40:15.000000000 +0200
@@ -799,17 +799,9 @@ void mark_rodata_ro(void)
 	unsigned long start = PFN_ALIGN(_text);
 	unsigned long size = PFN_ALIGN(_etext) - start;
 
-#ifndef CONFIG_KPROBES
-#ifdef CONFIG_HOTPLUG_CPU
-	/* It must still be possible to apply SMP alternatives. */
-	if (num_possible_cpus() <= 1)
-#endif
-	{
-		change_page_attr(virt_to_page(start),
-		                 size >> PAGE_SHIFT, PAGE_KERNEL_RX);
-		printk("Write protecting the kernel text: %luk\n", size >> 10);
-	}
-#endif
+	change_page_attr(virt_to_page(start),
+	                 size >> PAGE_SHIFT, PAGE_KERNEL_RX);
+	printk("Write protecting the kernel text: %luk\n", size >> 10);
 	start += size;
 	size = (unsigned long)__end_rodata - start;
 	change_page_attr(virt_to_page(start),
--- linux-2.6.22-rc7/arch/x86_64/kernel/vmlinux.lds.S	2007-07-03 10:57:39.000000000 +0200
+++ 2.6.22-rc7-x86-alt-page-attr/arch/x86_64/kernel/vmlinux.lds.S	2007-07-02 14:40:15.000000000 +0200
@@ -131,20 +131,11 @@ SECTIONS
   /* might get freed after init */
   . = ALIGN(4096);
   __smp_alt_begin = .;
-  __smp_alt_instructions = .;
-  .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
-	*(.smp_altinstructions)
-  }
-  __smp_alt_instructions_end = .;
-  . = ALIGN(8);
   __smp_locks = .;
   .smp_locks : AT(ADDR(.smp_locks) - LOAD_OFFSET) {
 	*(.smp_locks)
   }
   __smp_locks_end = .;
-  .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
-	*(.smp_altinstr_replacement)
-  }
   . = ALIGN(4096);
   __smp_alt_end = .;
 
--- linux-2.6.22-rc7/arch/x86_64/mm/init.c	2007-07-03 10:57:39.000000000 +0200
+++ 2.6.22-rc7-x86-alt-page-attr/arch/x86_64/mm/init.c	2007-07-02 14:40:15.000000000 +0200
@@ -592,6 +592,10 @@ void free_initmem(void)
 	free_init_pages("unused kernel memory",
 			(unsigned long)(&__init_begin),
 			(unsigned long)(&__init_end));
+	change_page_attr_addr(PFN_ALIGN(_end),
+			      (KERNEL_TEXT_SIZE - __pa_symbol(_end)) >> PAGE_SHIFT,
+			      __pgprot(0));
+	global_flush_tlb();
 }
 
 #ifdef CONFIG_DEBUG_RODATA
@@ -600,16 +604,6 @@ void mark_rodata_ro(void)
 {
 	unsigned long start = (unsigned long)_stext, end;
 
-#ifdef CONFIG_HOTPLUG_CPU
-	/* It must still be possible to apply SMP alternatives. */
-	if (num_possible_cpus() > 1)
-		start = (unsigned long)_etext;
-#endif
-
-#ifdef CONFIG_KPROBES
-	start = (unsigned long)__start_rodata;
-#endif
-	
 	end = (unsigned long)__end_rodata;
 	start = (start + PAGE_SIZE - 1) & PAGE_MASK;
 	end &= PAGE_MASK;

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> Instead of suppressing the change of .text to become readonly, make
> the SMP locks patching code properly adjust/restore the page access
> rights.
>   

I guess we'll need to do the same thing for paravirt_ops patching?

> On x86-64 additionally remove all mappings past the kernel image, and
> remove leftovers from the removal of the more general (but abandoned)
> SMP alternatives.
>   

I'd already posted a patch to remove smp alternatives.  I thought it had 
been merged already, but I guess it's in Andi's queue.

    J

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Andi Kleen]

On Wednesday 04 July 2007 17:41:39 Jeremy Fitzhardinge wrote:
> Jan Beulich wrote:
> > Instead of suppressing the change of .text to become readonly, make
> > the SMP locks patching code properly adjust/restore the page access
> > rights.
> >   
> 
> I guess we'll need to do the same thing for paravirt_ops patching?

Not if the patching happens early enough at boot. That's true for Xen
and lguest, but I'm not sure about VMI

-Andi

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jan Beulich]

>>> Jeremy Fitzhardinge <jeremy@goop.org> 04.07.07 17:41 >>>
>Jan Beulich wrote:
>> Instead of suppressing the change of .text to become readonly, make
>> the SMP locks patching code properly adjust/restore the page access
>> rights.
>>   
>
>I guess we'll need to do the same thing for paravirt_ops patching?

No, the patch already does this (as long as there no code to do that other
than in alternatives.c).

>> On x86-64 additionally remove all mappings past the kernel image, and
>> remove leftovers from the removal of the more general (but abandoned)
>> SMP alternatives.
>>   
>
>I'd already posted a patch to remove smp alternatives.  I thought it had 
>been merged already, but I guess it's in Andi's queue.

Oh, sorry, I forgot to remove that part of the comment after merging up to
2.6..22-rcX.

Jan

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jan Beulich]

>>> On x86-64 additionally remove all mappings past the kernel image, and
>>> remove leftovers from the removal of the more general (but abandoned)
>>> SMP alternatives.
>>>   
>>
>>I'd already posted a patch to remove smp alternatives.  I thought it had 
>>been merged already, but I guess it's in Andi's queue.
>
>Oh, sorry, I forgot to remove that part of the comment after merging up to
>2.6..22-rcX.

Actually, no, the comment is right - it talks about pieces left in x86-64's
vmlinux.lds.S.

Jan

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> Not if the patching happens early enough at boot. That's true for Xen
> and lguest, but I'm not sure about VMI

paravirt_ops patching happens at the same time as all other patching, 
via check_bugs().  So its the same regardless of backend, and quite late.

    J

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jeremy Fitzhardinge]

Jan Beulich wrote:
>> Oh, sorry, I forgot to remove that part of the comment after merging up to
>> 2.6..22-rcX.
>>     
>
> Actually, no, the comment is right - it talks about pieces left in x86-64's
> vmlinux.lds.S.

Hm.  I was pretty sure I did that too, but maybe it was in a separate 
patch which got lost.

    J

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Dave Jones]

On Wed, Jul 04, 2007 at 08:41:39AM -0700, Jeremy Fitzhardinge wrote:
 
 > I'd already posted a patch to remove smp alternatives.  I thought it had 
 > been merged already, but I guess it's in Andi's queue.

What was the rationale behind that?  Given many distros have now
moved to shipping just an SMP kernel, this seems like a step backwards?

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jeremy Fitzhardinge]

Dave Jones wrote:
> On Wed, Jul 04, 2007 at 08:41:39AM -0700, Jeremy Fitzhardinge wrote:
>  
>  > I'd already posted a patch to remove smp alternatives.  I thought it had 
>  > been merged already, but I guess it's in Andi's queue.
>
> What was the rationale behind that?  Given many distros have now
> moved to shipping just an SMP kernel, this seems like a step backwards?
>   

No, there's two mechanisms.  There's the clobber the lock prefix thing, 
which is unaffected.  And there's smp_alternatives, which was a more 
ambitious mechanism to completely replace instructions in the SMP vs 
non-SMP cases, which is completely unused and therefore removed.

    J

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Andi Kleen]

On Wednesday 04 July 2007 08:33:24 Jan Beulich wrote:

Sorry for late feedback.


> +#ifdef CONFIG_DEBUG_RODATA
> +
> +#ifdef CONFIG_X86_32
> +#include <asm/highmem.h>
> +#define MODULES_VADDR VMALLOC_START
> +#endif
> +
> +static inline void make_writable(const void *instr, unsigned int len)
> +{
> +	unsigned long va = (unsigned long)instr;
> +
> +	if (va < MODULES_VADDR) {
> +		change_page_attr(virt_to_page(instr),
> +				 PFN_UP(va + len) - PFN_DOWN(va),
> +				 PAGE_KERNEL_EXEC);
> +		global_flush_tlb();
> +	}
> +}
> +
> +static inline void make_readonly(const void *instr, unsigned int len)
> +{
> +	unsigned long va = (unsigned long)instr;
> +
> +	if (va < MODULES_VADDR) {
> +		change_page_attr(virt_to_page(instr),
> +				 PFN_UP(va + len) - PFN_DOWN(va),
> +#ifdef CONFIG_X86_64
> +				 PAGE_KERNEL_RO);
> +#else
> +				 PAGE_KERNEL_RX);
> +#endif
> +		global_flush_tlb();
> +	}
> +}

Can you please move those into the respective pageattr.cs without ifdefs? 
Also the va < MODULES_VADDR test seems overly magic; while it may work right now
it seems fragile. It would be better to test the exact ranges in the respective
architectures

>  void alternatives_smp_module_del(struct module *mod)
>  {
>  	struct smp_alt_module *item;
> -	unsigned long flags;
>  
>  	if (smp_alt_once || noreplace_smp)
>  		return;
>  
> -	spin_lock_irqsave(&smp_alt, flags);
> +	spin_lock(&smp_alt);

Unrelated change?  Why? Should probably be separate patch.

> --- linux-2.6.22-rc7/arch/x86_64/kernel/vmlinux.lds.S	2007-07-03 10:57:39.000000000 +0200
> +++ 2.6.22-rc7-x86-alt-page-attr/arch/x86_64/kernel/vmlinux.lds.S	2007-07-02 14:40:15.000000000 +0200
> @@ -131,20 +131,11 @@ SECTIONS
>    /* might get freed after init */
>    . = ALIGN(4096);
>    __smp_alt_begin = .;
> -  __smp_alt_instructions = .;
> -  .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
> -	*(.smp_altinstructions)
> -  }
> -  __smp_alt_instructions_end = .;
> -  . = ALIGN(8);
>    __smp_locks = .;
>    .smp_locks : AT(ADDR(.smp_locks) - LOAD_OFFSET) {
>  	*(.smp_locks)
>    }
>    __smp_locks_end = .;
> -  .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
> -	*(.smp_altinstr_replacement)
> -  }

While ok too it should be also separate please.

-Andi

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jeremy Fitzhardinge]

Jan Beulich wrote:
> +	if (va < MODULES_VADDR) {
> +		change_page_attr(virt_to_page(instr),
> +				 PFN_UP(va + len) - PFN_DOWN(va),
> +#ifdef CONFIG_X86_64
> +				 PAGE_KERNEL_RO);
> +#else
> +				 PAGE_KERNEL_RX);
> +#endif
>   

This kind if unbalanced parenthesis can confuse various tools.  Format
it differently?

    J

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Mathieu Desnoyers]

Hi Andi,

I guess the patch I proposed there:
"Text Edit Lock"
http://www.ussg.iu.edu/hypermail/linux/kernel/0707.1/2980.html

solves the problem more generally than the patch discussed in this
thread. It provides locking (architecture independent) and correct
page protection (architecture dependent) for paravirt, alternatives
kprobes and, eventually, immediate values.

Mathieu

* Andi Kleen (ak@suse.de) wrote:
> On Wednesday 04 July 2007 08:33:24 Jan Beulich wrote:
> 
> Sorry for late feedback.
> 
> 
> > +#ifdef CONFIG_DEBUG_RODATA
> > +
> > +#ifdef CONFIG_X86_32
> > +#include <asm/highmem.h>
> > +#define MODULES_VADDR VMALLOC_START
> > +#endif
> > +
> > +static inline void make_writable(const void *instr, unsigned int len)
> > +{
> > +	unsigned long va = (unsigned long)instr;
> > +
> > +	if (va < MODULES_VADDR) {
> > +		change_page_attr(virt_to_page(instr),
> > +				 PFN_UP(va + len) - PFN_DOWN(va),
> > +				 PAGE_KERNEL_EXEC);
> > +		global_flush_tlb();
> > +	}
> > +}
> > +
> > +static inline void make_readonly(const void *instr, unsigned int len)
> > +{
> > +	unsigned long va = (unsigned long)instr;
> > +
> > +	if (va < MODULES_VADDR) {
> > +		change_page_attr(virt_to_page(instr),
> > +				 PFN_UP(va + len) - PFN_DOWN(va),
> > +#ifdef CONFIG_X86_64
> > +				 PAGE_KERNEL_RO);
> > +#else
> > +				 PAGE_KERNEL_RX);
> > +#endif
> > +		global_flush_tlb();
> > +	}
> > +}
> 
> Can you please move those into the respective pageattr.cs without ifdefs? 
> Also the va < MODULES_VADDR test seems overly magic; while it may work right now
> it seems fragile. It would be better to test the exact ranges in the respective
> architectures
> 
> >  void alternatives_smp_module_del(struct module *mod)
> >  {
> >  	struct smp_alt_module *item;
> > -	unsigned long flags;
> >  
> >  	if (smp_alt_once || noreplace_smp)
> >  		return;
> >  
> > -	spin_lock_irqsave(&smp_alt, flags);
> > +	spin_lock(&smp_alt);
> 
> Unrelated change?  Why? Should probably be separate patch.
> 
> > --- linux-2.6.22-rc7/arch/x86_64/kernel/vmlinux.lds.S	2007-07-03 10:57:39.000000000 +0200
> > +++ 2.6.22-rc7-x86-alt-page-attr/arch/x86_64/kernel/vmlinux.lds.S	2007-07-02 14:40:15.000000000 +0200
> > @@ -131,20 +131,11 @@ SECTIONS
> >    /* might get freed after init */
> >    . = ALIGN(4096);
> >    __smp_alt_begin = .;
> > -  __smp_alt_instructions = .;
> > -  .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
> > -	*(.smp_altinstructions)
> > -  }
> > -  __smp_alt_instructions_end = .;
> > -  . = ALIGN(8);
> >    __smp_locks = .;
> >    .smp_locks : AT(ADDR(.smp_locks) - LOAD_OFFSET) {
> >  	*(.smp_locks)
> >    }
> >    __smp_locks_end = .;
> > -  .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
> > -	*(.smp_altinstr_replacement)
> > -  }
> 
> While ok too it should be also separate please.
> 
> -Andi
> -
> 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/
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

[Jan Beulich]

>>  void alternatives_smp_module_del(struct module *mod)
>>  {
>>  	struct smp_alt_module *item;
>> -	unsigned long flags;
>>  
>>  	if (smp_alt_once || noreplace_smp)
>>  		return;
>>  
>> -	spin_lock_irqsave(&smp_alt, flags);
>> +	spin_lock(&smp_alt);
>
>Unrelated change?  Why? Should probably be separate patch.

Absolutely not: You can't call change_page_attr(), or more precisely
global_flush_tlb(), with interrupts disabled. And since interrupt disabling
is unneeded here afaics, I simply changed the calls instead of inventing
some ugly workaround.

I'll do the other adjustments you asked for, but it'll be only in about three
weeks from now...

Jan