[PATCH 0/6] nvme: fabrics and target fixes

[PATCH 0/6] nvme: fabrics and target fixes

[Guixin Liu]

Six fixes found by code inspection of the nvme host and target drivers:

 - a reconnect_delay of 0 is accepted through sysfs and makes the
   ctrl_loss_tmo store divide by zero; reproduced on an unfixed
   kernel, rejected on the patched one
 - list_for_each_entry_safe() in nvme_fc_match_disconn_ls() cannot
   tolerate the rport lock drop inside the loop: the cached next
   entry may be freed in the window, so the loop switches back to a
   plain iterator, which is safe because the reference taken on the
   current entry pins it on the list; reproduced under KASAN through
   fcloop with an amplified lock-drop window
 - a data race on ctrl->transaction between concurrent per-queue
   DH-HMAC-CHAP exchanges
 - nvmet_subsys_attr_tbkas_store() walks subsys->ctrls without
   subsys->lock, the same bug fixed for qid_max_store() in patch 4
   of this series
 - nvmet_install_queue() leaks a controller reference and permanently
   wedges the queue on its error paths
 - needless GFP_ATOMIC allocations in the nvmet persistent reservation
   code

All fixes are verified on an initiator/target VM pair running the
patched kernel: 30 connect/IO/disconnect cycles, tbkas toggles racing
connection churn, persistent reservation operations across all six
reservation types, and a full DH-HMAC-CHAP authenticated session with
I/O.  The nvme-fc fix is additionally exercised through fcloop with
three controllers on one remote port, where the modified
nvme_fc_match_disconn_ls() path ran on every host- and target-initiated
association teardown, including 15 tbkas force-reconnect cycles that
send Disconnect Association LSs into the multi-entry list walk.

Guixin Liu (6):
  nvme: reject a reconnect_delay of 0 through sysfs
  nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()
  nvme-auth: fix data race on ctrl->transaction
  nvmet: take subsys->lock in nvmet_subsys_attr_tbkas_store()
  nvmet: fix controller reference leak in nvmet_install_queue() error paths
  nvmet: use GFP_KERNEL for PR allocations

 drivers/nvme/host/auth.c          | 2 +-
 drivers/nvme/host/fc.c            | 4 ++--
 drivers/nvme/host/nvme.h          | 2 +-
 drivers/nvme/host/sysfs.c         | 3 +++
 drivers/nvme/target/configfs.c    | 2 ++
 drivers/nvme/target/fabrics-cmd.c | 5 +++++
 drivers/nvme/target/pr.c          | 4 ++--
 7 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.43.7

[PATCH 1/6] nvme: reject a reconnect_delay of 0 through sysfs

[Guixin Liu]

nvme_ctrl_reconnect_delay_store() accepts any value the option parser
also accepts, except that it misses the lower bound: writing 0 is
rejected by nvmf_parse_options() but not by the sysfs attribute.
nvme_ctrl_loss_tmo_store() then divides by opts->reconnect_delay in
DIV_ROUND_UP(), so writing 0 followed by any ctrl_loss_tmo update
crashes the kernel with a division by zero.

Reject 0 like the option parser does.

Fixes: 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
Cc: stable@vger.kernel.org
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---

Reproduced on an unfixed 7.3.0-rc7 kernel in a VM with a fabrics
controller connected:

  echo 0 > /sys/class/nvme/nvme1/reconnect_delay
  echo 30 > /sys/class/nvme/nvme1/ctrl_loss_tmo

  Oops: divide error: 0000 [#1] SMP KASAN PTI
  CPU: 7 UID: 0 PID: 1542 Comm: bash Not tainted 7.3.0-rc7
  RIP: 0010:nvme_ctrl_loss_tmo_store+0xce/0x120 [nvme_core]
  Code: ... 8d 44 08 ff <f7> f1 ...
  RAX: 000000000000001d RCX: 0000000000000000

Verified on the patched kernel: writing 0 returns -EINVAL and the
ctrl_loss_tmo update succeeds without an oops.
---
 drivers/nvme/host/sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 10018c6356b6..bae0d6258d01 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -634,6 +634,9 @@ static ssize_t nvme_ctrl_reconnect_delay_store(struct device *dev,
 	if (err)
 		return err;
 
+	if (!v)
+		return -EINVAL;
+
 	ctrl->opts->reconnect_delay = v;
 	return count;
 }
-- 
2.43.7

Re: [PATCH 1/6] nvme: reject a reconnect_delay of 0 through sysfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] nvme: incorrect Fixes tag for nvme_ctrl_reconnect_delay_store() fix
--

commit b125e1489e83edb63c16621248dd345277c89cc1
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme: reject a reconnect_delay of 0 through sysfs

This commit updates nvme_ctrl_reconnect_delay_store() in
drivers/nvme/host/sysfs.c to reject a reconnect_delay value of 0,
preventing a division by zero in nvme_ctrl_loss_tmo_store().

