linux-kernel - Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <84d028aa-31dd-4f8f-8370-3db4667d0dc7@flourine.local>
Date: Thu, 30 Oct 2025 10:49:08 +0100
From: Daniel Wagner <dwagner@...e.de>
To: Justin Tee <justintee8345@...il.com>
Cc: Daniel Wagner <wagi@...nel.org>, Christoph Hellwig <hch@....de>, 
	Keith Busch <kbusch@...nel.org>, James Smart <james.smart@...adcom.com>, 
	Jens Axboe <axboe@...nel.dk>, linux-nvme@...ts.infradead.org, 
	LKML <linux-kernel@...r.kernel.org>, Justin Tee <justin.tee@...adcom.com>
Subject: Re: [PATCH 1/5] nvme-fc: don't hold rport lock when putting ctrl

On Tue, Oct 28, 2025 at 05:33:17PM -0700, Justin Tee wrote:
> So, something like this?
> 
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 03987f497a5b..fcd30801921b 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1468,14 +1468,16 @@ 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, *ret = NULL;
> +       struct nvme_fc_ctrl *ctrl, *tmp, *ret = NULL;
>         struct nvmefc_ls_rcv_op *oldls = NULL;
>         u64 association_id = be64_to_cpu(rqst->associd.association_id);
>         unsigned long flags;
> 
> +       nvme_fc_rport_get(rport);
> +
>         spin_lock_irqsave(&rport->lock, flags);
> 
> -       list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> +       list_for_each_entry_safe(ctrl, tmp, &rport->ctrl_list, ctrl_list) {
>                 if (!nvme_fc_ctrl_get(ctrl))
>                         continue;
>                 spin_lock(&ctrl->lock);
> @@ -1488,11 +1490,15 @@ nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
>                 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);
> 
> +       nvme_fc_rport_put(rport);
> +
>         /* transmit a response for anything that was pending */
>         if (oldls) {
>                 dev_info(rport->lport->dev,

I've dropped the nvme_fc_rport_get because it's not necessary, a ref is
taken in nvme_fc_rcv_ls_req, so the port is not going away until the
disconnect LS is processed.

All tests seems to work fine except one, nvme/58 (test rapid namespace
remapping). But this one also fails for the other trasnport, e.g TCP:

	block nvme1n1: no available path - failing I/O
	Oops: general protection fault, probably for non-canonical address 0xdffffc00000I
	KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
	CPU: 5 UID: 0 PID: 707 Comm: kworker/5:2H Tainted: G      D W           6.17.0-ra
	Tainted: [D]=DIE, [W]=WARN
	Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-6.fc43 04/01/2014
	Workqueue: kblockd nvme_requeue_work [nvme_core]
	RIP: 0010:__rq_qos_done_bio+0x24/0xa0
	Code: 90 90 90 90 90 90 0f 1f 44 00 00 41 54 55 53 49 89 f4 48 83 ec 08 48 89 fbc
	RSP: 0018:ffff888116e07980 EFLAGS: 00010256
	RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
	RDX: 1ffff11022f2a6eb RSI: ffff88810d2a6140 RDI: 0000000000000000
	nvme nvme4: IDs don't match for shared namespace 3
	RBP: dffffc0000000000 R08: 0000000000000001 R09: ffffffff9f4df3cf
	R10: ffff888116e0760f R11: ffffffffa6b771d0 R12: ffff88810d2a6140
	R13: ffff88810d2a6148 R14: ffff88810d2a6140 R15: ffff888106d66808
	FS:  0000000000000000(0000) GS:ffff8881b2408000(0000) knlGS:0000000000000000
	CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
	nvme nvme3: rescanning namespaces.
	CR2: 000055f4b1f6d080 CR3: 000000011c082000 CR4: 0000000000750ef0
	PKRU: 55555554
	Call Trace:
	 <TASK>
	 bio_endio+0x1c9/0x610
	 nvme_ns_head_submit_bio+0x79e/0xa20 [nvme_core 8550ebbc328e8e8a801d6cacfcd551a5]
	 ? lock_release+0x244/0x450
	 __submit_bio+0x31a/0x6c0
	 ? __pfx___submit_bio+0x10/0x10
	 ? lock_acquire+0x292/0x2e0
	 ? lock_release+0x244/0x450
	 ? __pfx_blk_cgroup_bio_start+0x10/0x10
	 ? lock_acquire+0x292/0x2e0
	 ? submit_bio_noacct_nocheck+0x577/0xaa0
	 submit_bio_noacct_nocheck+0x577/0xaa0
	 ? trace_irq_enable.constprop.0+0xc0/0x100
	 ? trace_hardirqs_on+0x32/0x40
	 ? __pfx_submit_bio_noacct_nocheck+0x10/0x10
	 ? submit_bio_noacct+0x97d/0x1ce0
	 nvme_requeue_work+0xa2/0xd0 [nvme_core 8550ebbc328e8e8a801d6cacfcd551a5345e7fd9]
	 process_one_work+0x7f3/0x1420
	 ? __pfx_process_one_work+0x10/0x10
	 ? local_clock+0x11/0x30
	 ? assign_work+0x156/0x390
	 worker_thread+0x5cf/0xfe0
	 ? _raw_spin_unlock_irqrestore+0x40/0x60
	 ? __pfx_worker_thread+0x10/0x10
	 ? __kthread_parkme+0xb3/0x1f0
	 ? __pfx_worker_thread+0x10/0x10
	 kthread+0x39d/0x740
	 ? __pfx_do_raw_spin_trylock+0x10/0x10
	 ? __pfx_kthread+0x10/0x10
	 ? kvm_sched_clock_read+0xd/0x20
	 ? local_clock_noinstr+0xb/0xf0
	 ? local_clock+0x11/0x30
	 ? lock_release+0x2e2/0x450
	 ? __pfx_kthread+0x10/0x10
	 ret_from_fork+0x371/0x460
	 ? __pfx_kthread+0x10/0x10
	 ? __pfx_kthread+0x10/0x10
	 ret_from_fork_asm+0x1a/0x30
	 </TASK>
	Modules linked in: nvmet_tcp nvmet nvme_tcp nvme nvme_fabrics nvme_core nvme_autp
	---[ end trace 0000000000000000 ]---