| 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: <d4b09a75-30f8-4db7-a02b-25ae405f7340@suse.de>
Date: Fri, 28 Feb 2025 08:19:19 +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 04/11] nvmet-fcloop: track ref counts for nports
On 2/26/25 19:45, 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 makes the unreliable reference updates in
> the two callback fcloop_targetport_delete and fcloop_remoteport_delete
> obsolete.
>
> Signed-off-by: Daniel Wagner <wagi@...nel.org>
> ---
> drivers/nvme/target/fcloop.c | 57 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index de1963c34bd88d0335f70de569565740fd395a0a..80693705c069dd114b2d4f15d0482dd2d713a273 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -1071,16 +1071,11 @@ fcloop_remoteport_delete(struct nvme_fc_remote_port *remoteport)
> struct fcloop_rport *rport = remoteport->private;
>
> flush_work(&rport->ls_work);
> - fcloop_nport_put(rport->nport);
> }
>
> static void
> fcloop_targetport_delete(struct nvmet_fc_target_port *targetport)
> {
> - struct fcloop_tport *tport = targetport->private;
> -
> - flush_work(&tport->ls_work);
> - fcloop_nport_put(tport->nport);
> }
>
Errm. Isn't this function empty now? Can't it be remove altogether?
> #define FCLOOP_HW_QUEUES 4
> @@ -1358,6 +1353,7 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
> struct nvme_fc_port_info pinfo;
> int ret;
>
> + /* nport ref get: rport */
> nport = fcloop_alloc_nport(buf, count, true);
> if (!nport)
> return -EIO;
> @@ -1375,6 +1371,9 @@ fcloop_create_remote_port(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> + /* nport ref get: remoteport */
> + fcloop_nport_get(nport);
> +
> /* success */
> rport = remoteport->private;
> rport->remoteport = remoteport;
> @@ -1403,16 +1402,27 @@ __unlink_remote_port(struct fcloop_nport *nport)
> nport->tport->remoteport = NULL;
> nport->rport = NULL;
>
> + /* nport ref put: rport */
> + fcloop_nport_put(nport);
> +
> return rport;
> }
>
> static int
> __remoteport_unreg(struct fcloop_nport *nport, struct fcloop_rport *rport)
> {
> - if (!rport)
> - return -EALREADY;
> + int ret;
>
> - return nvme_fc_unregister_remoteport(rport->remoteport);
> + if (!rport) {
> + ret = -EALREADY;
> + goto out;
> + }
> +
> + ret = nvme_fc_unregister_remoteport(rport->remoteport);
> +out:
> + /* nport ref put: remoteport */
> + fcloop_nport_put(nport);
> + return ret;
> }
>
> static ssize_t
> @@ -1434,6 +1444,9 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
> list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> if (tmpport->node_name == nodename &&
> tmpport->port_name == portname && tmpport->rport) {
> +
> + if (!fcloop_nport_get(tmpport))
> + break;
> nport = tmpport;
> rport = __unlink_remote_port(nport);
> break;
> @@ -1447,6 +1460,8 @@ fcloop_delete_remote_port(struct device *dev, struct device_attribute *attr,
>
> ret = __remoteport_unreg(nport, rport);
>
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
> @@ -1460,6 +1475,7 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> struct nvmet_fc_port_info tinfo;
> int ret;
>
> + /* nport ref get: tport */
> nport = fcloop_alloc_nport(buf, count, false);
> if (!nport)
> return -EIO;
> @@ -1475,6 +1491,9 @@ fcloop_create_target_port(struct device *dev, struct device_attribute *attr,
> return ret;
> }
>
> + /* nport ref get: targetport */
> + fcloop_nport_get(nport);
> +
I would rather move it to the end of the function, after we set all the
references. But that's probably personal style...
> /* success */
> tport = targetport->private;
> tport->targetport = targetport;
> @@ -1501,16 +1520,27 @@ __unlink_target_port(struct fcloop_nport *nport)
> nport->rport->targetport = NULL;
> nport->tport = NULL;
>
> + /* nport ref put: tport */
> + fcloop_nport_put(nport);
> +
> return tport;
> }
>
> static int
> __targetport_unreg(struct fcloop_nport *nport, struct fcloop_tport *tport)
> {
> - if (!tport)
> - return -EALREADY;
> + int ret;
That's iffy.
Q1: How can you end up with a NULL tport?
Q2: Why do we have _two_ arguments? Can't we use 'nport->tport'?
Q3: What do you do when tport is valid and tport != nport->tport?
>
> - return nvmet_fc_unregister_targetport(tport->targetport);
> + if (!tport) {
> + ret = -EALREADY;
> + goto out;
> + }
> +
> + ret = nvmet_fc_unregister_targetport(tport->targetport);
> +out:
> + /* nport ref put: targetport */
> + fcloop_nport_put(nport);
> + return ret;
> }
>
> static ssize_t
> @@ -1532,6 +1562,9 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
> list_for_each_entry(tmpport, &fcloop_nports, nport_list) {
> if (tmpport->node_name == nodename &&
> tmpport->port_name == portname && tmpport->tport) {
> +
> + if (!fcloop_nport_get(tmpport))
> + break;
> nport = tmpport;
> tport = __unlink_target_port(nport);
> break;
> @@ -1545,6 +1578,8 @@ fcloop_delete_target_port(struct device *dev, struct device_attribute *attr,
>
> ret = __targetport_unreg(nport, tport);
>
> + fcloop_nport_put(nport);
> +
> return ret ? ret : count;
> }
>
>
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