>     nvme: reject a reconnect_delay of 0 through sysfs
>     
>     nvme_ctrl_reconnect_delay_store() accepts any value the option parser
>     also accepts, except that it misses the lower bound: writing 0 is
>     rejected by nvmf_parse_options() but not by the sysfs attribute.
>     nvme_ctrl_loss_tmo_store() then divides by opts->reconnect_delay in
>     DIV_ROUND_UP(), so writing 0 followed by any ctrl_loss_tmo update
>     crashes the kernel with a division by zero.
>     
>     Reject 0 like the option parser does.
>     
>     Fixes: 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

[Severity: Low]
Commit 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
only added ctrl_loss_tmo option parsing in nvmf_parse_options() in
drivers/nvme/host/fabrics.c, where a reconnect_delay <= 0 was already
rejected.

The sysfs store handlers nvme_ctrl_reconnect_delay_store() and
nvme_ctrl_loss_tmo_store() (later moved to drivers/nvme/host/sysfs.c in
commit 942e21c042e6) were introduced in commit 764075fdcb2f ("nvme: expose
reconnect_delay and ctrl_loss_tmo via sysfs").

Should the Fixes tag reference that commit instead:

  Fixes: 764075fdcb2f ("nvme: expose reconnect_delay and ctrl_loss_tmo via sysfs")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106015842.3534898-1-kanie@linux.alibaba.com?part=1

Re: [PATCH 1/6] nvme: reject a reconnect_delay of 0 through sysfs

[Hannes Reinecke]

On 11/6/26 2:58 AM, Guixin Liu wrote:
> nvme_ctrl_reconnect_delay_store() accepts any value the option parser
> also accepts, except that it misses the lower bound: writing 0 is
> rejected by nvmf_parse_options() but not by the sysfs attribute.
> nvme_ctrl_loss_tmo_store() then divides by opts->reconnect_delay in
> DIV_ROUND_UP(), so writing 0 followed by any ctrl_loss_tmo update
> crashes the kernel with a division by zero.
> 
> Reject 0 like the option parser does.
> 
> Fixes: 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
> 
> Reproduced on an unfixed 7.3.0-rc7 kernel in a VM with a fabrics
> controller connected:
> 
>    echo 0 > /sys/class/nvme/nvme1/reconnect_delay
>    echo 30 > /sys/class/nvme/nvme1/ctrl_loss_tmo
> 
>    Oops: divide error: 0000 [#1] SMP KASAN PTI
>    CPU: 7 UID: 0 PID: 1542 Comm: bash Not tainted 7.3.0-rc7
>    RIP: 0010:nvme_ctrl_loss_tmo_store+0xce/0x120 [nvme_core]
>    Code: ... 8d 44 08 ff <f7> f1 ...
>    RAX: 000000000000001d RCX: 0000000000000000
> 
> Verified on the patched kernel: writing 0 returns -EINVAL and the
> ctrl_loss_tmo update succeeds without an oops.
> ---
>   drivers/nvme/host/sysfs.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 10018c6356b6..bae0d6258d01 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -634,6 +634,9 @@ static ssize_t nvme_ctrl_reconnect_delay_store(struct device *dev,
>   	if (err)
>   		return err;
>   
> +	if (!v)
> +		return -EINVAL;
> +
>   	ctrl->opts->reconnect_delay = v;
>   	return count;
>   }

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 1/6] nvme: reject a reconnect_delay of 0 through sysfs

[Christoph Hellwig]

On Fri, Nov 06, 2026 at 09:58:37AM +0800, Guixin Liu wrote:
> nvme_ctrl_reconnect_delay_store() accepts any value the option parser
> also accepts, except that it misses the lower bound: writing 0 is
> rejected by nvmf_parse_options() but not by the sysfs attribute.
> nvme_ctrl_loss_tmo_store() then divides by opts->reconnect_delay in
> DIV_ROUND_UP(), so writing 0 followed by any ctrl_loss_tmo update
> crashes the kernel with a division by zero.
> 
> Reject 0 like the option parser does.
> 
> Fixes: 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

I think the Sashiko nit about the fixes tag is correct.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 1/6] nvme: reject a reconnect_delay of 0 through sysfs

[Nilay Shroff]

On 11/6/26 7:28 AM, Guixin Liu wrote:
> nvme_ctrl_reconnect_delay_store() accepts any value the option parser
> also accepts, except that it misses the lower bound: writing 0 is
> rejected by nvmf_parse_options() but not by the sysfs attribute.
> nvme_ctrl_loss_tmo_store() then divides by opts->reconnect_delay in
> DIV_ROUND_UP(), so writing 0 followed by any ctrl_loss_tmo update
> crashes the kernel with a division by zero.
> 
> Reject 0 like the option parser does.
> 
> Fixes: 42a45274c2258 ("nvme-fabrics: Allow ctrl loss timeout configuration")
> Cc:stable@vger.kernel.org
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>

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

[PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[Guixin Liu]

Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
switched the loop to list_for_each_entry_safe() to allow dropping the
rport lock for the nvme_fc_ctrl_put() of a non-matching controller.
But the safe iterator only protects the current entry within one
iteration: the next entry is cached before the body runs, and while
the lock is dropped that entry can run through its own final put into
nvme_fc_ctrl_free(), which removes it from the list and frees it.
The next iteration then dereferences freed memory.

The safe variant is not needed here at all. The reference the loop
takes on the current entry pins it on the list until the iteration
moves on, so a plain list_for_each_entry() tolerates the lock drop:
while the lock is held the current entry cannot be unlinked, and once
the lock is dropped the cached ->next of the pinned entry still points
at a valid list member or the head.

Switch back to the plain iterator.

Fixes: b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
Cc: stable@vger.kernel.org
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

---

Reproduced under KASAN in a VM through fcloop with three or more
controllers on one remote port, a tbkas force-reconnect racing
concurrent controller disconnects.  The lock-drop window was widened
with a test-only mdelay(200) inserted at the same place in both the
buggy and the fixed version so the iterator is the only variable; the
stock microsecond window did not trigger in 400 iterations of the
same workload.

  BUG: KASAN: slab-use-after-free in nvme_fc_match_disconn_ls
  Read of size 8 by task kworker
  Workqueue: events nvme_fc_handle_ls_rqst_work
  Call Trace:
   nvme_fc_match_disconn_ls
   nvme_fc_ls_disconnect_assoc
   nvme_fc_handle_ls_rqst_work
   process_one_work

The object was allocated by nvme_fc_alloc_ctrl() on connect and freed
by nvme_free_ctrl() on the concurrent teardown of the cached next
entry.  With the plain iterator the identical amplified sequence
reports nothing and survives; the read hits the pinned current entry,
which cannot be freed while the loop holds its reference.
---
 drivers/nvme/host/fc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0b9ff50d26ff..edf192055618 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
 {
 	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
 					&lsop->rqstbuf->rq_dis_assoc;
-	struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
+	struct nvme_fc_ctrl *ctrl, *ret = NULL;
 	struct nvmefc_ls_rcv_op *oldls = NULL;
 	u64 association_id = be64_to_cpu(rqst->associd.association_id);
 	unsigned long flags;
 
 	spin_lock_irqsave(&rport->lock, flags);
 
-	list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
 		if (!nvme_fc_ctrl_get(ctrl))
 			continue;
 		spin_lock(&ctrl->lock);
-- 
2.43.7

Re: [PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] nvme-fc: use-after-free in nvme_fc_match_disconn_ls() after nvme_fc_ctrl_put()
--

commit 8782ae8cb9a0539907c2a9d660e71e6ada06052f
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

This commit switches the rport->ctrl_list traversal in
nvme_fc_match_disconn_ls() from list_for_each_entry_safe() to
list_for_each_entry() to avoid caching a next entry that could be freed while
rport->lock is dropped.

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0b9ff50d26ff3..edf192055618c 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>  {
>  	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>  					&lsop->rqstbuf->rq_dis_assoc;
> -	struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
> +	struct nvme_fc_ctrl *ctrl, *ret = NULL;
>  	struct nvmefc_ls_rcv_op *oldls = NULL;
>  	u64 association_id = be64_to_cpu(rqst->associd.association_id);
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&rport->lock, flags);
>  
> -	list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
> +	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
>  		if (!nvme_fc_ctrl_get(ctrl))
>  			continue;
>  		spin_lock(&ctrl->lock);

[Severity: Critical]
Does switching to list_for_each_entry() in nvme_fc_match_disconn_ls() in
drivers/nvme/host/fc.c still leave a use-after-free on ctrl itself when
nvme_fc_handle_ls_rqst_work() -> nvme_fc_ls_disconnect_assoc() ->
nvme_fc_match_disconn_ls() iterates past a non-matching controller that is
concurrently being torn down?

At the end of each non-matching iteration in nvme_fc_match_disconn_ls(), the
loop drops rport->lock and releases the reference on ctrl before the loop step
runs:

	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
		if (!nvme_fc_ctrl_get(ctrl))
			continue;
		...
		if (ret)
			/* leave the ctrl get reference */
			break;
		spin_unlock_irqrestore(&rport->lock, flags);
		nvme_fc_ctrl_put(ctrl);
		spin_lock_irqsave(&rport->lock, flags);
	}

Because list_for_each_entry() evaluates ctrl = list_next_entry(ctrl,
ctrl_list) after nvme_fc_ctrl_put(ctrl) and after rport->lock was dropped and
re-acquired, ctrl is no longer pinned when ctrl->ctrl_list.next is read. If
nvme_fc_ctrl_put(ctrl) (or a concurrent nvme_fc_free_ctrl() while rport->lock
is dropped) drops ctrl->ref to zero, nvme_fc_ctrl_free() unlinks and frees
ctrl:

	spin_lock_irqsave(&ctrl->rport->lock, flags);
	owns_opts = !list_empty(&ctrl->ctrl_list);
	list_del(&ctrl->ctrl_list);
	spin_unlock_irqrestore(&ctrl->rport->lock, flags);
	...
	kfree(ctrl);

When the loop step then reads ctrl->ctrl_list.next, it reads from the freed
ctrl struct and follows LIST_POISON1 into the next iteration (or loops
forever if a concurrent nvme_fc_init_ctrl() error path ran
list_del_init(&ctrl->ctrl_list)).

Since rport->lock already serializes against list_del() in
nvme_fc_ctrl_free() and list_del_init() in nvme_fc_init_ctrl(), could
nvme_fc_match_disconn_ls() avoid dropping rport->lock in the loop altogether
by checking association_id == ctrl->association_id first and only calling
nvme_fc_ctrl_get(ctrl) when a matching controller is found?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106015842.3534898-1-kanie@linux.alibaba.com?part=2

Re: [PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[Hannes Reinecke]

On 11/6/26 2:58 AM, Guixin Liu wrote:
> Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
> switched the loop to list_for_each_entry_safe() to allow dropping the
> rport lock for the nvme_fc_ctrl_put() of a non-matching controller.
> But the safe iterator only protects the current entry within one
> iteration: the next entry is cached before the body runs, and while
> the lock is dropped that entry can run through its own final put into
> nvme_fc_ctrl_free(), which removes it from the list and frees it.
> The next iteration then dereferences freed memory.
> 
> The safe variant is not needed here at all. The reference the loop
> takes on the current entry pins it on the list until the iteration
> moves on, so a plain list_for_each_entry() tolerates the lock drop:
> while the lock is held the current entry cannot be unlinked, and once
> the lock is dropped the cached ->next of the pinned entry still points
> at a valid list member or the head.
> 
> Switch back to the plain iterator.
> 
> Fixes: b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> 
> ---
> 
> Reproduced under KASAN in a VM through fcloop with three or more
> controllers on one remote port, a tbkas force-reconnect racing
> concurrent controller disconnects.  The lock-drop window was widened
> with a test-only mdelay(200) inserted at the same place in both the
> buggy and the fixed version so the iterator is the only variable; the
> stock microsecond window did not trigger in 400 iterations of the
> same workload.
> 
>    BUG: KASAN: slab-use-after-free in nvme_fc_match_disconn_ls
>    Read of size 8 by task kworker
>    Workqueue: events nvme_fc_handle_ls_rqst_work
>    Call Trace:
>     nvme_fc_match_disconn_ls
>     nvme_fc_ls_disconnect_assoc
>     nvme_fc_handle_ls_rqst_work
>     process_one_work
> 
> The object was allocated by nvme_fc_alloc_ctrl() on connect and freed
> by nvme_free_ctrl() on the concurrent teardown of the cached next
> entry.  With the plain iterator the identical amplified sequence
> reports nothing and survives; the read hits the pinned current entry,
> which cannot be freed while the loop holds its reference.
> ---
>   drivers/nvme/host/fc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0b9ff50d26ff..edf192055618 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>   {
>   	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>   					&lsop->rqstbuf->rq_dis_assoc;
> -	struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
> +	struct nvme_fc_ctrl *ctrl, *ret = NULL;
>   	struct nvmefc_ls_rcv_op *oldls = NULL;
>   	u64 association_id = be64_to_cpu(rqst->associd.association_id);
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&rport->lock, flags);
>   
> -	list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
> +	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
>   		if (!nvme_fc_ctrl_get(ctrl))
>   			continue;
>   		spin_lock(&ctrl->lock);

Not sure here. The implication of the function is that the any 
controller might've been deleted during each loop iteration
(disconnect might be triggered from both sides, either host
_or_ controller).
So while it's true that a controller might not get deleted while
a reference is held, that other process might drop a reference
to the controller while the first process is executing the loop.
Then the call to 'nvme_fc_ctrl_put()' at the end of the loop
will be the last reference, the controller gets deleted,
and the ->next element becomes invalid causing the loop
to crash.

So, no.

And the issue you've described should've caused the
nvme_fc_ctrl_get() at the start of the loop to return
'false'; why didn't it trigger here?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/6] nvme-fc: fix iterator over rport->ctrl_list in nvme_fc_match_disconn_ls()

[Guixin Liu]

在 2026/11/6 15:38, Hannes Reinecke 写道:
> On 11/6/26 2:58 AM, Guixin Liu wrote:
>> Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
>> switched the loop to list_for_each_entry_safe() to allow dropping the
>> rport lock for the nvme_fc_ctrl_put() of a non-matching controller.
>> But the safe iterator only protects the current entry within one
>> iteration: the next entry is cached before the body runs, and while
>> the lock is dropped that entry can run through its own final put into
>> nvme_fc_ctrl_free(), which removes it from the list and frees it.
>> The next iteration then dereferences freed memory.
>>
>> The safe variant is not needed here at all. The reference the loop
>> takes on the current entry pins it on the list until the iteration
>> moves on, so a plain list_for_each_entry() tolerates the lock drop:
>> while the lock is held the current entry cannot be unlinked, and once
>> the lock is dropped the cached ->next of the pinned entry still points
>> at a valid list member or the head.
>>
>> Switch back to the plain iterator.
>>
>> Fixes: b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>>
>> ---
>>
>> Reproduced under KASAN in a VM through fcloop with three or more
>> controllers on one remote port, a tbkas force-reconnect racing
>> concurrent controller disconnects.  The lock-drop window was widened
>> with a test-only mdelay(200) inserted at the same place in both the
>> buggy and the fixed version so the iterator is the only variable; the
>> stock microsecond window did not trigger in 400 iterations of the
>> same workload.
>>
>>    BUG: KASAN: slab-use-after-free in nvme_fc_match_disconn_ls
>>    Read of size 8 by task kworker
>>    Workqueue: events nvme_fc_handle_ls_rqst_work
>>    Call Trace:
>>     nvme_fc_match_disconn_ls
>>     nvme_fc_ls_disconnect_assoc
>>     nvme_fc_handle_ls_rqst_work
>>     process_one_work
>>
>> The object was allocated by nvme_fc_alloc_ctrl() on connect and freed
>> by nvme_free_ctrl() on the concurrent teardown of the cached next
>> entry.  With the plain iterator the identical amplified sequence
>> reports nothing and survives; the read hits the pinned current entry,
>> which cannot be freed while the loop holds its reference.
>> ---
>>   drivers/nvme/host/fc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
>> index 0b9ff50d26ff..edf192055618 100644
>> --- a/drivers/nvme/host/fc.c
>> +++ b/drivers/nvme/host/fc.c
>> @@ -1470,14 +1470,14 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>>   {
>>       struct fcnvme_ls_disconnect_assoc_rqst *rqst =
>>                       &lsop->rqstbuf->rq_dis_assoc;
>> -    struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
>> +    struct nvme_fc_ctrl *ctrl, *ret = NULL;
>>       struct nvmefc_ls_rcv_op *oldls = NULL;
>>       u64 association_id = be64_to_cpu(rqst->associd.association_id);
>>       unsigned long flags;
>>         spin_lock_irqsave(&rport->lock, flags);
>>   -    list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
>> +    list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
>>           if (!nvme_fc_ctrl_get(ctrl))
>>               continue;
>>           spin_lock(&ctrl->lock);
>
> Not sure here. The implication of the function is that the any controller might've been deleted during each loop iteration
> (disconnect might be triggered from both sides, either host
> _or_ controller).
> So while it's true that a controller might not get deleted while
> a reference is held, that other process might drop a reference
> to the controller while the first process is executing the loop.
> Then the call to 'nvme_fc_ctrl_put()' at the end of the loop
> will be the last reference, the controller gets deleted,
> and the ->next element becomes invalid causing the loop
> to crash.
You are right, Hannes, the issue still exists.
>
> So, no.
>
> And the issue you've described should've caused the
> nvme_fc_ctrl_get() at the start of the loop to return
> 'false'; why didn't it trigger here?
The UAF happens earlier, inside the "for" loop,
list_for_each_entry_safe() expands to "pos = n, n = list_next_entry(n)",
so at that point it already dereferences the next pointer of that ctrl
that has been freed. That why KASAN reports "Read of size 8 by task kworker"
in nvme_fc_match_disconn_ls().

By the time execution reaches nvmet_fc_ctrl_get(), the memory state is
event less clear.

I will change a v2 fix, thanks.

Best Regards,
Guixin Liu
>  
>
> Cheers,
>
> Hannes

[PATCH 3/6] nvme-auth: fix data race on ctrl->transaction

[Guixin Liu]

nvme_ctrl_auth_work() queues the DH-HMAC-CHAP exchange of every queue
on nvme-auth-wq without waiting for them, so multiple
nvme_queue_auth_work() instances run concurrently and all increment
ctrl->transaction in nvme_queue_auth_work(). The plain increment is a
data race, and the lost updates hand the same transaction id to
concurrent queues.

The controller side only echoes the transaction id within a queue, so
the race has no functional impact today, but the increment is still
unsynchronized and KCSAN reports it. Use an atomic_t.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/host/auth.c | 2 +-
 drivers/nvme/host/nvme.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 85f005a5efd0..107355ee9ef1 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -765,7 +765,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		return;
 	}
 
-	chap->transaction = ctrl->transaction++;
+	chap->transaction = (u16)atomic_fetch_inc(&ctrl->transaction);
 
 	/* DH-HMAC-CHAP Step 1: send negotiate */
 	dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 92c048002ecd..1fb676dade12 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -446,7 +446,7 @@ struct nvme_ctrl {
 	struct nvme_dhchap_queue_context *dhchap_ctxs;
 	struct nvme_dhchap_key *host_key;
 	struct nvme_dhchap_key *ctrl_key;
-	u16 transaction;
+	atomic_t transaction;
 #endif
 	key_serial_t tls_pskid;
 
-- 
2.43.7

Re: [PATCH 3/6] nvme-auth: fix data race on ctrl->transaction

[Hannes Reinecke]

On 11/6/26 2:58 AM, Guixin Liu wrote:
> nvme_ctrl_auth_work() queues the DH-HMAC-CHAP exchange of every queue
> on nvme-auth-wq without waiting for them, so multiple
> nvme_queue_auth_work() instances run concurrently and all increment
> ctrl->transaction in nvme_queue_auth_work(). The plain increment is a
> data race, and the lost updates hand the same transaction id to
> concurrent queues.
> 
> The controller side only echoes the transaction id within a queue, so
> the race has no functional impact today, but the increment is still
> unsynchronized and KCSAN reports it. Use an atomic_t.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/host/auth.c | 2 +-
>   drivers/nvme/host/nvme.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 85f005a5efd0..107355ee9ef1 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -765,7 +765,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
>   		return;
>   	}
>   
> -	chap->transaction = ctrl->transaction++;
> +	chap->transaction = (u16)atomic_fetch_inc(&ctrl->transaction);
>   
>   	/* DH-HMAC-CHAP Step 1: send negotiate */
>   	dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 92c048002ecd..1fb676dade12 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -446,7 +446,7 @@ struct nvme_ctrl {
>   	struct nvme_dhchap_queue_context *dhchap_ctxs;
>   	struct nvme_dhchap_key *host_key;
>   	struct nvme_dhchap_key *ctrl_key;
> -	u16 transaction;
> +	atomic_t transaction;
>   #endif
>   	key_serial_t tls_pskid;
>   
Hi-speed authentication races ... every day a new surprise.
Although I wonder why a normal 'atomic_inc_return' isn't sufficient.

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/6] nvme-auth: fix data race on ctrl->transaction

[Guixin Liu]

在 2026/11/6 15:27, Hannes Reinecke 写道:
> On 11/6/26 2:58 AM, Guixin Liu wrote:
>> nvme_ctrl_auth_work() queues the DH-HMAC-CHAP exchange of every queue
>> on nvme-auth-wq without waiting for them, so multiple
>> nvme_queue_auth_work() instances run concurrently and all increment
>> ctrl->transaction in nvme_queue_auth_work(). The plain increment is a
>> data race, and the lost updates hand the same transaction id to
>> concurrent queues.
>>
>> The controller side only echoes the transaction id within a queue, so
>> the race has no functional impact today, but the increment is still
>> unsynchronized and KCSAN reports it. Use an atomic_t.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/nvme/host/auth.c | 2 +-
>>   drivers/nvme/host/nvme.h | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index 85f005a5efd0..107355ee9ef1 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -765,7 +765,7 @@ static void nvme_queue_auth_work(struct work_struct *work)
>>           return;
>>       }
>>   -    chap->transaction = ctrl->transaction++;
>> +    chap->transaction = (u16)atomic_fetch_inc(&ctrl->transaction);
>>         /* DH-HMAC-CHAP Step 1: send negotiate */
>>       dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index 92c048002ecd..1fb676dade12 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -446,7 +446,7 @@ struct nvme_ctrl {
>>       struct nvme_dhchap_queue_context *dhchap_ctxs;
>>       struct nvme_dhchap_key *host_key;
>>       struct nvme_dhchap_key *ctrl_key;
>> -    u16 transaction;
>> +    atomic_t transaction;
>>   #endif
>>       key_serial_t tls_pskid;
>>   
> Hi-speed authentication races ... every day a new surprise.
> Although I wonder why a normal 'atomic_inc_return' isn't sufficient.
Sure, changed in v2.

Best Regards,
Guixin Liu
>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
>
> Cheers,
>
> Hannes

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

[Guixin Liu]

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;
-- 
2.43.7

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

[Hannes Reinecke]

On 11/6/26 2:58 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;

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Christoph Hellwig]

On Fri, Nov 06, 2026 at 09:58:40AM +0800, 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")

I'm pretty sure I saw this fly by already in the last days.  If
that wasn't you we might already have something like this in the
queue.

If not:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Nilay Shroff]

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>

