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

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

[Haoxiang Li]

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@163.com>
---
 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;
+	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();
 	}
+	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)
-- 
2.25.1

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `sas_rphy_add()` is ignored in `sas_ex_discover_expander()`, leading to incomplete sysfs registration and kernel warnings during device teardown on error.
--

commit 89b8642b2949851837e884780fd3067fcff48014
Author: Haoxiang Li <haoxiang_li2024@163.com>

scsi: libsas: Handle expander discovery allocation failures

This commit adds error handling for sas_port_alloc(), sas_port_add(), and
sas_expander_alloc() in sas_ex_discover_expander(). It unwinds the
allocated resources to prevent dereferencing uninitialized or NULL pointers.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f471ab464a78b..56c04c4ae8181 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c

[ ... ]

> @@ -926,6 +928,9 @@ static struct domain_device *sas_ex_discover_expander(
>  		rphy = NULL;	/* shut gcc up */
>  		BUG();
>  	}
> +	if (!rphy)
> +		goto out_delete_port;
> +

[Severity: High]
This isn't a bug introduced by this patch, but further down in
sas_ex_discover_expander(), the return value of sas_rphy_add() is ignored:

	sas_init_dev(child);
	sas_fill_in_rphy(child, rphy);
	sas_rphy_add(rphy);

If sas_rphy_add() fails (for example, if device_add() fails under memory
pressure), the remote phy is not registered with sysfs.

When the device is eventually removed, or if discovery fails later in this
function, sas_rphy_delete() is unconditionally called:

	res = sas_discover_expander(child);
	if (res) {
		sas_rphy_delete(rphy);

Because sas_rphy_delete() calls device_del(), doing this on a device that
failed device_add() violates the driver core API.

Can this trigger a kernel WARN_ON or oops during device teardown? Could we
check the return value of sas_rphy_add() and handle the registration failure
along with these other resource allocation failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623112909.2172701-1-haoxiang_li2024@163.com?part=1

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@163.com>

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