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

On Wed, Aug 12, 2026 at 2:32 PM <[email protected]> wrote:
>
> 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?
will fix.

>
> 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?
yes, will fix

>
> 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?
Yes it uses a reference count.

> 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
>