[PATCH 5/6] nvmet: fix controller reference leak in nvmet_install_queue() error paths

[Guixin Liu]

nvmet_install_queue() takes a controller reference for req->cq->ctrl
and installs the controller in both req->sq->ctrl and req->cq->ctrl,
but neither error path undoes its work: the busy return after the
failed cq->ctrl cmpxchg leaves sq->ctrl set and the reference taken,
and the err path for a failed transport ->install_queue() clears
sq->ctrl but leaves cq->ctrl set with the reference still held.

The queue is then permanently wedged: a connect retry finds the stale
cq->ctrl, takes another reference and fails again, and the leaked
references keep the controller alive forever. Clearing cq->ctrl also
matters because the caller drops its own reference, which can be the
last one on the admin connect path, leaving cq->ctrl dangling.

Roll back the sq->ctrl assignment and drop the reference on both
error paths, and also clear ctrl->cqs[qid] on the err path to mirror
what nvmet_cq_destroy() would do.

Fixes: bb78836b3a7ca ("nvmet: fabrics: add CQ init and destroy")
Cc: stable@vger.kernel.org
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/fabrics-cmd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index fc626c2fd2c9..773dad857a35 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -216,6 +216,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 	if (old) {
 		pr_warn("queue already connected!\n");
 		req->error_loc = offsetof(struct nvmf_connect_command, opcode);
+		req->sq->ctrl = NULL;
+		nvmet_ctrl_put(ctrl);
 		return NVME_SC_CONNECT_CTRL_BUSY | NVME_STATUS_DNR;
 	}
 
