From patchwork Thu Feb 13 15:32:05 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hannes Reinecke X-Patchwork-Id: 11380577 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4E9B592A for ; Thu, 13 Feb 2020 15:35:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 37D6A20675 for ; Thu, 13 Feb 2020 15:35:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728366AbgBMPfD (ORCPT ); Thu, 13 Feb 2020 10:35:03 -0500 Received: from mx2.suse.de ([195.135.220.15]:37674 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388138AbgBMPcL (ORCPT ); Thu, 13 Feb 2020 10:32:11 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1C291AF0E; Thu, 13 Feb 2020 15:32:09 +0000 (UTC) From: Hannes Reinecke To: "Martin K. Petersen" Cc: Christoph Hellwig , Bart van Assche , James Bottomley , linux-scsi@vger.kernel.org, Hannes Reinecke Subject: [PATCH 1/3] ch: fixup refcounting imbalance for SCSI devices Date: Thu, 13 Feb 2020 16:32:05 +0100 Message-Id: <20200213153207.123357-2-hare@suse.de> X-Mailer: git-send-email 2.16.4 In-Reply-To: <20200213153207.123357-1-hare@suse.de> References: <20200213153207.123357-1-hare@suse.de> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org The SCSI device is required to be present during ch_probe() and ch_open(). But the SCSI device itself is only checked during ch_open(), so it's anyones guess if it had been present during ch_probe(). And consequently we can't reliably detach it during ch_release(), as ch_remove() might have been called first. So initialize the changer device during ch_probe(), and take a reference to the SCSI device during both ch_probe() and ch_open(). Signed-off-by: Hannes Reinecke Reviewed-by: Bart Van Assche --- drivers/scsi/ch.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c index ed5f4a6ae270..974afb4bd5fe 100644 --- a/drivers/scsi/ch.c +++ b/drivers/scsi/ch.c @@ -569,6 +569,7 @@ static void ch_destroy(struct kref *ref) { scsi_changer *ch = container_of(ref, scsi_changer, ref); + ch->device = NULL; kfree(ch->dt); kfree(ch); } @@ -594,14 +595,17 @@ ch_open(struct inode *inode, struct file *file) spin_lock(&ch_index_lock); ch = idr_find(&ch_index_idr, minor); - if (NULL == ch || scsi_device_get(ch->device)) { + if (NULL == ch || !kref_get_unless_zero(&ch->ref)) { spin_unlock(&ch_index_lock); mutex_unlock(&ch_mutex); return -ENXIO; } - kref_get(&ch->ref); spin_unlock(&ch_index_lock); - + if (scsi_device_get(ch->device)) { + kref_put(&ch->ref, ch_destroy); + mutex_unlock(&ch_mutex); + return -ENXIO; + } file->private_data = ch; mutex_unlock(&ch_mutex); return 0; @@ -938,6 +942,12 @@ static int ch_probe(struct device *dev) ch->minor = ret; sprintf(ch->name,"ch%d",ch->minor); + ret = scsi_device_get(sd); + if (ret) { + sdev_printk(KERN_WARNING, sd, "ch%d: failed to get device\n", + ch->minor); + goto remove_idr; + } class_dev = device_create(ch_sysfs_class, dev, MKDEV(SCSI_CHANGER_MAJOR, ch->minor), ch, @@ -946,7 +956,7 @@ static int ch_probe(struct device *dev) sdev_printk(KERN_WARNING, sd, "ch%d: device_create failed\n", ch->minor); ret = PTR_ERR(class_dev); - goto remove_idr; + goto put_device; } mutex_init(&ch->lock); @@ -964,6 +974,8 @@ static int ch_probe(struct device *dev) return 0; destroy_dev: device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR, ch->minor)); +put_device: + scsi_device_put(sd); remove_idr: idr_remove(&ch_index_idr, ch->minor); free_ch: @@ -977,9 +989,11 @@ static int ch_remove(struct device *dev) spin_lock(&ch_index_lock); idr_remove(&ch_index_idr, ch->minor); + dev_set_drvdata(dev, NULL); spin_unlock(&ch_index_lock); device_destroy(ch_sysfs_class, MKDEV(SCSI_CHANGER_MAJOR,ch->minor)); + scsi_device_put(ch->device); kref_put(&ch->ref, ch_destroy); return 0; } From patchwork Thu Feb 13 15:32:06 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hannes Reinecke X-Patchwork-Id: 11380583 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id EC84292A for ; Thu, 13 Feb 2020 15:35:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CAA5B217F4 for ; Thu, 13 Feb 2020 15:35:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728869AbgBMPfF (ORCPT ); Thu, 13 Feb 2020 10:35:05 -0500 Received: from mx2.suse.de ([195.135.220.15]:37682 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388141AbgBMPcK (ORCPT ); Thu, 13 Feb 2020 10:32:10 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1D1E3B137; Thu, 13 Feb 2020 15:32:09 +0000 (UTC) From: Hannes Reinecke To: "Martin K. Petersen" Cc: Christoph Hellwig , Bart van Assche , James Bottomley , linux-scsi@vger.kernel.org, Hannes Reinecke Subject: [PATCH 2/3] ch: synchronize ch_probe() and ch_open() Date: Thu, 13 Feb 2020 16:32:06 +0100 Message-Id: <20200213153207.123357-3-hare@suse.de> X-Mailer: git-send-email 2.16.4 In-Reply-To: <20200213153207.123357-1-hare@suse.de> References: <20200213153207.123357-1-hare@suse.de> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org The 'ch' device node is created before the configuration is being read in, which leads to a race window when ch_open() is called before that. To avoid any races we should be taking the device mutex during ch_readconfig() and ch_init_elem(), and also during ch_open(). That ensures ch_probe is finished before ch_open() completes. Signed-off-by: Hannes Reinecke Reviewed-by: Bart Van Assche --- drivers/scsi/ch.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c index 974afb4bd5fe..9cbfb00ab950 100644 --- a/drivers/scsi/ch.c +++ b/drivers/scsi/ch.c @@ -606,7 +606,10 @@ ch_open(struct inode *inode, struct file *file) mutex_unlock(&ch_mutex); return -ENXIO; } + /* Synchronize with ch_probe() */ + mutex_lock(&ch->lock); file->private_data = ch; + mutex_unlock(&ch->lock); mutex_unlock(&ch_mutex); return 0; } @@ -949,6 +952,9 @@ static int ch_probe(struct device *dev) goto remove_idr; } + mutex_init(&ch->lock); + kref_init(&ch->ref); + ch->device = sd; class_dev = device_create(ch_sysfs_class, dev, MKDEV(SCSI_CHANGER_MAJOR, ch->minor), ch, "s%s", ch->name); @@ -959,15 +965,16 @@ static int ch_probe(struct device *dev) goto put_device; } - mutex_init(&ch->lock); - kref_init(&ch->ref); - ch->device = sd; + mutex_lock(&ch->lock); ret = ch_readconfig(ch); - if (ret) + if (ret) { + mutex_unlock(&ch->lock); goto destroy_dev; + } if (init) ch_init_elem(ch); + mutex_unlock(&ch->lock); dev_set_drvdata(dev, ch); sdev_printk(KERN_INFO, sd, "Attached scsi changer %s\n", ch->name); From patchwork Thu Feb 13 15:32:07 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hannes Reinecke X-Patchwork-Id: 11380579 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 7CC60139A for ; Thu, 13 Feb 2020 15:35:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5BD9C20675 for ; Thu, 13 Feb 2020 15:35:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728383AbgBMPfD (ORCPT ); Thu, 13 Feb 2020 10:35:03 -0500 Received: from mx2.suse.de ([195.135.220.15]:37680 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388140AbgBMPcL (ORCPT ); Thu, 13 Feb 2020 10:32:11 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 1CBEDB12D; Thu, 13 Feb 2020 15:32:09 +0000 (UTC) From: Hannes Reinecke To: "Martin K. Petersen" Cc: Christoph Hellwig , Bart van Assche , James Bottomley , linux-scsi@vger.kernel.org, Hannes Reinecke Subject: [PATCH 3/3] ch: remove ch_mutex() Date: Thu, 13 Feb 2020 16:32:07 +0100 Message-Id: <20200213153207.123357-4-hare@suse.de> X-Mailer: git-send-email 2.16.4 In-Reply-To: <20200213153207.123357-1-hare@suse.de> References: <20200213153207.123357-1-hare@suse.de> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org ch_mutex() was introduced with a mechanical conversion, but as we now have correct locking we can remove it altogether. Signed-off-by: Hannes Reinecke --- drivers/scsi/ch.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c index 9cbfb00ab950..4ea3b61f275f 100644 --- a/drivers/scsi/ch.c +++ b/drivers/scsi/ch.c @@ -44,7 +44,6 @@ MODULE_LICENSE("GPL"); MODULE_ALIAS_CHARDEV_MAJOR(SCSI_CHANGER_MAJOR); MODULE_ALIAS_SCSI_DEVICE(TYPE_MEDIUM_CHANGER); -static DEFINE_MUTEX(ch_mutex); static int init = 1; module_param(init, int, 0444); MODULE_PARM_DESC(init, \ @@ -591,26 +590,22 @@ ch_open(struct inode *inode, struct file *file) scsi_changer *ch; int minor = iminor(inode); - mutex_lock(&ch_mutex); spin_lock(&ch_index_lock); ch = idr_find(&ch_index_idr, minor); if (NULL == ch || !kref_get_unless_zero(&ch->ref)) { spin_unlock(&ch_index_lock); - mutex_unlock(&ch_mutex); return -ENXIO; } spin_unlock(&ch_index_lock); if (scsi_device_get(ch->device)) { kref_put(&ch->ref, ch_destroy); - mutex_unlock(&ch_mutex); return -ENXIO; } /* Synchronize with ch_probe() */ mutex_lock(&ch->lock); file->private_data = ch; mutex_unlock(&ch->lock); - mutex_unlock(&ch_mutex); return 0; }