From patchwork Wed Oct 31 23:01:33 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: NeilBrown X-Patchwork-Id: 10663317 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 42BCB13BF for ; Wed, 31 Oct 2018 23:01:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 221802B984 for ; Wed, 31 Oct 2018 23:01:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 12D582B9F2; Wed, 31 Oct 2018 23:01:55 +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 8B88C2B984 for ; Wed, 31 Oct 2018 23:01:54 +0000 (UTC) Received: from pdx1-mailman02.dreamhost.com (localhost [IPv6:::1]) by pdx1-mailman02.dreamhost.com (Postfix) with ESMTP id 2DF7C21F81B; Wed, 31 Oct 2018 16:01:49 -0700 (PDT) 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 B7E8121F7B5 for ; Wed, 31 Oct 2018 16:01:46 -0700 (PDT) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 6A362AE06; Wed, 31 Oct 2018 23:01:45 +0000 (UTC) From: NeilBrown To: quentin.bouget@cea.fr, Andreas Dilger , "Oleg Drokin" , James Simmons Date: Thu, 01 Nov 2018 10:01:33 +1100 In-Reply-To: <7db27e7d-e685-af8f-80d8-891a0d8db4d5@cea.fr> References: <87bm7a3nue.fsf@notabene.neil.brown.name> <7db27e7d-e685-af8f-80d8-891a0d8db4d5@cea.fr> Message-ID: <875zxh3elu.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Subject: [lustre-devel] [PATCH v2] lustre: mdc: fix possible deadlock in chlg_open() 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 Lockdep reports a possible deadlock between chlg_open() and mdc_changelog_cdev_init() mdc_changelog_cdev_init() takes chlg_registered_dev_lock and then calls misc_register() which takes misc_mtx. chlg_open() is called while misc_mtx is held, and tries to take chlg_registered_dev_lock. If these two functions race, a deadlock can occur as each thread will hold one of the locks while trying to take the other. chlg_open() does not need to take a lock. It only uses the lock to stablize a list while looking for the matching chlg_registered_dev, and this can be found directly by examining file->private_data. So remove chlg_obd_get(), and use file->private_data to find the obd_device. Also ensure the device is fully initialized before calling misc_register(). This means setting up some list linkage before the call, and tearing it down if there is an error. Signed-off-by: NeilBrown Reviewed-by: James Simmons Reviewed-by: Quentin Bouget Reviewed-by: James Simmons <jsimmons@infradead.org> Reviewed-by: Quentin Bouget <quentin.bouget@cea.fr> --- This is the revised version with the problem identified by Quentin fixed. drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 46 +++++++---------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c index d83507cbf95c..af29ea73c48a 100644 --- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c +++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c @@ -444,31 +444,6 @@ static ssize_t chlg_write(struct file *file, const char __user *buff, return rc < 0 ? rc : count; } -/** - * Find the OBD device associated to a changelog character device. - * @param[in] cdev character device instance descriptor - * @return corresponding OBD device or NULL if none was found. - */ -static struct obd_device *chlg_obd_get(dev_t cdev) -{ - int minor = MINOR(cdev); - struct obd_device *obd = NULL; - struct chlg_registered_dev *curr; - - mutex_lock(&chlg_registered_dev_lock); - list_for_each_entry(curr, &chlg_registered_devices, ced_link) { - if (curr->ced_misc.minor == minor) { - /* take the first available OBD device attached */ - obd = list_first_entry(&curr->ced_obds, - struct obd_device, - u.cli.cl_chg_dev_linkage); - break; - } - } - mutex_unlock(&chlg_registered_dev_lock); - return obd; -} - /** * Open handler, initialize internal CRS state and spawn prefetch thread if * needed. @@ -479,12 +454,16 @@ static struct obd_device *chlg_obd_get(dev_t cdev) static int chlg_open(struct inode *inode, struct file *file) { struct chlg_reader_state *crs; - struct obd_device *obd = chlg_obd_get(inode->i_rdev); + struct miscdevice *misc = file->private_data; + struct chlg_registered_dev *dev; + struct obd_device *obd; struct task_struct *task; int rc; - if (!obd) - return -ENODEV; + dev = container_of(misc, struct chlg_registered_dev, ced_misc); + obd = list_first_entry(&dev->ced_obds, + struct obd_device, + u.cli.cl_chg_dev_linkage); crs = kzalloc(sizeof(*crs), GFP_KERNEL); if (!crs) @@ -669,13 +648,16 @@ int mdc_changelog_cdev_init(struct obd_device *obd) goto out_unlock; } + list_add_tail(&obd->u.cli.cl_chg_dev_linkage, &entry->ced_obds); + list_add_tail(&entry->ced_link, &chlg_registered_devices); + /* Register new character device */ rc = misc_register(&entry->ced_misc); - if (rc != 0) + if (rc != 0) { + list_del_init(&obd->u.cli.cl_chg_dev_linkage); + list_del(&entry->ced_link); goto out_unlock; - - list_add_tail(&obd->u.cli.cl_chg_dev_linkage, &entry->ced_obds); - list_add_tail(&entry->ced_link, &chlg_registered_devices); + } entry = NULL; /* prevent it from being freed below */