@@ -234,6 +236,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 			pr_err("failed to install queue %d cntlid %d ret %x\n",
 				qid, ctrl->cntlid, ret);
 			ctrl->sqs[qid] = NULL;
+			ctrl->cqs[qid] = NULL;
 			goto err;
 		}
 	}
@@ -242,6 +245,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 
 err:
 	req->sq->ctrl = NULL;
+	req->cq->ctrl = NULL;
+	nvmet_ctrl_put(ctrl);
 	return ret;
 }
 
-- 
2.43.7

Re: [PATCH 5/6] nvmet: fix controller reference leak in nvmet_install_queue() error paths

[Hannes Reinecke]

On 11/6/26 2:58 AM, Guixin Liu wrote:
> nvmet_install_queue() takes a controller reference for req->cq->ctrl
> and installs the controller in both req->sq->ctrl and req->cq->ctrl,
> but neither error path undoes its work: the busy return after the
> failed cq->ctrl cmpxchg leaves sq->ctrl set and the reference taken,
> and the err path for a failed transport ->install_queue() clears
> sq->ctrl but leaves cq->ctrl set with the reference still held.
> 
> The queue is then permanently wedged: a connect retry finds the stale
> cq->ctrl, takes another reference and fails again, and the leaked
> references keep the controller alive forever. Clearing cq->ctrl also
> matters because the caller drops its own reference, which can be the
> last one on the admin connect path, leaving cq->ctrl dangling.
> 
> Roll back the sq->ctrl assignment and drop the reference on both
> error paths, and also clear ctrl->cqs[qid] on the err path to mirror
> what nvmet_cq_destroy() would do.
> 
> Fixes: bb78836b3a7ca ("nvmet: fabrics: add CQ init and destroy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/target/fabrics-cmd.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index fc626c2fd2c9..773dad857a35 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -216,6 +216,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>   	if (old) {
>   		pr_warn("queue already connected!\n");
>   		req->error_loc = offsetof(struct nvmf_connect_command, opcode);
> +		req->sq->ctrl = NULL;
> +		nvmet_ctrl_put(ctrl);
>   		return NVME_SC_CONNECT_CTRL_BUSY | NVME_STATUS_DNR;
>   	}
>   
> @@ -234,6 +236,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>   			pr_err("failed to install queue %d cntlid %d ret %x\n",
>   				qid, ctrl->cntlid, ret);
>   			ctrl->sqs[qid] = NULL;
> +			ctrl->cqs[qid] = NULL;
>   			goto err;
>   		}
>   	}
> @@ -242,6 +245,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>   
>   err:
>   	req->sq->ctrl = NULL;
> +	req->cq->ctrl = NULL;
> +	nvmet_ctrl_put(ctrl);
>   	return ret;
>   }
>   

