Re: [patch 05/24] Text Edit Lock - Architecture Independent Code

[Mathieu Desnoyers]

* zhangxiliang (zhangxiliang@xxxxxxxxxxxxxx) wrote:
> hello,
>        I have some questions for your patches.
> 

Hi,

> > Paravirt and alternatives are always done when SMP is
> > inactive, so there is no
> > need to use locks.
> 
> > -#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(KERN_INFO "Write protecting the kernel text: %luk\n",
> > +		size >> 10);
> > +
> 
> Why "mark_rodata_ro" doesn't consider smp instance? Maybe it will be appied in 
> future.
> 

In its previous state, mark_rodata_ro was disabled in these situations :
- System supports CPU_HOTPLUG (alternatives will need to be applied when
  we pass from 1->2 and 2->1 cpu.
- System supports KPROBES, it need to put breakpoints in the code.

The main effect of the change I introduce is that I allow the kernel
code to be marked RO even in these situations. Alternatives and kprobes
uses the text_poke to modify kernel code, which temporarily disabled the
Write Protection bit on the local CPU so the memory write to RO pages
can be done.

So I guess the answer to your question is : previously, mark_rodata_ro
did not support CPU HOTPLUG nor KPROBES, and now it does, which is
much cleaner.


> 
> > ===================================================================
> > --- linux-2.6-lttng.orig/kernel/kprobes.c	2007-12-12
> > 18:10:32.000000000 -0500
> > +++ linux-2.6-lttng/kernel/kprobes.c	2007-12-12
> > 18:10:34.000000000 -0500
> > @@ -644,7 +644,9 @@ valid_p:
> >  			list_del_rcu(&p->list);
> >  			kfree(old_p);
> >  		}
> > +		mutex_lock(&kprobe_mutex);
> >  		arch_remove_kprobe(p);
> > +		mutex_unlock(&kprobe_mutex);
> >  	} else {
> >  		mutex_lock(&kprobe_mutex);
> >  		if (p->break_handler)
> 
> I think "mutex_lock" and "mutex_unlock" shoud be in architecture code.
> In "__register_kprobe" funtion, its implement "arch_prepare_kprobe" and 
> "arch_arm_kprobe" is also depended on arch.  So the remove implement is not 
> the same on the different architecture code.
> 
> Maybe it doesn't need the mutex_lock in "arch_remove_kprobe" on some embeded 
> system chips if linux can support the other embeded system chips in future.
> 

Which patch is this coming from ?

My Text Edit Lock - kprobes architecture independent support
patch _removes_ the kprobe_mutex taken around arch_remove_kprobes
because it is useless, I don't see how this patch snippet applies to my
patchset at all.

If you suggest to change the way locking is currently done in
kprobes, please do this in a separate thread, as a RFC ?

Mathieu

> 
> > -----Original Message-----
> > From: linux-kernel-owner@xxxxxxxxxxxxxxx
> > [mailto:linux-kernel-owner@xxxxxxxxxxxxxxx] On Behalf Of
> > Mathieu Desnoyers
> > Sent: Friday, December 21, 2007 9:55 AM
> > To: akpm@xxxxxxxxxxxxxxxxxxxx; Ingo Molnar;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Mathieu Desnoyers; Andi Kleen
> > Subject: [patch 05/24] Text Edit Lock - Architecture Independent Code
> >
> > This is an architecture independant synchronization around kernel text
> > modifications through use of a global mutex.
> >
> > A mutex has been chosen so that kprobes, the main user of
> > this, can sleep during
> > memory allocation between the memory read of the instructions
> > it must replace
> > and the memory write of the breakpoint.
> >
> > Other user of this interface: immediate values.
> >
> > Paravirt and alternatives are always done when SMP is
> > inactive, so there is no
> > need to use locks.
> >
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxx>
> > CC: Andi Kleen <andi@xxxxxxxxxxxxxx>
> > ---
> >  include/linux/memory.h |    7 +++++++
> >  mm/memory.c            |   34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> >
> > Index: linux-2.6-lttng/include/linux/memory.h
> > ===================================================================
> > --- linux-2.6-lttng.orig/include/linux/memory.h> 
> > 2007-11-07 11:11:26.000000000 -0500
> > +++ linux-2.6-lttng/include/linux/memory.h	2007-11-07
> > 11:13:48.000000000 -0500
> > @@ -93,4 +93,11 @@ extern int memory_notify(unsigned long v
> >  #define hotplug_memory_notifier(fn, pri) do { } while (0)
> >  #endif
> >
> > +/*
> > + * Take and release the kernel text modification lock, used
> > for code patching.
> > + * Users of this lock can sleep.
> > + */
> > +extern void kernel_text_lock(void);
> > +extern void kernel_text_unlock(void);
> > +
> >  #endif /* _LINUX_MEMORY_H_ */
> > Index: linux-2.6-lttng/mm/memory.c
> > ===================================================================
> > --- linux-2.6-lttng.orig/mm/memory.c	2007-11-07
> > 11:12:33.000000000 -0500
> > +++ linux-2.6-lttng/mm/memory.c	2007-11-07
> > 11:14:25.000000000 -0500
> > @@ -50,6 +50,8 @@
> >  #include <linux/delayacct.h>
> >  #include <linux/init.h>
> >  #include <linux/writeback.h>
> > +#include <linux/kprobes.h>
> > +#include <linux/mutex.h>
> >
> >  #include <asm/pgalloc.h>
> >  #include <asm/uaccess.h>
> > @@ -84,6 +86,12 @@ EXPORT_SYMBOL(high_memory);
> >
> >  int randomize_va_space __read_mostly = 1;
> >
> > +/*
> > + * mutex protecting text section modification (dynamic code
> > patching).
> > + * some users need to sleep (allocating memory...) while
> > they hold this lock.
> > + */
> > +static DEFINE_MUTEX(text_mutex);
> > +
> >  static int __init disable_randmaps(char *s)
> >  {
> >  	randomize_va_space = 0;
> > @@ -2748,3 +2756,29 @@ int access_process_vm(struct task_struct
> >
> >  	return buf - old_buf;
> >  }
> > +
> > +/**
> > + * kernel_text_lock     -   Take the kernel text modification lock
> > + *
> > + * Insures mutual write exclusion of kernel and modules text
> > live text
> > + * modification. Should be used for code patching.
> > + * Users of this lock can sleep.
> > + */
> > +void __kprobes kernel_text_lock(void)
> > +{
> > +	mutex_lock(&text_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(kernel_text_lock);
> > +
> > +/**
> > + * kernel_text_unlock   -   Release the kernel text modification lock
> > + *
> > + * Insures mutual write exclusion of kernel and modules text
> > live text
> > + * modification. Should be used for code patching.
> > + * Users of this lock can sleep.
> > + */
> > +void __kprobes kernel_text_unlock(void)
> > +{
> > +	mutex_unlock(&text_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(kernel_text_unlock);
> >
> > -- 
> > Mathieu Desnoyers
> > Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
> > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25
> > A8FE 3BAE 9A68
> > --
> > 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/
> >
> >
> 
> 
> 

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