Re: [PATCH v3 2/3] scsi: libsas: Handle errors in sas_set_ex_phy()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning -EBUSY when SAS_HA_ATA_EH_ACTIVE is set aborts the entire expander discovery.

Pre-existing issues:
- [High] Setting phy->phy = NULL on sas_phy_add failure without cleaning up phy_state leads to a NULL pointer dereference in sas_ex_get_linkrate.
--

--- Patch [2]: [PATCH v3 2/3] scsi: libsas: Handle errors in sas_set_ex_phy() ---
commit bb6067e9bf28e3d0ad6e9081b09eb33b9747146c
Author: Eshaan Deshmukh <[email protected]>

scsi: libsas: Handle errors in sas_set_ex_phy()

The function sas_set_ex_phy() uses BUG_ON() if sas_phy_alloc() fails.
Change it to return -ENOMEM error code if that happens. Change all the
other returns in sas_set_ex_phy() to return 0 to preserve current
functionality, or to return an error. Change all uses of sas_set_ex_phy
to handle the returned error codes.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f1a1417b09819..ab6afbad37feb 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
[ ... ]
> @@ -220,14 +220,15 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
>  	struct sas_rphy *rphy = dev->rphy;
>  	bool new_phy = !phy->phy;
>  	char *type;
> +	int error;
>  
>  	if (new_phy) {
>  		if (WARN_ON_ONCE(test_bit(SAS_HA_ATA_EH_ACTIVE, &ha->state)))
> -			return;
> +			return -EBUSY;

[Severity: High]
Will returning -EBUSY here abort the entire expander discovery?

Previously, this returned void, allowing the discovery loop to continue.
If a transient ATA error coincides with an expander hotplug event, this
-EBUSY error propagates up through sas_ex_phy_discover_helper() to
sas_discover_domain(), where it causes the newly discovered expander device
to be destroyed via sas_rphy_free().

Since sas_discover_domain() does not defer on ATA EH, could a transient
ATA error permanently fail the discovery of a SAS expander?

>  		phy->phy = sas_phy_alloc(&rphy->dev, phy_id);
>  
> -		/* FIXME: error_handling */
> -		BUG_ON(!phy->phy);
> +		if (!phy->phy)
> +			return -ENOMEM;
>  	}
>  
[ ... ]
> @@ -296,11 +297,14 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
>  	phy->phy->enabled = (phy->linkrate != SAS_PHY_DISABLED);
>  
>   skip:
> -	if (new_phy)
> -		if (sas_phy_add(phy->phy)) {
> +	if (new_phy) {
> +		error = sas_phy_add(phy->phy);
> +		if (error) {
>  			sas_phy_free(phy->phy);
> -			return;
> +			phy->phy = NULL;
> +			return error;
>  		}
> +	}

[Severity: High]
This is a pre-existing issue, but does setting phy->phy to NULL here on
sas_phy_add() failure lead to a NULL pointer dereference later?

If device registration fails, phy->phy is freed and set to NULL, but
phy->phy_state remains PHY_EMPTY and attached_sas_addr is still populated.

If this error is ignored during revalidation, later operations like
sas_ex_get_linkrate() might iterate over the phys, match the address, and
pass the NULL phy->phy to sas_port_add_phy():

sas_ex_get_linkrate()
   if (sas_phy_match_dev_addr(child, phy)) {
      ...
      sas_port_add_phy(port, phy->phy);

sas_port_add_phy() will then unconditionally dereference the NULL phy pointer
when checking the port_siblings list.

Should the ex_phy array be scrubbed (e.g., reverting phy_state to
PHY_VACANT) when registration fails to prevent this?

[ ... ]

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