Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754766AbbG1Aby (ORCPT ); Mon, 27 Jul 2015 20:31:54 -0400 Received: from ozlabs.org ([103.22.144.67]:45065 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753934AbbG1Abx (ORCPT ); Mon, 27 Jul 2015 20:31:53 -0400 From: Rusty Russell To: He Kuang Cc: wangnan0@huawei.com, linux-kernel@vger.kernel.org, "Peter Zijlstra" Subject: Re: [PATCH] module: Fix missing to hold module_mutex lock in module_kallsyms_lookup_name In-Reply-To: <1437991286-101906-1-git-send-email-hekuang@huawei.com> References: <1437991286-101906-1-git-send-email-hekuang@huawei.com> User-Agent: Notmuch/0.17 (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Tue, 28 Jul 2015 10:01:35 +0930 Message-ID: <878ua1p1g8.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2735 Lines: 80 He Kuang writes: > Function find_module_all() searches for module by name and must be > called with module_mutex. module_kallsyms_lookup_name() calls it without > this mutex which emits a warning message (CONFIG_LOCKDEP=y) by failed > assertion for testing this module_mutex lock, as following: > > [ 202.877152] ------------[ cut here ]------------ > [ 202.881070] WARNING: CPU: 0 PID: 1010 at kernel/module.c:281 > module_assert_mutex+0x35/0x40() > [ 202.885446] Modules linked in: test_bpf > [ 202.886997] CPU: 0 PID: 1010 Comm: perf Tainted: G W 4.2.0-rc3+ #5 > ... > > This patch wraps this call with mutex_{lock,unlock} and fix the bug. Hi He! Thanks for this report! This warning is overzealous; preempt disabling should be enough to read the list. Unfortunately, as you can see from the comment, taking a lock is a bad idea here: it's called in the oops path (we don't want to risk deadlock). Peter? Cheers, Rusty. module: weaken locking assertion for oops path. We don't actually hold the module_mutex when calling find_module_all from module_kallsyms_lookup_name: that's because it's used by the oops code and we don't want to deadlock. However, access to the list read-only is safe if preempt is disabled, so we can weaken the assertion. Keep a strong version for external callers though. Fixes: 0be964be0d45 ("module: Sanitize RCU usage and locking") Reported-by: He Kuang Signed-off-by: Rusty Russell diff --git a/kernel/module.c b/kernel/module.c index 4d2b82e610e2..b86b7bf1be38 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -602,13 +602,16 @@ const struct kernel_symbol *find_symbol(const char *name, } EXPORT_SYMBOL_GPL(find_symbol); -/* Search for module by name: must hold module_mutex. */ +/* + * Search for module by name: must hold module_mutex (or preempt disabled + * for read-only access). + */ static struct module *find_module_all(const char *name, size_t len, bool even_unformed) { struct module *mod; - module_assert_mutex(); + module_assert_mutex_or_preempt(); list_for_each_entry(mod, &modules, list) { if (!even_unformed && mod->state == MODULE_STATE_UNFORMED) @@ -621,6 +624,7 @@ static struct module *find_module_all(const char *name, size_t len, struct module *find_module(const char *name) { + module_assert_mutex(); return find_module_all(name, strlen(name), false); } EXPORT_SYMBOL_GPL(find_module); -- 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/