nvmet: Fix possible infinite loop triggered on hot namespace removal
Subsystems (3)
Sagi Grimberg
maintainerPATCH v2 1/1 nvmet: Fix possible infinite loop triggered on hot namespace removal
From: Solganik Alexander <[email protected]>
When removing a namespace we delete it from the subsystem namespaces list with list_del_init which allows us to know if it is enabled or not.
The problem is that list_del_init initialize the list next and does not respect the RCU list-traversal we do on the IO path for locating a namespace. Instead we need to use list_del_rcu which is allowed to run concurrently with the _rcu list-traversal primitives (keeps list next intact) and guarantees concurrent nvmet_find_naespace forward progress.
By changing that, we cannot rely on ns->dev_link for knowing if the namspace is enabled, so add enabled indicator entry to nvmet_ns for that.
---
Changes from v1:
- Changed enabled from atomic bit to bool and updated it under the subsys lock in order to protect against enable/disable running concurrently
- Fixed nvmet_ns_enabled display
drivers/nvme/target/core.c | 15 +++++++++------ drivers/nvme/target/nvmet.h | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-)
drivers/nvme/target/core.c +9 -6
@@ -264,9 +264,11 @@ int nvmet_ns_enable(struct nvmet_ns *ns)int ret =0 ;mutex_lock (&subsys->lock); -if (!list_empty (&ns->dev_link)) +if (ns->enabled) goto out_unlock; + ns->enabled = true; + ns->bdev =blkdev_get_by_path (ns->device_path,FMODE_READ |FMODE_WRITE ,NULL );if (IS_ERR (ns->bdev)) {
@@ -325,11 +327,11 @@ void nvmet_ns_disable(struct nvmet_ns *ns)struct nvmet_ctrl *ctrl;mutex_lock (&subsys->lock); -if (list_empty (&ns->dev_link)) { -mutex_unlock (&subsys->lock); -return ; - } -list_del_init (&ns->dev_link); +if (!ns->enabled) + goto out_unlock; + + ns->enabled = false; +list_del_rcu (&ns->dev_link);mutex_unlock (&subsys->lock);/*
@@ -351,6 +353,7 @@ void nvmet_ns_disable(struct nvmet_ns *ns)if (ns->bdev)blkdev_put (ns->bdev,FMODE_WRITE |FMODE_READ ); +out_unlock:mutex_unlock (&subsys->lock); }
drivers/nvme/target/nvmet.h +2 -1
@@ -47,6 +47,7 @@ struct nvmet_ns {loff_t size;u8 nguid[16 ]; +bool enabled;struct nvmet_subsys *subsys;const char *device_path;
@@ -63,7 +64,7 @@ static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)static inline bool nvmet_ns_enabled (struct nvmet_ns *ns) { -return !list_empty_careful (&ns->dev_link); +return ns->enabled; }struct nvmet_cq {
Signature · 1 line
2.7.4
Christoph Hellwig
maintainerPATCH v2 1/1 nvmet: Fix possible infinite loop triggered on hot namespace removal
Quoted 38 lines · Sagi Grimberg
From: Solganik Alexander <sashas at lightbitslabs.com>
When removing a namespace we delete it from the subsystem namespaces list with
list_del_initwhich allows us to know if it is enabled or not.The problem is that
list_del_initinitialize the list next and does not respect the RCU list-traversal we do on the IO path for locating a namespace. Instead we need to uselist_del_rcuwhich is allowed to run concurrently with the_rculist-traversal primitives (keeps list next intact) and guarantees concurrentnvmet_find_naespaceforward progress.By changing that, we cannot rely on
ns->dev_linkfor knowing if the namspace is enabled, so add enabled indicator entry tonvmet_nsfor that.Signed-off-by Sagi Grimberg <sagi at grimberg.me>Signed-off-by Solganik Alexander <sashas at lightbitslabs.com>Cc <stable at vger.kernel.org> # v4.8+---
Changes from v1:
- Changed enabled from atomic bit to bool and updated it under the subsys lock in order to protect against enable/disable running concurrently
- Fixednvmet_ns_enableddisplaydrivers/nvme/target/core.c | 15 +++++++++------ drivers/nvme/target/nvmet.h | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-)drivers/nvme/target/core.c +0
-0@@ -264,9 +264,11 @@ int nvmet_ns_enable(struct nvmet_ns *ns)int ret =0 ;
drivers/nvme/target/core.c int nvmet_ns_enable(struct nvmet_ns *ns) · line 274
Quoted 10 lines
drivers/nvme/target/core.c +3
-1mutex_lock (&subsys->lock); -if (!list_empty (&ns->dev_link)) +if (ns->enabled) goto out_unlock; + ns->enabled = true; + ns->bdev =blkdev_get_by_path (ns->device_path,FMODE_READ |FMODE_WRITE ,NULL );if (IS_ERR (ns->bdev)) {
This will leave the enable flag set when an error happen? later, won't it? I'd set it just before dropping the lock.
drivers/nvme/target/core.c
Quoted 4 lines
static inline bool
nvmet_ns_enabled(struct nvmet_ns *ns){drivers/nvme/target/core.c +1
-1-return !list_empty_careful (&ns->dev_link); +return ns->enabled;
and we can probably kill this helper, it's pretty pointless now.
Sagi Grimberg
maintainerPATCH v2 1/1 nvmet: Fix possible infinite loop triggered on hot namespace removal
Quoted 13 lines
Quoted 13 lines
drivers/nvme/target/core.c +1
-1@@ -264,9 +264,11 @@ int nvmet_ns_enable(struct nvmet_ns *ns)int ret =0 ;mutex_lock (&subsys->lock); -if (!list_empty (&ns->dev_link)) +if (ns->enabled) goto out_unlock;
Quoted 8 lines
drivers/nvme/target/core.c int nvmet_ns_enable(struct nvmet_ns *ns) · line 274
Quoted 5 lines
drivers/nvme/target/core.c +2
-0+ ns->enabled = true; + ns->bdev =blkdev_get_by_path (ns->device_path,FMODE_READ |FMODE_WRITE ,NULL );if (IS_ERR (ns->bdev)) {This will leave the enable flag set when an error happen? later, won't it? I'd set it just before dropping the lock.
Yep, will do.
Quoted 8 lines
drivers/nvme/target/core.c
Quoted 5 lines
static inline bool
nvmet_ns_enabled(struct nvmet_ns *ns){drivers/nvme/target/core.c +1
-1-return !list_empty_careful (&ns->dev_link); +return ns->enabled;and we can probably kill this helper, it's pretty pointless now.
I will.