Re: [PATCH 4/6] nvmet: take subsys->lock in nvmet_subsys_attr_tbkas_store()

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

On 11/6/26 7:28 AM, Guixin Liu wrote:
> The tbkas store handler 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(). A concurrent controller
> teardown can therefore corrupt the walk.
> 
> This is the same bug fixed for nvmet_subsys_attr_qid_max_store() in
> commit 9f426f140511 ("nvmet: take subsys->lock while
> force-reconnecting in nvmet_subsys_attr_qid_max_store()"), which
> missed this second instance of the pattern. Take subsys->lock around
> the list_for_each_entry() loop. ->delete_ctrl() only queues
> asynchronous controller teardown, so it does not re-enter subsys->lock
> under the loop.
> 
> Fixes: e98e45e9465b ("nvmet: Add traffic based keep alive support to configfs")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/target/configfs.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index b3a5242d06f9..c17f2c8ab2df 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1466,8 +1466,10 @@ static ssize_t nvmet_subsys_attr_tbkas_store(struct config_item *item,
>   	subsys->tbkas = tbkas;
>   
>   	/* Force reconnect */
> +	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 count;

Ah, I also came across the same issue while working on my Clang context
annotation series for NVMe target, which is still in my local queue and
that I'm planning to send upstream soon.
Glad you spotted it as well. I'll drop this fix from my local queue.

Looks good to me:
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>