That looks wrong.
We already do a call to 'nvmet_ctrl_put()' in 
'nvmet_execute_admin_connect()', right after nvmet_install_queue()
failed.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 5/6] nvmet: fix controller reference leak in nvmet_install_queue() error paths

[Guixin Liu]

在 2026/11/6 15:31, Hannes Reinecke 写道:
> On 11/6/26 2:58 AM, Guixin Liu wrote:
>> nvmet_install_queue() takes a controller reference for req->cq->ctrl
>> and installs the controller in both req->sq->ctrl and req->cq->ctrl,
>> but neither error path undoes its work: the busy return after the
>> failed cq->ctrl cmpxchg leaves sq->ctrl set and the reference taken,
>> and the err path for a failed transport ->install_queue() clears
>> sq->ctrl but leaves cq->ctrl set with the reference still held.
>>
>> The queue is then permanently wedged: a connect retry finds the stale
>> cq->ctrl, takes another reference and fails again, and the leaked
>> references keep the controller alive forever. Clearing cq->ctrl also
>> matters because the caller drops its own reference, which can be the
>> last one on the admin connect path, leaving cq->ctrl dangling.
>>
>> Roll back the sq->ctrl assignment and drop the reference on both
>> error paths, and also clear ctrl->cqs[qid] on the err path to mirror
>> what nvmet_cq_destroy() would do.
>>
>> Fixes: bb78836b3a7ca ("nvmet: fabrics: add CQ init and destroy")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>   drivers/nvme/target/fabrics-cmd.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
>> index fc626c2fd2c9..773dad857a35 100644
>> --- a/drivers/nvme/target/fabrics-cmd.c
>> +++ b/drivers/nvme/target/fabrics-cmd.c
>> @@ -216,6 +216,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>>       if (old) {
>>           pr_warn("queue already connected!\n");
>>           req->error_loc = offsetof(struct nvmf_connect_command, opcode);
>> +        req->sq->ctrl = NULL;
>> +        nvmet_ctrl_put(ctrl);
>>           return NVME_SC_CONNECT_CTRL_BUSY | NVME_STATUS_DNR;
>>       }
>>   @@ -234,6 +236,7 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>>               pr_err("failed to install queue %d cntlid %d ret %x\n",
>>                   qid, ctrl->cntlid, ret);
>>               ctrl->sqs[qid] = NULL;
>> +            ctrl->cqs[qid] = NULL;
>>               goto err;
>>           }
>>       }
>> @@ -242,6 +245,8 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>>     err:
>>       req->sq->ctrl = NULL;
>> +    req->cq->ctrl = NULL;
>> +    nvmet_ctrl_put(ctrl);
>>       return ret;
>>   }
>>   
>
> That looks wrong.
> We already do a call to 'nvmet_ctrl_put()' in 'nvmet_execute_admin_connect()', right after nvmet_install_queue()
> failed.
>
> Cheers,
>
> Hannes
Right, I miss that, sorry, remove this in v2.

