From patchwork Tue Feb 19 00:09:05 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 10819049 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id C9A566C2 for ; Tue, 19 Feb 2019 00:12:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B43342BDE4 for ; Tue, 19 Feb 2019 00:12:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A650B2BDEC; Tue, 19 Feb 2019 00:12:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from pdx1-mailman02.dreamhost.com (pdx1-mailman02.dreamhost.com [64.90.62.194]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4DDDD2BDE4 for ; Tue, 19 Feb 2019 00:12:28 +0000 (UTC) Received: from pdx1-mailman02.dreamhost.com (localhost [IPv6:::1]) by pdx1-mailman02.dreamhost.com (Postfix) with ESMTP id 1D47921F709; Mon, 18 Feb 2019 16:12:28 -0800 (PST) X-Original-To: lustre-devel@lists.lustre.org Delivered-To: lustre-devel-lustre.org@pdx1-mailman02.dreamhost.com Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) by pdx1-mailman02.dreamhost.com (Postfix) with ESMTP id 9D88E21FA3A for ; Mon, 18 Feb 2019 16:12:25 -0800 (PST) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id BB015AF57; Tue, 19 Feb 2019 00:12:24 +0000 (UTC) From: NeilBrown To: James Simmons , Andreas Dilger , Oleg Drokin Date: Tue, 19 Feb 2019 11:09:05 +1100 Message-ID: <155053494536.24125.16645429190038913975.stgit@noble.brown> In-Reply-To: <155053473693.24125.6976971762921761309.stgit@noble.brown> References: <155053473693.24125.6976971762921761309.stgit@noble.brown> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Subject: [lustre-devel] [PATCH 09/37] lustre: obdclass: fix module load locking. X-BeenThere: lustre-devel@lists.lustre.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "For discussing Lustre software development." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Lustre Development List Errors-To: lustre-devel-bounces@lists.lustre.org Sender: "lustre-devel" X-Virus-Scanned: ClamAV using ClamSMTP Safe module loading requires that we try_module_get() in a context where the module cannot be unloaded, typically protected by a spinlock that module-unload has to take. This doesn't currently happen in class_get_type(). As free_module() calls synchronize_rcu() between calling the exit function and freeing the module, we can use rcu_read_lock() to check if the exit function has been called, and try_module_get() if it hasn't. We must also check the return status of try_module_get(). Signed-off-by: NeilBrown Reviewed-by: James Simmons --- drivers/staging/lustre/lustre/obdclass/genops.c | 24 ++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c index d013e9a358f1..1d1a6b2d436e 100644 --- a/drivers/staging/lustre/lustre/obdclass/genops.c +++ b/drivers/staging/lustre/lustre/obdclass/genops.c @@ -98,21 +98,31 @@ static struct obd_type *class_get_type(const char *name) { struct obd_type *type; + rcu_read_lock(); type = class_search_type(name); if (!type) { const char *modname = name; + rcu_read_unlock(); if (!request_module("%s", modname)) { CDEBUG(D_INFO, "Loaded module '%s'\n", modname); - type = class_search_type(name); } else { LCONSOLE_ERROR_MSG(0x158, "Can't load module '%s'\n", modname); } + rcu_read_lock(); + type = class_search_type(name); } if (type) { - if (try_module_get(type->typ_dt_ops->owner)) { + /* + * Holding rcu_read_lock() matches the synchronize_rcu() call + * in free_module() and ensures that if type->typ_dt_ops is + * not yet NULL, then the module won't be freed until after + * we rcu_read_unlock(). + */ + const struct obd_ops *dt_ops = READ_ONCE(type->typ_dt_ops); + if (dt_ops && try_module_get(dt_ops->owner)) { refcount_inc(&type->typ_refcnt); /* class_search_type() returned a counted reference, * but we don't need that count any more as @@ -124,6 +134,7 @@ static struct obd_type *class_get_type(const char *name) type = NULL; } } + rcu_read_unlock(); return type; } @@ -213,11 +224,18 @@ int class_unregister_type(const char *name) return -EINVAL; } + /* + * Ensure that class_get_type doesn't try to get the module + * as it could be freed before the obd_type is released. + * synchronize_rcu() will be called before the module + * is freed. + */ + type->typ_dt_ops = NULL; + if (refcount_read(&type->typ_refcnt)) { CERROR("type %s has refcount (%d)\n", name, refcount_read(&type->typ_refcnt)); /* This is a bad situation, let's make the best of it */ /* Remove ops, but leave the name for debugging */ - type->typ_dt_ops = NULL; type->typ_md_ops = NULL; kobject_put(&type->typ_kobj); return -EBUSY;