Re: [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A TOCTOU / race condition in `fc_rport_set_marginal_state` allows the software remote port state (`rport->port_state`) to become permanently desynchronized from the low-level driver's hardware state.
- [Medium] Meaningful mismatch between the commit message and the implemented code regarding the callback function used.
--

--- Patch [8]: [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL ---
commit 22fbcc98c1f2a6b4c603d5765d66e355a3468638
Author: John Meneghini <[email protected]>

scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL

This commit refactors fc_rport_set_marginal_state to hold the host_lock
around port_state accesses. It also introduces a callback mechanism to
propagate the marginal state changes to the low-level driver when modified
via sysfs.

> Call nvme_fc_modify_rport_fpin_state() when FC_PORTSTATE_MARGINAL is set
> or cleared.  This allows the user to quickly set or clear the
> NVME_CTRL_MARGINAL state from sysfs.

[Severity: Medium]
Does the commit message match the implemented code?

The message explicitly states that nvme_fc_modify_rport_fpin_state() is
called, but the code relies on the generic transport template callback
set_rport_marginal() instead.

> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index df1edf3c23fa8..af309a7bdc221 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
[ ... ]
> @@ -1251,34 +1251,58 @@ static ssize_t fc_rport_set_marginal_state(struct device *dev,
[ ... ]
> +	spin_lock_irqsave(shost->host_lock, flags);
> +
> +	switch (port_state) {
> +	case FC_PORTSTATE_MARGINAL:
>  		/*
>  		 * Change the state to Marginal only if the
>  		 * current rport state is Online
>  		 * Allow only Online->Marginal
>  		 */
> -		if (rport->port_state == FC_PORTSTATE_ONLINE)
> +		if (rport->port_state == FC_PORTSTATE_ONLINE) {
>  			rport->port_state = port_state;
> -		else if (port_state != rport->port_state)
> -			return -EINVAL;
> -	} else if (port_state == FC_PORTSTATE_ONLINE) {
> +			spin_unlock_irqrestore(shost->host_lock, flags);
> +			if (i->f->set_rport_marginal)
> +				i->f->set_rport_marginal(rport, true);
> +			return count;

[Severity: High]
Could this sequence introduce a regression where the transport layer state
becomes permanently desynchronized from the hardware state?

By dropping the host_lock before calling set_rport_marginal(), a race window
is opened. If one thread sets the state to FC_PORTSTATE_MARGINAL and drops
the lock, and another thread concurrently sets the state to
FC_PORTSTATE_ONLINE, drops the lock, and executes set_rport_marginal() with
false, the first thread might then execute its callback with true. The
transport software state would be online, but the hardware state would be left
in a marginal state.

> +		}
> +		break;
> +
> +	case FC_PORTSTATE_ONLINE:
>  		/*
>  		 * Change the state to Online only if the
>  		 * current rport state is Marginal
>  		 * Allow only Marginal->Online
>  		 */
> -		if (rport->port_state == FC_PORTSTATE_MARGINAL)
> +		if (rport->port_state == FC_PORTSTATE_MARGINAL) {
>  			rport->port_state = port_state;
> -		else if (port_state != rport->port_state)
> -			return -EINVAL;
> -	} else
> +			spin_unlock_irqrestore(shost->host_lock, flags);
> +			if (i->f->set_rport_marginal)
> +				i->f->set_rport_marginal(rport, false);
> +			return count;

[Severity: High]
Is this code also vulnerable to the same regression? Dropping the lock prior
to executing set_rport_marginal() permits concurrent sysfs writes to reorder
the callback executions, which can lead to a state mismatch.

> +		}
> +		break;

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