[PATCH] nvmet: take subsys->lock while force-reconnecting in nvmet_subsys_attr_qid_max_store()

[PATCH] nvmet: take subsys->lock while force-reconnecting in nvmet_subsys_attr_qid_max_store()

[Yu Zhang]

nvmet_subsys_attr_qid_max_store() sets subsys->max_qid and then walks
subsys->ctrls to force every controller to reconnect, but it holds only
nvmet_config_sem (for write).  subsys->ctrls is protected by subsys->lock:
nvmet_alloc_ctrl() adds an entry under subsys->lock, and nvmet_ctrl_free()
removes it under subsys->lock with list_del() + kfree() (not kfree_rcu).
The free path takes no nvmet_config_sem, so the two locks are independent.

A controller teardown (host disconnect, keep-alive timeout or fatal error
-> nvmet_sq_destroy() -> nvmet_ctrl_put() -> nvmet_ctrl_free()) can
therefore list_del() an entry while qid_max_store() is iterating the list,
corrupting the walk.

KASAN, concurrent "echo 128 > .../subsystems/<nqn>/attr_qid_max" racing
nvme-loop controllers connecting and disconnecting, panics in seconds:

  BUG: KASAN: null-ptr-deref in nvmet_subsys_attr_qid_max_store+0x10d/0x1b0
  Read of size 8 at addr 0000000000000090
  Call Trace:
   nvmet_subsys_attr_qid_max_store+0x10d/0x1b0
   configfs_write_iter+0x1a7/0x250
   vfs_write+0x5df/0x940
   ksys_write+0xfd/0x1e0

(also seen as a general-protection fault dereferencing a LIST_POISON
pointer at the same instruction.)

Take subsys->lock around the list_for_each_entry() loop, matching every
other subsys->ctrls accessor.  ->delete_ctrl() only queues asynchronous
controller teardown, so it does not re-enter subsys->lock under the loop.

Fixes: 3e980f5995e0 ("nvmet: expose max queues to configfs")
Cc: stable@vger.kernel.org
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
Tested-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/nvme/target/configfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 1111111..2222222 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1683,9 +1683,16 @@ static ssize_t nvmet_subsys_attr_qid_max_store(struct config_item *item,
 	down_write(&nvmet_config_sem);
 	subsys->max_qid = qid_max;

-	/* Force reconnect */
+	/*
+	 * Force reconnect.  subsys->ctrls is protected by subsys->lock
+	 * (nvmet_alloc_ctrl()/nvmet_ctrl_free() add and remove entries under
+	 * it); take it here too so a concurrent controller teardown cannot
+	 * list_del() an entry while this loop walks the list.
+	 */
+	mutex_lock(&subsys->lock);
 	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
 		ctrl->ops->delete_ctrl(ctrl);
+	mutex_unlock(&subsys->lock);
 	up_write(&nvmet_config_sem);

 	return cnt;
--
2.43.0

Re: [PATCH] nvmet: take subsys->lock while force-reconnecting in nvmet_subsys_attr_qid_max_store()

[Maurizio Lombardi]

On Wed Oct 14, 2026 at 7:47 AM CEST, Yu Zhang wrote:
> nvmet_subsys_attr_qid_max_store() sets subsys->max_qid and then walks
> subsys->ctrls to force every controller to reconnect, but it holds only
> nvmet_config_sem (for write).  subsys->ctrls is protected by subsys->lock:
> nvmet_alloc_ctrl() adds an entry under subsys->lock, and nvmet_ctrl_free()
> removes it under subsys->lock with list_del() + kfree() (not kfree_rcu).
> The free path takes no nvmet_config_sem, so the two locks are independent.
>
> A controller teardown (host disconnect, keep-alive timeout or fatal error
> -> nvmet_sq_destroy() -> nvmet_ctrl_put() -> nvmet_ctrl_free()) can
> therefore list_del() an entry while qid_max_store() is iterating the list,
> corrupting the walk.
>
> KASAN, concurrent "echo 128 > .../subsystems/<nqn>/attr_qid_max" racing
> nvme-loop controllers connecting and disconnecting, panics in seconds:
>
>   BUG: KASAN: null-ptr-deref in nvmet_subsys_attr_qid_max_store+0x10d/0x1b0
>   Read of size 8 at addr 0000000000000090
>   Call Trace:
>    nvmet_subsys_attr_qid_max_store+0x10d/0x1b0
>    configfs_write_iter+0x1a7/0x250
>    vfs_write+0x5df/0x940
>    ksys_write+0xfd/0x1e0
>
> (also seen as a general-protection fault dereferencing a LIST_POISON
> pointer at the same instruction.)
>
> Take subsys->lock around the list_for_each_entry() loop, matching every
> other subsys->ctrls accessor.  ->delete_ctrl() only queues asynchronous
> controller teardown, so it does not re-enter subsys->lock under the loop.
>
> Fixes: 3e980f5995e0 ("nvmet: expose max queues to configfs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>
> Tested-by: Yu Zhang <yuz08559@gmail.com>
> ---
>  drivers/nvme/target/configfs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 1111111..2222222 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1683,9 +1683,16 @@ static ssize_t nvmet_subsys_attr_qid_max_store(struct config_item *item,
>  	down_write(&nvmet_config_sem);
>  	subsys->max_qid = qid_max;
>
> -	/* Force reconnect */
> +	/*
> +	 * Force reconnect.  subsys->ctrls is protected by subsys->lock
> +	 * (nvmet_alloc_ctrl()/nvmet_ctrl_free() add and remove entries under
> +	 * it); take it here too so a concurrent controller teardown cannot
> +	 * list_del() an entry while this loop walks the list.
> +	 */
> +	mutex_lock(&subsys->lock);
>  	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
>  		ctrl->ops->delete_ctrl(ctrl);
> +	mutex_unlock(&subsys->lock);
>  	up_write(&nvmet_config_sem);
>
>  	return cnt;
> --
> 2.43.0

I don't think we need a comment in the source code just to explain that.
It's an established pattern all over the target driver code that accesses
to the subsys->ctrls list are protected by subsys->lock, so this is already implied.

Anyway the fix looks good to me.

Reviewed-by: Maurizio Lombardi <mlombard@redhat.com>