[PATCH v2 0/5] nvme: fabrics and target fixes

[PATCH v2 0/5] nvme: fabrics and target fixes

[Guixin Liu]

Five 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
 - a use-after-free in nvme_fc_match_disconn_ls(): no iterator is
   sound across the rport lock drop inside the loop, so the loop is
   reworked to run entirely under the rport lock and to take a
   reference only for the match; reproduced under KASAN through fcloop
 - 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
 - needless GFP_ATOMIC allocations in the nvmet persistent reservation
   code

The reconnect_delay, tbkas, nvme-auth and persistent reservation fixes
are verified on an initiator/target VM pair running the patched
kernel: 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
reworked nvme-fc loop is compile tested and exercised under KASAN
through fcloop with six controllers on one remote port across tbkas
force-reconnect cycles and concurrent disconnects, with no reports.

Changes since v1:

 - rework the nvme-fc fix: neither the _safe iterator nor a plain one
   is sound across the lock drop, as the loop's own put can be the
   final reference; the loop now runs entirely under the rport lock
   and takes a reference only for the match (Hannes Reinecke, sashiko)
 - drop the nvmet_install_queue() patch: the callers already release
   their reference on failure (Hannes Reinecke)
 - use atomic_inc_return() in the nvme-auth patch (Hannes Reinecke)
 - correct the Fixes tag of the reconnect_delay patch to the commit
   that introduced the sysfs store (sashiko, confirmed by Christoph
   Hellwig)

Link: https://lore.kernel.org/all/20261106015842.3534898-1-kanie@linux.alibaba.com/

Guixin Liu (5):
  nvme: reject a reconnect_delay of 0 through sysfs
  nvme-fc: fix use-after-free 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: use GFP_KERNEL for PR allocations

 drivers/nvme/host/auth.c       |  2 +-
 drivers/nvme/host/fc.c         | 22 ++++++++++++----------
 drivers/nvme/host/nvme.h       |  2 +-
 drivers/nvme/host/sysfs.c      |  3 +++
 drivers/nvme/target/configfs.c |  2 ++
 drivers/nvme/target/pr.c       |  4 ++--
 6 files changed, 21 insertions(+), 14 deletions(-)

-- 
2.43.7

[PATCH v2 1/5] 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: 764075fdcb2f ("nvme: expose reconnect_delay and ctrl_loss_tmo via sysfs")
Cc: stable@vger.kernel.org
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
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

[PATCH v2 2/5] nvme-fc: fix use-after-free in nvme_fc_match_disconn_ls()

[Guixin Liu]

Commit b71cbcf7d170e ("nvme-fc: don't hold rport lock when putting ctrl")
dropped the rport lock inside the loop to put the reference of a
non-matching controller, because nvme_fc_ctrl_free() takes the rport
lock to unlink the controller. But no iterator is sound across that
drop: with list_for_each_entry_safe() the prefetched next entry can be
freed in the window, and with a plain iterator the loop's own
nvme_fc_ctrl_put() can be the final reference, freeing the current
entry before the iterator reads its ->next pointer.

The reference dance is unnecessary in the first place. An entry cannot
leave ctrl_list while rport->lock is held, as the final put unlinks it
under that very lock, so association_id can be inspected under
ctrl->lock without holding a reference. Take a reference only for the
matching controller, still under the rport lock, and hand it to the
caller as before. The loop then no longer needs to drop the lock at
all, which also removes the reason the lock was dropped for in the
first place.

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 several controllers
on one remote port and a tbkas force-reconnect racing concurrent
controller disconnects; the lock-drop window was widened with a
test-only mdelay(200) to make the microsecond window deterministic:

  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 freed object was the prefetched next entry, allocated by
nvme_fc_alloc_ctrl() on connect and freed by nvme_free_ctrl() on the
concurrent teardown. The loop increment dereferences it before
nvme_fc_ctrl_get() gets a chance to reject it, which is why the
get-unless-zero check in the loop body does not help. The reworked
loop does not drop the lock, so no such window is left to widen.
---
 drivers/nvme/host/fc.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0b9ff50d26ff..e28eafc362db 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1470,29 +1470,31 @@ 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) {
-		if (!nvme_fc_ctrl_get(ctrl))
-			continue;
+	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
 		spin_lock(&ctrl->lock);
 		if (association_id == ctrl->association_id) {
-			oldls = ctrl->rcv_disconn;
-			ctrl->rcv_disconn = lsop;
-			ret = ctrl;
+			/*
+			 * Entries cannot leave the list while rport->lock is
+			 * held, so they can be inspected without a reference.
+			 * Only the match needs one, for the caller.
+			 */
+			if (nvme_fc_ctrl_get(ctrl)) {
+				oldls = ctrl->rcv_disconn;
+				ctrl->rcv_disconn = lsop;
+				ret = ctrl;
+			}
 		}
 		spin_unlock(&ctrl->lock);
 		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);
 	}
 
 	spin_unlock_irqrestore(&rport->lock, flags);
