Re: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `opts` structure is allocated on the stack and passed uninitialized to `fcloop_parse_options`, leading to undefined behavior and use of stack garbage.
- [High] A reference count leak occurs on the error path in `fcloop_set_marginal_rport` when the target port or remote port is missing.
- [High] Lockless access to `nport->tport` and `nport->tport->remoteport` creates a TOCTOU bug that can lead to a Use-After-Free.
--

--- Patch [12]: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs ---
commit 0c2a40a45b26fd619c8713ddb24578a051043b92
Author: Jesse Taube <[email protected]>

nvme: fcloop: Add set_rport_marginal to sysfs

This commit adds a sysfs attribute to allow setting a remote port as marginal.
This helps facilitate testing of multipath failover by simulating a marginal
link in the fcloop LLDD.

> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index b63af3b643a64..51a50743128db 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c

[ ... ]

> @@ -1663,6 +1677,31 @@ fcloop_set_cmd_drop(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static ssize_t
> +fcloop_set_marginal_rport(struct device *dev, struct device_attribute *attr,
> +		const char *buf, size_t count)
> +{
> +	struct fcloop_nport *nport;
> +	struct fcloop_ctrl_options opts;

[Severity: High]
Does this leave the opts structure uninitialized?

Looking at fcloop_parse_options(), it uses opts->mask |= token during parsing,
which performs a read-modify-write operation. Since opts is allocated on the
stack here without initialization, could this use stack garbage and result in
false validation bypass or incorrect functional behavior?

> +	int ret;
> +
> +	ret = fcloop_parse_options(&opts, buf);
> +	if (ret)
> +		return ret;
> +
> +	/* everything there ? */
> +	if ((opts.mask & MARGINAL_OPTS) != MARGINAL_OPTS)
> +		return -EINVAL;
> +
> +	nport = fcloop_nport_lookup(opts.wwnn, opts.wwpn);
> +	if (!nport || !nport->tport || !nport->tport->remoteport)
> +		return -ENOENT;

[Severity: High]
If nport is successfully found but !nport->tport or !nport->tport->remoteport
is true, does this leak the reference count on nport?

The fcloop_nport_lookup() function increments the reference count of the
returned nport. Returning -ENOENT early here skips the fcloop_nport_put()
call, which would permanently prevent the nport from being freed.

[Severity: High]
Is it safe to access nport->tport and nport->tport->remoteport locklessly
here?

Could a concurrent write to del_target_port take fcloop_lock, clear these
pointers, and free the targetport memory while this function is actively
dereferencing them, resulting in a use-after-free?

> +
> +	nvme_fc_set_remoteport_fpin(nport->tport->remoteport, opts.marginal);
> +	fcloop_nport_put(nport);
> +
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12