Best Regards,
Guixin Liu

[PATCH 6/6] nvmet: use GFP_KERNEL for PR allocations

[Guixin Liu]

nvmet_pr_update_reg_attr() and nvmet_pr_alloc_and_insert_pc_ref()
allocate with GFP_ATOMIC, but both run in sleepable context: the PR
command handlers execute from the transport workqueues and take the
pr_sem semaphore, and the pc_ref allocation is reached from the
connect and configfs paths. The latter function even passes
GFP_KERNEL to the percpu_ref_init() in the same scope, so the
GFP_ATOMIC only raises the failure rate for no reason.

Switch both allocations to GFP_KERNEL.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Cc: stable@vger.kernel.org
Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 drivers/nvme/target/pr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 14eaa99f98fa..026653ce338f 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -335,7 +335,7 @@ static u16 nvmet_pr_update_reg_attr(struct nvmet_pr *pr,
 		return NVME_SC_SUCCESS;
 	}
 
-	new = kmalloc_obj(*new, GFP_ATOMIC);
+	new = kmalloc_obj(*new, GFP_KERNEL);
 	if (!new)
 		return NVME_SC_INTERNAL;
 
@@ -1040,7 +1040,7 @@ static int nvmet_pr_alloc_and_insert_pc_ref(struct nvmet_ns *ns,
 	struct nvmet_pr_per_ctrl_ref *pc_ref;
 	int ret;
 
-	pc_ref = kmalloc_obj(*pc_ref, GFP_ATOMIC);
+	pc_ref = kmalloc_obj(*pc_ref, GFP_KERNEL);
 	if (!pc_ref)
 		return  -ENOMEM;
 
-- 
2.43.7

Re: [PATCH 6/6] nvmet: use GFP_KERNEL for PR allocations

[Hannes Reinecke]

On 11/6/26 2:58 AM, Guixin Liu wrote:
> nvmet_pr_update_reg_attr() and nvmet_pr_alloc_and_insert_pc_ref()
> allocate with GFP_ATOMIC, but both run in sleepable context: the PR
> command handlers execute from the transport workqueues and take the
> pr_sem semaphore, and the pc_ref allocation is reached from the
> connect and configfs paths. The latter function even passes
> GFP_KERNEL to the percpu_ref_init() in the same scope, so the
> GFP_ATOMIC only raises the failure rate for no reason.
> 
> Switch both allocations to GFP_KERNEL.
> 
> Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   drivers/nvme/target/pr.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
> index 14eaa99f98fa..026653ce338f 100644
> --- a/drivers/nvme/target/pr.c
> +++ b/drivers/nvme/target/pr.c
> @@ -335,7 +335,7 @@ static u16 nvmet_pr_update_reg_attr(struct nvmet_pr *pr,
>   		return NVME_SC_SUCCESS;
>   	}
>   
> -	new = kmalloc_obj(*new, GFP_ATOMIC);
> +	new = kmalloc_obj(*new, GFP_KERNEL);
>   	if (!new)
>   		return NVME_SC_INTERNAL;
>   
> @@ -1040,7 +1040,7 @@ static int nvmet_pr_alloc_and_insert_pc_ref(struct nvmet_ns *ns,
>   	struct nvmet_pr_per_ctrl_ref *pc_ref;
>   	int ret;
>   
> -	pc_ref = kmalloc_obj(*pc_ref, GFP_ATOMIC);
> +	pc_ref = kmalloc_obj(*pc_ref, GFP_KERNEL);
>   	if (!pc_ref)
>   		return  -ENOMEM;
>   

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 6/6] nvmet: use GFP_KERNEL for PR allocations

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/6] nvmet: use GFP_KERNEL for PR allocations

[Nilay Shroff]

On 11/6/26 7:28 AM, Guixin Liu wrote:
> nvmet_pr_update_reg_attr() and nvmet_pr_alloc_and_insert_pc_ref()
> allocate with GFP_ATOMIC, but both run in sleepable context: the PR
> command handlers execute from the transport workqueues and take the
> pr_sem semaphore, and the pc_ref allocation is reached from the
> connect and configfs paths. The latter function even passes
> GFP_KERNEL to the percpu_ref_init() in the same scope, so the
> GFP_ATOMIC only raises the failure rate for no reason.
> 
> Switch both allocations to GFP_KERNEL.
> 
> Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
> Cc:stable@vger.kernel.org
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>

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