Re: [PATCH] scsi: libsas: Handle expander discovery allocation failures

[Damien Le Moal]

On 6/23/26 20:29, Haoxiang Li wrote:
> sas_ex_discover_expander() allocates a domain device and SAS port before
> allocating the expander rphy, but it does not check all allocation and
> registration failures. In particular, sas_expander_alloc() can return
> NULL and the returned rphy is dereferenced unconditionally.
> 
> Add error handling for sas_port_alloc(), sas_port_add(), and
> sas_expander_alloc(), and unwind the resources allocated on each path.
> Use sas_port_free() before a port has been added and sas_port_delete()
> after it has been added.
> 
> Free the child device directly on these early failures because child->rphy
> has not been initialized yet, and sas_put_device() would dereference it.
> 
> Signed-off-by: Haoxiang Li <haoxiang_li2024@xxxxxxx>

Looks OK. A couple of nits below.

> ---
>  drivers/scsi/libsas/sas_expander.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f471ab464a78..56c04c4ae818 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -909,9 +909,11 @@ static struct domain_device *sas_ex_discover_expander(
>  		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 (!phy->port)
> +		goto out_free_child;

For readability, a blank line would be nice here.

> +	res = sas_port_add(phy->port);
> +	if (res)
> +		goto out_free_port;
>  
>  	switch (phy->attached_dev_type) {
>  	case SAS_EDGE_EXPANDER_DEVICE:
> @@ -926,6 +928,9 @@ static struct domain_device *sas_ex_discover_expander(
>  		rphy = NULL;	/* shut gcc up */
>  		BUG();

can we drop this BUG() too so that instead of crashing we properly error unwind?

>  	}
> +	if (!rphy)
> +		goto out_delete_port;
> +
>  	port = parent->port;
>  	child->rphy = rphy;
>  	get_device(&rphy->dev);
> @@ -963,6 +968,19 @@ static struct domain_device *sas_ex_discover_expander(
>  	}
>  	list_add_tail(&child->siblings, &parent->ex_dev.children);
>  	return child;
> +
> +out_delete_port:
> +	sas_port_delete(phy->port);
> +	phy->port = NULL;
> +	kfree(child);
> +	return NULL;
> +
> +out_free_port:
> +	sas_port_free(phy->port);
> +	phy->port = NULL;
> +out_free_child:
> +	kfree(child);
> +	return NULL;
>  }
>  
>  static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)


-- 
Damien Le Moal
Western Digital Research