| 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: <a9055e3c-d36f-4706-9fdc-f4532d14adb6@suse.de>
Date: Tue, 18 Mar 2025 12:02:48 +0100
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 v3 04/18] nvmet-fcloop: refactor fcloop_nport_alloc
On 3/18/25 11:39, Daniel Wagner wrote:
> There are many different operations done under the spin lock. Since the
> lport and nport are ref counted it's possible to use the spin lock only
> for the list insert and lookup.
>
> This allows us to untangle the setup steps into a more linear form which
> reduces the complexity of the functions.
>
> Reviewed-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Daniel Wagner <wagi@...nel.org>
> ---
> drivers/nvme/target/fcloop.c | 156 +++++++++++++++++++++++--------------------
> 1 file changed, 84 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index c6d5a31ce0b5ccb10988e339ae22d528e06d5e2b..245bfe08d91ec81f1979251e8c757a0d46fd09e9 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1027,6 +1027,8 @@ fcloop_nport_put(struct fcloop_nport *nport)
> list_del(&nport->nport_list);
> spin_unlock_irqrestore(&fcloop_lock, flags);
>
> + if (nport->lport)
> + fcloop_lport_put(nport->lport);
> kfree(nport);
> }
>
> @@ -1189,33 +1191,63 @@ __wait_localport_unreg(struct fcloop_lport *lport)
> return ret;
> }
>
> +static struct fcloop_lport *
> +fcloop_lport_lookup(u64 node_name, u64 port_name)
> +{
> + struct fcloop_lport *lp, *lport = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&fcloop_lock, flags);
> + list_for_each_entry(lp, &fcloop_lports, lport_list) {
> + if (lp->localport->node_name != node_name ||
> + lp->localport->port_name != port_name)
> + continue;
> +
> + if (fcloop_lport_get(lp))
> + lport = lp;
> +
> + break;
> + }
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> + return lport;
> +}
> +
> +static struct fcloop_nport *
> +fcloop_nport_lookup(u64 node_name, u64 port_name)
> +{
> + struct fcloop_nport *np, *nport = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&fcloop_lock, flags);
> + list_for_each_entry(np, &fcloop_nports, nport_list) {
> + if (np->node_name != node_name ||
> + np->port_name != port_name)
> + continue;
> +
> + if (fcloop_nport_get(np))
> + nport = np;
> +
> + break;
> + }
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> +
> + return nport;
> +}
>
> static ssize_t
> fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct fcloop_lport *tlport, *lport = NULL;
> + struct fcloop_lport *lport;
> u64 nodename, portname;
> - unsigned long flags;
> int ret;
>
> ret = fcloop_parse_nm_options(dev, &nodename, &portname, buf);
> if (ret)
> return ret;
>
> - spin_lock_irqsave(&fcloop_lock, flags);
> -
> - list_for_each_entry(tlport, &fcloop_lports, lport_list) {
> - if (tlport->localport->node_name == nodename &&
> - tlport->localport->port_name == portname) {
> - if (!fcloop_lport_get(tlport))
> - break;
> - lport = tlport;
> - break;
> - }
> - }
> - spin_unlock_irqrestore(&fcloop_lock, flags);
> -
> + lport = fcloop_lport_lookup(nodename, portname);
> if (!lport)
> return -ENOENT;
>
> @@ -1228,9 +1260,9 @@ fcloop_delete_local_port(struct device *dev, struct device_attribute *attr,
> static struct fcloop_nport *
> fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
> {
> - struct fcloop_nport *newnport, *nport = NULL;
> - struct fcloop_lport *tmplport, *lport = NULL;
> struct fcloop_ctrl_options *opts;
> + struct fcloop_nport *nport;
> + struct fcloop_lport *lport;
> unsigned long flags;
> u32 opts_mask = (remoteport) ? RPORT_OPTS : TGTPORT_OPTS;
> int ret;
> @@ -1244,79 +1276,59 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport)
> goto out_free_opts;
>
> /* everything there ? */
> - if ((opts->mask & opts_mask) != opts_mask) {
> - ret = -EINVAL;
> + if ((opts->mask & opts_mask) != opts_mask)
> goto out_free_opts;
> - }
>
> - newnport = kzalloc(sizeof(*newnport), GFP_KERNEL);
> - if (!newnport)
> + lport = fcloop_lport_lookup(opts->wwnn, opts->wwpn);
> + if (lport) {
> + /* invalid configuration */
> + fcloop_lport_put(lport);
> goto out_free_opts;
> + }
>
> - INIT_LIST_HEAD(&newnport->nport_list);
> - newnport->node_name = opts->wwnn;
> - newnport->port_name = opts->wwpn;
> - if (opts->mask & NVMF_OPT_ROLES)
> - newnport->port_role = opts->roles;
> - if (opts->mask & NVMF_OPT_FCADDR)
> - newnport->port_id = opts->fcaddr;
> - refcount_set(&newnport->ref, 1);
> + nport = fcloop_nport_lookup(opts->wwnn, opts->wwpn);
> + if (nport && ((remoteport && nport->rport) ||
> + (!remoteport && nport->tport))) {
> + /* invalid configuration */
> + goto out_put_nport;
> + }
>
> - spin_lock_irqsave(&fcloop_lock, flags);
> + if (!nport) {
> + nport = kzalloc(sizeof(*nport), GFP_KERNEL);
> + if (!nport)
> + goto out_free_opts;
>
> - list_for_each_entry(tmplport, &fcloop_lports, lport_list) {
> - if (tmplport->localport->node_name == opts->wwnn &&
> - tmplport->localport->port_name == opts->wwpn)
> - goto out_invalid_opts;
> + INIT_LIST_HEAD(&nport->nport_list);
> + nport->node_name = opts->wwnn;
> + nport->port_name = opts->wwpn;
> + refcount_set(&nport->ref, 1);
>
> - if (tmplport->localport->node_name == opts->lpwwnn &&
> - tmplport->localport->port_name == opts->lpwwpn)
> - lport = tmplport;
> + spin_lock_irqsave(&fcloop_lock, flags);
> + list_add_tail(&nport->nport_list, &fcloop_nports);
> + spin_unlock_irqrestore(&fcloop_lock, flags);
> }
>
Hmm. I don't really like this pattern; there is a race condition
between lookup and allocation leading to possibly duplicate entries
on the list.
Lookup and allocation really need to be under the same lock.
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