nvme: Take refcount on transport module when using block device operations

From: Nitzan Carmi <[email protected]>

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.

---
 drivers/nvme/host/core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

index 839650e..cb6b08e 100644
@@ -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

From: Israel Rukshin <[email protected]>

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).

---
 drivers/nvme/host/core.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

index cb6b08e..f6dfdd3 100644
@@ -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

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.

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

Sure :-)

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

Looks fine,

applied to nvme-4.16.

Hi Max,

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,

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