| 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
| ||
|
Message-ID: <002aee76-9114-4029-85b3-aa04e8ef76ed@suse.de>
Date: Thu, 24 Apr 2025 12:08:33 +0200
From: Hannes Reinecke <hare@...e.de>
To: Daniel Wagner <wagi@...nel.org>, James Smart <james.smart@...adcom.com>,
Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>,
Chaitanya Kulkarni <kch@...dia.com>
Cc: Keith Busch <kbusch@...nel.org>, linux-nvme@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 01/14] nvmet-fcloop: track ref counts for nports
On 4/23/25 15:21, Daniel Wagner wrote:
> A nport object is always used in association with targerport,
> remoteport, tport and rport objects. Add explicit references for any of
> the associated object. This ensures that nport is not removed too early
> on shutdown sequences.
>
> Signed-off-by: Daniel Wagner <wagi@...nel.org>
> ---
> drivers/nvme/target/fcloop.c | 133 +++++++++++++++++++++++++++++--------------
> 1 file changed, 90 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 641201e62c1bafa13986642c6c4067b35f784edd..2b23e43ef4403fa4d70c66263f7750165d2ddc72 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1047,8 +1047,14 @@ static void
> fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
> {
> struct fcloop_rport *rport = remoteport->private;
> + unsigned long flags;
>
> flush_work(&rport->ls_work);
> +
> + spin_lock_irqsave(&fcloop_lock, flags);
> + rport->nport->rport = NULL;
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> fcloop_nport_put(rport->nport);
> }
>
> @@ -1056,8 +1062,14 @@ static void
> fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> {
> struct fcloop_tport *tport = targetport->private;
> + unsigned long flags;
>
> flush_work(&tport->ls_work);
> +
> + spin_lock_irqsave(&fcloop_lock, flags);
> + tport->nport->tport = NULL;
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> fcloop_nport_put(tport->nport);
> }
>
> @@ -1184,6 +1196,37 @@ __wait_localport_unreg(struct fcloop_lport *lport)
> return ret;
> }
>
> +static struct fcloop_nport *
> +__fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> + struct fcloop_nport *nport;
> +
> + list_for_each_entry(nport, &fcloop_nports, nport_list) {
> + if (nport->node_name != node_name ||
> + nport->port_name != port_name)
> + continue;
> +
> + if (fcloop_nport_get(nport))
> + return nport;
> +
> + break;
> + }
> +
> + return NULL;
> +}
> +
> +static struct fcloop_nport *
> +fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> + struct fcloop_nport *nport;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&fcloop_lock, flags);
> + nport = __fcloop_nport_lookup(node_name, port_name);
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> + return nport;
> +}
>
> static ssize_t
> fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
> @@ -1365,6 +1408,8 @@ __unlink_remote_port(struct fcloop_nport *nport)
> {
> struct fcloop_rport *rport = nport->rport;
>
> + lockdep_assert_held(&fcloop_lock);
> +
> if (rport && nport->tport)
> nport->tport->remoteport = NULL;
> nport->rport = NULL;
> @@ -1377,9 +1422,6 @@ __unlink_remote_port(struct fcloop_nport *nport)
> static int
> __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
> {
> - if (!rport)
> - return -EALREADY;
> -
> return nvme_fc_unregister_remoteport(rport->remoteport);
> }
>
> @@ -1387,8 +1429,8 @@ static ssize_t
> fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct fcloop_nport *nport = NULL, *tmpport;
> - static struct fcloop_rport *rport;
> + struct fcloop_nport *nport;
> + struct fcloop_rport *rport;
> u64 nodename, portname;
> unsigned long flags;
> int ret;
> @@ -1397,24 +1439,24 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
> if (ret)
> return ret;
>
> - spin_lock_irqsave(&fcloop_lock, flags);
> -
> - list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> - if (tmpport->node_name == nodename &&
> - tmpport->port_name == portname && tmpport->rport) {
> - nport = tmpport;
> - rport = __unlink_remote_port(nport);
> - break;
> - }
> - }
> + nport = fcloop_nport_lookup(nodename, portname);
> + if (!nport)
> + return -ENOENT;
>
> + spin_lock_irqsave(&fcloop_lock, flags);
> + rport = __unlink_remote_port(nport);
> spin_unlock_irqrestore(&fcloop_lock, flags);
>
> - if (!nport)
> - return -ENOENT;
> + if (!rport) {
> + ret = -ENOENT;
> + goto out_nport_put;
> + }
>
> ret = __remoteport_unreg(nport, rport);
>
> +out_nport_put:
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
> @@ -1465,6 +1507,8 @@ __unlink_target_port(struct fcloop_nport *nport)
> {
> struct fcloop_tport *tport = nport->tport;
>
> + lockdep_assert_held(&fcloop_lock);
> +
> if (tport && nport->rport)
> nport->rport->targetport = NULL;
> nport->tport = NULL;
> @@ -1475,9 +1519,6 @@ __unlink_target_port(struct fcloop_nport *nport)
> static int
> __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> {
> - if (!tport)
> - return -EALREADY;
> -
> return nvmet_fc_unregister_targetport(tport->targetport);
> }
>
> @@ -1485,8 +1526,8 @@ static ssize_t
> fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct fcloop_nport *nport = NULL, *tmpport;
> - struct fcloop_tport *tport = NULL;
> + struct fcloop_nport *nport;
> + struct fcloop_tport *tport;
> u64 nodename, portname;
> unsigned long flags;
> int ret;
> @@ -1495,24 +1536,24 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> if (ret)
> return ret;
>
> - spin_lock_irqsave(&fcloop_lock, flags);
> -
> - list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> - if (tmpport->node_name == nodename &&
> - tmpport->port_name == portname && tmpport->tport) {
> - nport = tmpport;
> - tport = __unlink_target_port(nport);
> - break;
> - }
> - }
> + nport = fcloop_nport_lookup(nodename, portname);
> + if (!nport)
> + return -ENOENT;
>
> + spin_lock_irqsave(&fcloop_lock, flags);
> + tport = __unlink_target_port(nport);
> spin_unlock_irqrestore(&fcloop_lock, flags);
>
Hmm. This now has a race condition; we're taking the lock
during lokup, drop the lock, take the lock again, and unlink
the port.
Please do a __fcloop_nport_lookup() function which doesn't
take a lock and avoid this race.
> - if (!nport)
> - return -ENOENT;
> + if (!tport) {
> + ret = -ENOENT;
> + goto out_nport_put;
> + }
>
> ret = __targetport_unreg(nport, tport);
>
> +out_nport_put:
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
> @@ -1609,8 +1650,8 @@ static int __init fcloop_init(void)
>
> static void __exit fcloop_exit(void)
> {
> - struct fcloop_lport *lport = NULL;
> - struct fcloop_nport *nport = NULL;
> + struct fcloop_lport *lport;
> + struct fcloop_nport *nport;
> struct fcloop_tport *tport;
> struct fcloop_rport *rport;
> unsigned long flags;
> @@ -1621,7 +1662,7 @@ static void __exit fcloop_exit(void)
> for (;;) {
> nport = list_first_entry_or_null(&fcloop_nports,
> typeof(*nport), nport_list);
> - if (!nport)
> + if (!nport || !fcloop_nport_get(nport))
> break;
>
> tport = __unlink_target_port(nport);
> @@ -1629,13 +1670,19 @@ static void __exit fcloop_exit(void)
>
> spin_unlock_irqrestore(&fcloop_lock, flags);
>
> - ret = __targetport_unreg(nport, tport);
> - if (ret)
> - pr_warn("%s: Failed deleting target port\n", __func__);
> + if (tport) {
> + ret = __targetport_unreg(nport, tport);
> + if (ret)
> + pr_warn("%s: Failed deleting target port\n", __func__);
> + }
>
And same here; don't drop the lock after lookup.
> - ret = __remoteport_unreg(nport, rport);
> - if (ret)
> - pr_warn("%s: Failed deleting remote port\n", __func__);
> + if (rport) {
> + ret = __remoteport_unreg(nport, rport);
> + if (ret)
> + pr_warn("%s: Failed deleting remote port\n", __func__);
> + }
> +
> + fcloop_nport_put(nport);
>
> spin_lock_irqsave(&fcloop_lock, flags);
> }
>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@...e.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
Powered by blists - more mailing lists