Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753457AbbDNGmj (ORCPT ); Tue, 14 Apr 2015 02:42:39 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:51769 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753382AbbDNGmW (ORCPT ); Tue, 14 Apr 2015 02:42:22 -0400 Date: Tue, 14 Apr 2015 08:41:49 +0200 From: Peter Zijlstra To: Rusty Russell Cc: mingo@kernel.org, mathieu.desnoyers@efficios.com, oleg@redhat.com, paulmck@linux.vnet.ibm.com, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, andi@firstfloor.org, rostedt@goodmis.org, tglx@linutronix.de, laijs@cn.fujitsu.com, linux@horizon.com Subject: Re: [PATCH v5 00/10] latched RB-trees and __module_address() Message-ID: <20150414064149.GM23123@twins.programming.kicks-ass.net> References: <20150413141126.756350256@infradead.org> <87wq1fs9ha.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87wq1fs9ha.fsf@rustcorp.com.au> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6260 Lines: 204 On Tue, Apr 14, 2015 at 12:27:05PM +0930, Rusty Russell wrote: > I was tempted to sneak in those module rcu fixes for 4.1, but seeing > Ingo's comments I'll wait for 4.2. I can get you a new version of that if you want. See below. The fixups are unmodified of the posting (patches 2,3). --- Subject: module: Sanitize RCU usage and locking From: Peter Zijlstra Date: Sat Feb 28 19:17:04 CET 2015 Currently the RCU usage in module is an inconsistent mess of RCU and RCU-sched, this is broken for CONFIG_PREEMPT where synchronize_rcu() does not imply synchronize_sched(). Most usage sites use preempt_{dis,en}able() which is RCU-sched, but (most of) the modification sites use synchronize_rcu(). With the exception of the module bug list, which actually uses RCU. Convert everything over to RCU-sched. Furthermore add lockdep asserts to all sites, because it's not at all clear to me the required locking is observed, esp. on exported functions. Cc: Rusty Russell Acked-by: "Paul E. McKenney" Signed-off-by: Peter Zijlstra (Intel) --- include/linux/module.h | 12 ++++++++++-- kernel/module.c | 40 ++++++++++++++++++++++++++++++++-------- lib/bug.c | 7 +++++-- 3 files changed, 47 insertions(+), 12 deletions(-) --- a/include/linux/module.h +++ b/include/linux/module.h @@ -419,14 +419,22 @@ struct symsearch { bool unused; }; -/* Search for an exported symbol by name. */ +/* + * Search for an exported symbol by name. + * + * Must be called with module_mutex held or preemption disabled. + */ const struct kernel_symbol *find_symbol(const char *name, struct module **owner, const unsigned long **crc, bool gplok, bool warn); -/* Walk the exported symbol table */ +/* + * Walk the exported symbol table + * + * Must be called with module_mutex held or preemption disabled. + */ bool each_symbol_section(bool (*fn)(const struct symsearch *arr, struct module *owner, void *data), void *data); --- a/kernel/module.c +++ b/kernel/module.c @@ -105,6 +105,22 @@ static LIST_HEAD(modules); struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ #endif /* CONFIG_KGDB_KDB */ +static void module_assert_mutex(void) +{ + lockdep_assert_held(&module_mutex); +} + +static void module_assert_mutex_or_preempt(void) +{ +#ifdef CONFIG_LOCKDEP + if (!unlikely(debug_locks)) + return; + + WARN_ON(!rcu_held_lock_sched_held() && + !lockdep_is_held(&module_mutex)); +#endif +} + #ifdef CONFIG_MODULE_SIG #ifdef CONFIG_MODULE_SIG_FORCE static bool sig_enforce = true; @@ -318,6 +334,8 @@ bool each_symbol_section(bool (*fn)(cons #endif }; + module_assert_mutex_or_preempt(); + if (each_symbol_in_section(arr, ARRAY_SIZE(arr), NULL, fn, data)) return true; @@ -457,6 +475,8 @@ static struct module *find_module_all(co { struct module *mod; + module_assert_mutex(); + list_for_each_entry(mod, &modules, list) { if (!even_unformed && mod->state == MODULE_STATE_UNFORMED) continue; @@ -1854,8 +1874,8 @@ static void free_module(struct module *m list_del_rcu(&mod->list); /* Remove this module from bug list, this uses list_del_rcu */ module_bug_cleanup(mod); - /* Wait for RCU synchronizing before releasing mod->list and buglist. */ - synchronize_rcu(); + /* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */ + synchronize_sched(); mutex_unlock(&module_mutex); /* This may be NULL, but that's OK */ @@ -3106,11 +3126,11 @@ static noinline int do_init_module(struc mod->init_text_size = 0; /* * We want to free module_init, but be aware that kallsyms may be - * walking this with preempt disabled. In all the failure paths, - * we call synchronize_rcu/synchronize_sched, but we don't want - * to slow down the success path, so use actual RCU here. + * walking this with preempt disabled. In all the failure paths, we + * call synchronize_sched(), but we don't want to slow down the success + * path, so use actual RCU here. */ - call_rcu(&freeinit->rcu, do_free_init); + call_rcu_sched(&freeinit->rcu, do_free_init); mutex_unlock(&module_mutex); wake_up_all(&module_wq); @@ -3368,8 +3388,8 @@ static int load_module(struct load_info /* Unlink carefully: kallsyms could be walking list. */ list_del_rcu(&mod->list); wake_up_all(&module_wq); - /* Wait for RCU synchronizing before releasing mod->list. */ - synchronize_rcu(); + /* Wait for RCU-sched synchronizing before releasing mod->list. */ + synchronize_sched(); mutex_unlock(&module_mutex); free_module: /* Free lock-classes; relies on the preceding sync_rcu() */ @@ -3636,6 +3656,8 @@ int module_kallsyms_on_each_symbol(int ( unsigned int i; int ret; + module_assert_mutex(); + list_for_each_entry(mod, &modules, list) { if (mod->state == MODULE_STATE_UNFORMED) continue; @@ -3810,6 +3832,8 @@ struct module *__module_address(unsigned if (addr < module_addr_min || addr > module_addr_max) return NULL; + module_assert_mutex_or_preempt(); + list_for_each_entry_rcu(mod, &modules, list) { if (mod->state == MODULE_STATE_UNFORMED) continue; --- a/lib/bug.c +++ b/lib/bug.c @@ -66,7 +66,7 @@ static const struct bug_entry *module_fi struct module *mod; const struct bug_entry *bug = NULL; - rcu_read_lock(); + rcu_read_lock_sched(); list_for_each_entry_rcu(mod, &module_bug_list, bug_list) { unsigned i; @@ -77,7 +77,7 @@ static const struct bug_entry *module_fi } bug = NULL; out: - rcu_read_unlock(); + rcu_read_unlock_sched(); return bug; } @@ -88,6 +88,8 @@ void module_bug_finalize(const Elf_Ehdr char *secstrings; unsigned int i; + lockdep_assert_held(&module_mutex); + mod->bug_table = NULL; mod->num_bugs = 0; @@ -113,6 +115,7 @@ void module_bug_finalize(const Elf_Ehdr void module_bug_cleanup(struct module *mod) { + lockdep_assert_held(&module_mutex); list_del_rcu(&mod->bug_list); } -- 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/