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-next>] [day] [month] [year] [list]

Message-ID: <CABPRKS9GGS2mQ46UMmM3G_NnamCsYj7zBaa_c4rQfO1h=59wrQ@mail.gmail.com>
Date: Tue, 28 Oct 2025 17:33:17 -0700
From: Justin Tee <justintee8345@...il.com>
To: Daniel Wagner <wagi@...nel.org>
Cc: 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

Hi Daniel,

> nvme_fc_ctrl_put can acquire the rport lock when freeing the
> ctrl object:
>
> nvme_fc_ctrl_put
>   nvme_fc_ctrl_free
>     spin_lock_irqsave(rport->lock)
>
> Thus we can't hold the rport lock when calling nvme_fc_ctrl_put.

While I agree that we can’t hold the rport lock when calling
nvme_fc_ctrl_put, nvme_fc_ctrl_free also does a nvme_fc_rport_put,
which could also trigger nvme_fc_free_rport, making rport invalid.
Should we also add kref get on the rport before entering the
list_for_each_entry loop?

Also, because nvme_fc_ctrl_free removes itself from the
rport->ctrl_list, should we also start using list_for_each_entry_safe?

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,

Regards,
Justin