-- 
2.43.7

[PATCH v2 3/5] 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 atomic_inc_return(); the
first transaction id is now 1 instead of 0, which is immaterial as
the controller only echoes the id within a queue.

Reviewed-by: Hannes Reinecke <hare@kernel.org>
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..c605e8f47e9a 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_inc_return(&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

[PATCH v2 4/5] 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
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
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

[PATCH v2 5/5] 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
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
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 v2 3/5] nvme-auth: fix data race on ctrl->transaction

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] nvme-auth: missing Fixes trailer for ctrl->transaction data race
  fix
--

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

> nvme-auth: fix data race on ctrl->transaction
>
> 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 atomic_inc_return(); the
> first transaction id is now 1 instead of 0, which is immaterial as
> the controller only echoes the id within a queue.
>
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

[Severity: Low]
Since this fixes the unsynchronized increment of ctrl->transaction across
concurrent per-queue authentication work items, should the commit message
include:

  Fixes: f50fff73d620 ("nvme: implement In-Band authentication")

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

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

[Keith Busch]

On Mon, Nov 09, 2026 at 10:26:44AM +0800, Guixin Liu wrote:
> 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. 

So it's not a bug with any user observable behavior; it's just
unexpected. It sounds like the "transaction" value doesn't need to be
unique across queues. If it doesn't need to be unique at the "ctrl"
scope, then I'm not seeing any benefit to imposing that rule on the
driver.

> -	chap->transaction = ctrl->transaction++;
> +	chap->transaction = (u16)atomic_inc_return(&ctrl->transaction);

Can we just change this to:

	chap->transaction++;

? And delete the value reset in nvme_auth_reset_dhchap? You can also
delete the "transaction" field from the nvme_ctrl struct.

Re: (subset) [PATCH v2 0/5] nvme: fabrics and target fixes

[Keith Busch]

On Mon, 9 Nov 2026 10:26:41 +0800, Guixin Liu wrote:
> Five 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
>  - a use-after-free in nvme_fc_match_disconn_ls(): no iterator is
>    sound across the rport lock drop inside the loop, so the loop is
>    reworked to run entirely under the rport lock and to take a
>    reference only for the match; reproduced under KASAN through fcloop
>  - 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
>  - needless GFP_ATOMIC allocations in the nvmet persistent reservation
>    code
> 
> [...]

Thanks, applied to nvme-7.4.

[5/5] nvmet: use GFP_KERNEL for PR allocations
      commit: 976311c5594a7a26047891f087ad59a804e02baa
--
Keith Busch <kbusch@kernel.org>

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

[Keith Busch]

On Mon, Nov 09, 2026 at 10:26:42AM +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.

There was another patch sent earlier with an identical fix posted here:

  https://lore.kernel.org/linux-nvme/20261105215807526OqR0Bg86VChvwhCEz5xGr@zte.com.cn/

I ended up applying the one that published first, but removed the
unnecessary code comments.

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

[Guixin Liu]

在 2026/11/10 01:10, Keith Busch 写道:
> On Mon, Nov 09, 2026 at 10:26:44AM +0800, Guixin Liu wrote:
>> 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. 
> So it's not a bug with any user observable behavior; it's just
> unexpected. It sounds like the "transaction" value doesn't need to be
> unique across queues. If it doesn't need to be unique at the "ctrl"
> scope, then I'm not seeing any benefit to imposing that rule on the
> driver.
>
>> -	chap->transaction = ctrl->transaction++;
>> +	chap->transaction = (u16)atomic_inc_return(&ctrl->transaction);
> Can we just change this to:
>
> 	chap->transaction++;
>
> ? And delete the value reset in nvme_auth_reset_dhchap? You can also
> delete the "transaction" field from the nvme_ctrl struct.
This make sense, changed in v3.

Best Regards,
Guixin Liu

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

[Guixin Liu]

在 2026/11/10 01:54, Keith Busch 写道:
> On Mon, Nov 09, 2026 at 10:26:42AM +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.
> There was another patch sent earlier with an identical fix posted here:
>
>   https://lore.kernel.org/linux-nvme/20261105215807526OqR0Bg86VChvwhCEz5xGr@zte.com.cn/
>
> I ended up applying the one that published first, but removed the
> unnecessary code comments.
OK, and could you please apply the number 4 patch too? I will drop the two
patches which you applied in v3, thanks.

Best Regards,
Guixin Liu