Re: [PATCH v4 1/3] scsi: libsas: Handle errors in sas_ex_add_parent_port()

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

Pre-existing issues:
- [High] Pre-existing lack of error handling causes a kernel panic on port allocation or addition failure during edge/fanout expander discovery.
--

--- Patch [1]: [PATCH v4 1/3] scsi: libsas: Handle errors in sas_ex_add_parent_port() ---
commit bc14b1e6a1d6e30259a7082de185251e075a15fc
Author: Eshaan Deshmukh <[email protected]>

    scsi: libsas: Handle errors in sas_ex_add_parent_port()

This commit changes sas_ex_add_parent_port() to properly handle failures from
sas_port_alloc() and sas_port_add() by returning error codes rather than using
BUG_ON(). It also updates sas_ex_discover_dev() to check and return these new
error codes.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f471ab464a78b..f1a1417b09819 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -33,19 +33,29 @@ static void sas_port_add_ex_phy(struct sas_port *port, struct ex_phy *ex_phy)
>  	ex_phy->phy_state = PHY_DEVICE_DISCOVERED;
>  }
>  
> -static void sas_ex_add_parent_port(struct domain_device *dev, int phy_id)
> +static int sas_ex_add_parent_port(struct domain_device *dev, int phy_id)
>  {
>  	struct expander_device *ex = &dev->ex_dev;
>  	struct ex_phy *ex_phy = &ex->ex_phy[phy_id];
> +	int error;
>  
>  	if (!ex->parent_port) {
>  		ex->parent_port = sas_port_alloc(&dev->rphy->dev, phy_id);
> -		/* FIXME: error handling */
> -		BUG_ON(!ex->parent_port);
> -		BUG_ON(sas_port_add(ex->parent_port));
> +		if (!ex->parent_port)
> +			return -ENOMEM;
> +
> +		error = sas_port_add(ex->parent_port);
> +		if (error) {
> +			sas_port_free(ex->parent_port);
> +			ex->parent_port = NULL;
> +			return error;
> +		}
> +
>  		sas_port_mark_backlink(ex->parent_port);
>  	}
>  	sas_port_add_ex_phy(ex->parent_port, ex_phy);
> +
> +	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since this patch updates the
error handling for sas_port_alloc() and sas_port_add(), could the identical
pattern remaining in sas_ex_discover_expander() be addressed as well?

drivers/scsi/libsas/sas_expander.c:sas_ex_discover_expander() {
    ...
	child = sas_alloc_device();
	if (!child)
		return NULL;

	phy->port = sas_port_alloc(&parent->rphy->dev, phy_id);
	/* FIXME: better error handling */
	BUG_ON(sas_port_add(phy->port) != 0);
    ...
}

If sas_port_alloc() fails under memory pressure during edge or fanout expander
discovery, it returns a NULL pointer. This NULL pointer is directly passed to
sas_port_add(), leading to a NULL pointer dereference and a kernel panic.
Additionally, if sas_port_add() fails, the BUG_ON() triggers a kernel panic. 

Would it make sense to implement graceful error handling here as well instead
of crashing the system?

[ ... ]

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