Re: [PATCH 1/1] nvme: correctly account for namespace head reference counter

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/23/25 5:50 PM, Christoph Hellwig wrote:
> On Fri, Jun 20, 2025 at 01:32:43PM +0530, Nilay Shroff wrote:
>> During the failre
> 
> failure?
> 
>> To fix this, we now unconditionally release the nshead reference when
>> the last nvme path referencing to the nshead is removed, regardless of
>> the head’s state. Also during idnetify
> 
> identify
> 
> Otherwise this looks good.

Thanks! I'd update the above typo, however today I was testing some code 
after disabling CONFIG_NVME_MULTIPATH and I realized that although the
namespace head (ns->head) is still created, its associated head->disk is
not initialized in this configuration. As a result, ns->head only has a
single reference — the one from kref_init() during its creation. Therefore,
when we delete the namespace or handle an error case, we must ensure that
we only call nvme_put_ns_head() once to properly release the reference and
avoid a potential double-free. So I updated this patch as shown below (please
note that this is equally applicable when we insmod nvme_core with module
param "multipath" set to false):

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 92697f98c601..033fe0a12727 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4089,6 +4089,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
        struct nvme_ns *ns;
        struct gendisk *disk;
        int node = ctrl->numa_node;
+       bool last_path = false;
 
        ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
        if (!ns)
@@ -4181,9 +4182,14 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
  out_unlink_ns:
        mutex_lock(&ctrl->subsys->lock);
        list_del_rcu(&ns->siblings);
-       if (list_empty(&ns->head->list))
+       if (list_empty(&ns->head->list)) {
                list_del_init(&ns->head->entry);
+               if (ns->head->disk)
+                       last_path = true;
+       }
        mutex_unlock(&ctrl->subsys->lock);
+       if (last_path)
+               nvme_put_ns_head(ns->head);
        nvme_put_ns_head(ns->head);
  out_cleanup_disk:
        put_disk(disk);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e040e467f9fa..c7644631bb1a 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -690,8 +690,8 @@ static void nvme_remove_head(struct nvme_ns_head *head)
                nvme_cdev_del(&head->cdev, &head->cdev_device);
                synchronize_srcu(&head->srcu);
                del_gendisk(head->disk);
-               nvme_put_ns_head(head);
        }
+       nvme_put_ns_head(head);
 }
 
 static void nvme_remove_head_work(struct work_struct *work)
@@ -1291,6 +1291,9 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
        bool remove = false;
 
+       if (!head->disk)
+               return;
+
        mutex_lock(&head->subsys->lock);
        /*
         * We are called when all paths have been removed, and at that point

So I'll send out patchv2 with the above changes.

Thanks,
--Nilay