| 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
| ||
|
Message-ID: <20100923170226.GC2406@linux.vnet.ibm.com>
Date: Thu, 23 Sep 2010 10:02:26 -0700
From: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To: Andi Kleen <andi@...stfloor.org>
Cc: linux-kernel@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>,
rusty@...tcorp.com.au, tglx@...utronix.de
Subject: Re: [PATCH] Convert preempt_disabled in module.c to rcu read lock
On Wed, Sep 22, 2010 at 03:07:20PM +0200, Andi Kleen wrote:
> From: Andi Kleen <ak@...ux.intel.com>
>
> Thomas Gleixner pointed out that the list_for_each_rcu()
> in module really need to use RCU read lock, not preempt disable.
> This is especially needed for the preemptive RCU code.
> >From what I understand the only reason for the preemption
> disabling is to protect against rcu, so using rcu_read_lock()
> is correct.
>
> Open: this will do rcu_read_lock() asynchronously in oopses. That's ok
> with all RCU implementations without deadlocks?
The rcu_read_unlock() slowpath has some locking in the preemptible RCU
implementations (TINY_PREEMPT_RCU and TREE_PREEMPT_RCU). These locks
are acquired only when exiting an RCU read-side critical section that
was preempted.
But given that Rusty noted that you need preempt_disable(), how about
using synchronize_sched() instead of synchronize_rcu() on the update side?
Then rcu_read_unlock_sched() just does a preempt_enable().
Thanx, Paul
> Cc: paulmck@...ux.vnet.ibm.com
> Cc: rusty@...tcorp.com.au
> Cc: tglx@...utronix.de
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> ---
> kernel/module.c | 78 ++++++++++++++++++++++++++----------------------------
> 1 files changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index d0b5f8d..ea99f4c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -74,7 +74,7 @@
>
> /*
> * Mutex protects:
> - * 1) List of modules (also safely readable with preempt_disable),
> + * 1) List of modules (also safely readable with rcu_read_lock),
> * 2) module_use links,
> * 3) module_addr_min/module_addr_max.
> * (delete uses stop_machine/add uses RCU list operations). */
> @@ -344,7 +344,7 @@ static bool find_symbol_in_section(const struct symsearch *syms,
> }
>
> /* Find a symbol and return it, along with, (optional) crc and
> - * (optional) module which owns it. Needs preempt disabled or module_mutex. */
> + * (optional) module which owns it. Needs rcu_read_lock or module_mutex. */
> const struct kernel_symbol *find_symbol(const char *name,
> struct module **owner,
> const unsigned long **crc,
> @@ -443,8 +443,7 @@ bool is_module_percpu_address(unsigned long addr)
> struct module *mod;
> unsigned int cpu;
>
> - preempt_disable();
> -
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (!mod->percpu_size)
> continue;
> @@ -453,13 +452,13 @@ bool is_module_percpu_address(unsigned long addr)
>
> if ((void *)addr >= start &&
> (void *)addr < start + mod->percpu_size) {
> - preempt_enable();
> + rcu_read_unlock();
> return true;
> }
> }
> }
>
> - preempt_enable();
> + rcu_read_unlock();
> return false;
> }
>
> @@ -831,11 +830,11 @@ void __symbol_put(const char *symbol)
> {
> struct module *owner;
>
> - preempt_disable();
> + rcu_read_lock();
> if (!find_symbol(symbol, &owner, NULL, true, false))
> BUG();
> module_put(owner);
> - preempt_enable();
> + rcu_read_unlock();
> }
> EXPORT_SYMBOL(__symbol_put);
>
> @@ -870,7 +869,7 @@ static struct module_attribute refcnt = {
> void module_put(struct module *module)
> {
> if (module) {
> - preempt_disable();
> + rcu_read_lock();
> smp_wmb(); /* see comment in module_refcount */
> __this_cpu_inc(module->refptr->decs);
>
> @@ -878,7 +877,7 @@ void module_put(struct module *module)
> /* Maybe they're waiting for us to drop reference? */
> if (unlikely(!module_is_live(module)))
> wake_up_process(module->waiter);
> - preempt_enable();
> + rcu_read_unlock();
> }
> }
> EXPORT_SYMBOL(module_put);
> @@ -1584,11 +1583,11 @@ void *__symbol_get(const char *symbol)
> struct module *owner;
> const struct kernel_symbol *sym;
>
> - preempt_disable();
> + rcu_read_lock();
> sym = find_symbol(symbol, &owner, NULL, true, true);
> if (sym && strong_try_module_get(owner))
> sym = NULL;
> - preempt_enable();
> + rcu_read_unlock();
>
> return sym ? (void *)sym->value : NULL;
> }
> @@ -2813,7 +2812,7 @@ static const char *get_ksymbol(struct module *mod,
> }
>
> /* For kallsyms to ask for address resolution. NULL means not found. Careful
> - * not to lock to avoid deadlock on oopses, simply disable preemption. */
> + * not to lock to avoid deadlock on oopses, simply use rcu read lock. */
> const char *module_address_lookup(unsigned long addr,
> unsigned long *size,
> unsigned long *offset,
> @@ -2823,7 +2822,7 @@ const char *module_address_lookup(unsigned long addr,
> struct module *mod;
> const char *ret = NULL;
>
> - preempt_disable();
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (within_module_init(addr, mod) ||
> within_module_core(addr, mod)) {
> @@ -2834,11 +2833,12 @@ const char *module_address_lookup(unsigned long addr,
> }
> }
> /* Make a copy in here where it's safe */
> + /* AK: The string copy is likely racy. */
> if (ret) {
> strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> ret = namebuf;
> }
> - preempt_enable();
> + rcu_read_unlock();
> return ret;
> }
>
> @@ -2846,7 +2846,7 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
> {
> struct module *mod;
>
> - preempt_disable();
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (within_module_init(addr, mod) ||
> within_module_core(addr, mod)) {
> @@ -2854,14 +2854,13 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
>
> sym = get_ksymbol(mod, addr, NULL, NULL);
> if (!sym)
> - goto out;
> + break;
> strlcpy(symname, sym, KSYM_NAME_LEN);
> - preempt_enable();
> + rcu_read_unlock();
> return 0;
> }
> }
> -out:
> - preempt_enable();
> + rcu_read_unlock();
> return -ERANGE;
> }
>
> @@ -2870,7 +2869,7 @@ int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
> {
> struct module *mod;
>
> - preempt_disable();
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (within_module_init(addr, mod) ||
> within_module_core(addr, mod)) {
> @@ -2878,17 +2877,16 @@ int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
>
> sym = get_ksymbol(mod, addr, size, offset);
> if (!sym)
> - goto out;
> + break;
> if (modname)
> strlcpy(modname, mod->name, MODULE_NAME_LEN);
> if (name)
> strlcpy(name, sym, KSYM_NAME_LEN);
> - preempt_enable();
> + rcu_read_unlock();
> return 0;
> }
> }
> -out:
> - preempt_enable();
> + rcu_read_unlock();
> return -ERANGE;
> }
>
> @@ -2897,7 +2895,7 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> {
> struct module *mod;
>
> - preempt_disable();
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (symnum < mod->num_symtab) {
> *value = mod->symtab[symnum].st_value;
> @@ -2906,12 +2904,12 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> KSYM_NAME_LEN);
> strlcpy(module_name, mod->name, MODULE_NAME_LEN);
> *exported = is_exported(name, *value, mod);
> - preempt_enable();
> + rcu_read_unlock();
> return 0;
> }
> symnum -= mod->num_symtab;
> }
> - preempt_enable();
> + rcu_read_unlock();
> return -ERANGE;
> }
>
> @@ -2934,7 +2932,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
> unsigned long ret = 0;
>
> /* Don't lock: we're in enough trouble already. */
> - preempt_disable();
> + rcu_read_lock();
> if ((colon = strchr(name, ':')) != NULL) {
> *colon = '\0';
> if ((mod = find_module(name)) != NULL)
> @@ -2945,7 +2943,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
> if ((ret = mod_find_symname(mod, name)) != 0)
> break;
> }
> - preempt_enable();
> + rcu_read_unlock();
> return ret;
> }
>
> @@ -3083,7 +3081,7 @@ const struct exception_table_entry *search_module_extables(unsigned long addr)
> const struct exception_table_entry *e = NULL;
> struct module *mod;
>
> - preempt_disable();
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list) {
> if (mod->num_exentries == 0)
> continue;
> @@ -3094,7 +3092,7 @@ const struct exception_table_entry *search_module_extables(unsigned long addr)
> if (e)
> break;
> }
> - preempt_enable();
> + rcu_read_unlock();
>
> /* Now, if we found one, we are running inside it now, hence
> we cannot unload the module, hence no refcnt needed. */
> @@ -3112,9 +3110,9 @@ bool is_module_address(unsigned long addr)
> {
> bool ret;
>
> - preempt_disable();
> + rcu_read_lock();
> ret = __module_address(addr) != NULL;
> - preempt_enable();
> + rcu_read_unlock();
>
> return ret;
> }
> @@ -3153,9 +3151,9 @@ bool is_module_text_address(unsigned long addr)
> {
> bool ret;
>
> - preempt_disable();
> + rcu_read_lock();
> ret = __module_text_address(addr) != NULL;
> - preempt_enable();
> + rcu_read_unlock();
>
> return ret;
> }
> @@ -3164,7 +3162,7 @@ bool is_module_text_address(unsigned long addr)
> * __module_text_address - get the module whose code contains an address.
> * @addr: the address.
> *
> - * Must be called with preempt disabled or module mutex held so that
> + * Must be called with rcu read lock or module mutex held so that
> * module doesn't get freed during this.
> */
> struct module *__module_text_address(unsigned long addr)
> @@ -3187,11 +3185,11 @@ void print_modules(void)
> char buf[8];
>
> printk(KERN_DEFAULT "Modules linked in:");
> - /* Most callers should already have preempt disabled, but make sure */
> - preempt_disable();
> + /* Most callers should already have rcu read lock but make sure */
> + rcu_read_lock();
> list_for_each_entry_rcu(mod, &modules, list)
> printk(" %s%s", mod->name, module_flags(mod, buf));
> - preempt_enable();
> + rcu_read_unlock();
> if (last_unloaded_module[0])
> printk(" [last unloaded: %s]", last_unloaded_module);
> printk("\n");
> --
> 1.7.1
>
> --
> 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/
--
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/
Powered by blists - more mailing lists