[PATCH 1/2] nvme: Take refcount on transport module when using block device operations

[PATCH 1/2] nvme: Take refcount on transport module when using block device operations

[Max Gurtovoy]

From: Nitzan Carmi <nitzanc@mellanox.com>

The block device is backed by the transport so we must ensure that the related
ops driver will not be removed until all external application (e.g. LVM)
will release the reference. Otherwise, we might end up referencing freed memory.

Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Signed-off-by: Nitzan Carmi <nitzanc at mellanox.com>
---
 drivers/nvme/host/core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 839650e..cb6b08e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1217,16 +1217,27 @@ static int nvme_open(struct block_device *bdev, fmode_t mode)
 #ifdef CONFIG_NVME_MULTIPATH
 	/* should never be called due to GENHD_FL_HIDDEN */
 	if (WARN_ON_ONCE(ns->head->disk))
-		return -ENXIO;
+		goto fail;
 #endif
 	if (!kref_get_unless_zero(&ns->kref))
-		return -ENXIO;
+		goto fail;
+	if (!try_module_get(ns->ctrl->ops->module))
+		goto fail_put_ns;
+
 	return 0;
+
+fail_put_ns:
+	kref_put(&ns->kref, nvme_free_ns);
+fail:
+	return -ENXIO;
 }
 
 static void nvme_release(struct gendisk *disk, fmode_t mode)
 {
-	nvme_put_ns(disk->private_data);
+	struct nvme_ns *ns = disk->private_data;
+
+	module_put(ns->ctrl->ops->module);
+	nvme_put_ns(ns);
 }
 
 static int nvme_getgeo(struct block_device *bdev, struct hd_geometry *geo)
-- 
1.8.3.1

[PATCH 2/2] nvme: Fix subsystem multiple controllers support check

[Max Gurtovoy]

From: Israel Rukshin <israelr@mellanox.com>

There is a problem when another module (e.g. nvmet) takes
a refcount on the nvme block device and the physical nvme drive is removed.
In that case nvme_free_ctrl() will not be called and the controller state
will be "deleting" or "dead" unless nvmet module releases the block device.
Later on, the same nvme drive probes back and nvme_init_subsystem() will be
called and fail due to duplicate subnqn (if the nvme device doesn't support
subsystem with multiple controllers). This will cause a probe failure.
This commit changes the check of multiple controllers support at
nvme_init_subsystem() by not counting all the controllers at "dead" or
"deleting" state (this is safe because controllers at this state will never
be active again).

Fixes: ab9e00cc72fa ("nvme: track subsystems")
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Signed-off-by: Israel Rukshin <israelr at mellanox.com>
---
 drivers/nvme/host/core.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cb6b08e..f6dfdd3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2063,6 +2063,22 @@ static ssize_t nvme_subsys_show_nqn(struct device *dev,
 	NULL,
 };
 
+static int nvme_active_ctrls(struct nvme_subsystem *subsys)
+{
+	int count = 0;
+	struct nvme_ctrl *ctrl;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		if (ctrl->state != NVME_CTRL_DELETING &&
+		    ctrl->state != NVME_CTRL_DEAD)
+			count++;
+	}
+	mutex_unlock(&subsys->lock);
+
+	return count;
+}
+
 static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
 	struct nvme_subsystem *subsys, *found;
@@ -2101,7 +2117,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		 * Verify that the subsystem actually supports multiple
 		 * controllers, else bail out.
 		 */
-		if (!(id->cmic & (1 << 1))) {
+		if (nvme_active_ctrls(found) && !(id->cmic & (1 << 1))) {
 			dev_err(ctrl->device,
 				"ignoring ctrl due to duplicate subnqn (%s).\n",
 				found->subnqn);
-- 
1.8.3.1

[PATCH 2/2] nvme: Fix subsystem multiple controllers support check

[Christoph Hellwig]

device model refcounting bites again..

I don't really like the loop to much, but for now can't come up with
anything better.

Applied to nvme-4.16.

[PATCH 2/2] nvme: Fix subsystem multiple controllers support check

[Christoph Hellwig]

Btw, I suspect we'll need to to do something interesting here for
the multipath case as well.  Can you investigate that?

[PATCH 2/2] nvme: Fix subsystem multiple controllers support check

[Israel Rukshin]

On 1/8/2018 12:16 PM, Christoph Hellwig wrote:
> Btw, I suspect we'll need to to do something interesting here for
> the multipath case as well.  Can you investigate that?
Sure :-)

[PATCH 2/2] nvme: Fix subsystem multiple controllers support check

[Israel Rukshin]

On 1/8/2018 12:16 PM, Christoph Hellwig wrote:
> Btw, I suspect we'll need to to do something interesting here for
> the multipath case as well.  Can you investigate that?

Hi Christoph,

I have tested the same scenario with the multipath flag and it seems to 
work.
I created a target and took a refcount on "nvme0n1" block device and 
then I removed the nvme pci device.
After probing the pci I got a new name for the same nvme device "nvme0n2".
When I did the same test without the multipath case I got another new 
name "nvme1n2".

Regards,
Israel

[PATCH 1/2] nvme: Take refcount on transport module when using block device operations

[Christoph Hellwig]

Looks fine,

applied to nvme-4.16.

[PATCH 1/2] nvme: Take refcount on transport module when using block device operations

[Minwoo Im]

Hi Max,

On Fri, Jan 5, 2018@12:56 AM, Max Gurtovoy <maxg@mellanox.com> wrote:
> +       kref_put(&ns->kref, nvme_free_ns);

This seems identical to nvme_put_ns(ns);
It would look better if replaced with nvme_put_ns() function call just
like nvme_release()
that you have updated in this patch.

Thanks,

[PATCH 1/2] nvme: Take refcount on transport module when using block device operations

[Christoph Hellwig]

On Tue, Jan 09, 2018@12:51:11AM +0900, Minwoo Im wrote:
> Hi Max,
> 
> On Fri, Jan 5, 2018@12:56 AM, Max Gurtovoy <maxg@mellanox.com> wrote:
> > +       kref_put(&ns->kref, nvme_free_ns);
> 
> This seems identical to nvme_put_ns(ns);
> It would look better if replaced with nvme_put_ns() function call just
> like nvme_release()
> that you have updated in this patch.

I've fixed it in the applied patch, thanks!