[PATCH v3 0/3] scsi: libsas: Handle errors in 3 functions

[PATCH v3 0/3] scsi: libsas: Handle errors in 3 functions

[Eshaan Deshmukh]

These 3 functions include several uses of BUG_ON() and lack error
management. Change all 3 functions to handle their errors in better
ways.

Eshaan Deshmukh (3):
  scsi: libsas: Handle errors in sas_ex_add_parent_port()
  scsi: libsas: Handle errors in sas_set_ex_phy()
  scsi: libsas: Handle errors in  sas_ex_discover_expander()

 drivers/scsi/libsas/sas_expander.c | 99 ++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 34 deletions(-)

-- 
2.55.0

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

[Eshaan Deshmukh]

The function sas_ex_add_parent_port() uses BUG_ON() if sas_port_alloc()
or sas_port_add() fails. Change sas_ex_add_parent_port() to return error
codes to the caller and free the sas_port_alloc() allocation. Also
change sas_ex_discover_dev() to handle errors returned by
sas_ex_add_parent_port().

Signed-off-by: Eshaan Deshmukh <eshaan2031@icloud.com>
---
 drivers/scsi/libsas/sas_expander.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f471ab464..f1a1417b0 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;
 }
 
 /* ---------- SMP task management ---------- */
@@ -981,12 +991,14 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 	}
 
 	/* Parent and domain coherency */
-	if (!dev->parent && sas_phy_match_port_addr(dev->port, ex_phy)) {
-		sas_ex_add_parent_port(dev, phy_id);
-		return 0;
-	}
+	if (!dev->parent && sas_phy_match_port_addr(dev->port, ex_phy))
+		return sas_ex_add_parent_port(dev, phy_id);
+
 	if (dev->parent && sas_phy_match_dev_addr(dev->parent, ex_phy)) {
-		sas_ex_add_parent_port(dev, phy_id);
+		res = sas_ex_add_parent_port(dev, phy_id);
+		if (res)
+			return res;
+
 		if (ex_phy->routing_attr == TABLE_ROUTING)
 			sas_configure_phy(dev, phy_id, dev->port->sas_addr, 1);
 		return 0;
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked return value of `sas_port_alloc()` in `sas_ex_discover_expander()` leads to a NULL pointer dereference and kernel panic.
--

--- Patch [1]: [PATCH v3 1/3] scsi: libsas: Handle errors in sas_ex_add_parent_port() ---
commit b53b16ccede6280045a463721b18531d23ff3fb8
Author: Eshaan Deshmukh <eshaan2031@icloud.com>

scsi: libsas: Handle errors in sas_ex_add_parent_port()

This patch changes sas_ex_add_parent_port() to return error codes instead
of using BUG_ON() when sas_port_alloc() or sas_port_add() fail. It also
updates sas_ex_discover_dev() to handle these returned errors.

> 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;

[Severity: High]
This is a pre-existing issue, but since this patch fixes the missing error
handling here, does a similar problem exist in sas_ex_discover_expander()
in drivers/scsi/libsas/sas_expander.c?

It looks like sas_port_alloc() is called and its result is passed directly to
sas_port_add() without a NULL check:

    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 due to memory exhaustion and returns NULL, would
passing this NULL pointer to sas_port_add() immediately dereference it while
checking list_empty(&port->phy_list), resulting in a kernel panic?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812194808.576586-1-eshaan2031@icloud.com?part=1

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

[Eshaan Deshmukh]

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.

Signed-off-by: Eshaan Deshmukh <eshaan2031@icloud.com>
---
 drivers/scsi/libsas/sas_expander.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f1a1417b0..ab6afbad3 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -207,7 +207,7 @@ static enum sas_device_type to_dev_type(struct discover_resp *dr)
 		return dr->attached_dev_type;
 }
 
-static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
+static int sas_set_ex_phy(struct domain_device *dev, int phy_id,
 			   struct smp_disc_resp *disc_resp)
 {
 	enum sas_device_type dev_type;
@@ -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;
 		phy->phy = sas_phy_alloc(&rphy->dev, phy_id);
 
-		/* FIXME: error_handling */
-		BUG_ON(!phy->phy);
+		if (!phy->phy)
+			return -ENOMEM;
 	}
 
 	switch (disc_resp->result) {
@@ -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;
 		}
+	}
 
  out:
 	switch (phy->attached_dev_type) {
@@ -339,7 +343,7 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
 	    SAS_ADDR(phy->attached_sas_addr) != SAS_ADDR(sas_addr))
 		/* pass */;
 	else
-		return;
+		return 0;
 
 	/* if the attached device type changed and ata_eh is active,
 	 * make sure we run revalidation when eh completes (see:
@@ -353,6 +357,8 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
 		 SAS_ADDR(dev->sas_addr), phy->phy_id,
 		 sas_route_char(dev, phy), phy->linkrate,
 		 SAS_ADDR(phy->attached_sas_addr), type);
+
+	return 0;
 }
 
 /* check if we have an existing attached ata device on this expander phy */
@@ -397,8 +403,8 @@ static int sas_ex_phy_discover_helper(struct domain_device *dev, u8 *disc_req,
 		pr_notice("Found loopback topology, just ignore it!\n");
 		return 0;
 	}
-	sas_set_ex_phy(dev, single, disc_resp);
-	return 0;
+
+	return sas_set_ex_phy(dev, single, disc_resp);
 }
 
 int sas_ex_phy_discover(struct domain_device *dev, int single)
@@ -2021,7 +2027,7 @@ static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
 		 * the PHY info, like negotiated linkrate.
 		 */
 		if (res == 0)
-			sas_set_ex_phy(dev, phy_id, disc_resp);
+			res = sas_set_ex_phy(dev, phy_id, disc_resp);
 		goto out_free_resp;
 	} else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
 		   dev_type_flutter(type, phy->attached_dev_type)) {
-- 
2.55.0

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

[sashiko-bot]

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 <eshaan2031@icloud.com>

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/20260812194808.576586-1-eshaan2031@icloud.com?part=2

[PATCH v3 3/3] scsi: libsas: Handle errors in sas_ex_discover_expander()

[Eshaan Deshmukh]

The function sas_ex_discover_expander() does not account for the
potential failure of sas_port_alloc() for phy->port. It also calls
BUG_ON in case sas_port_add fails for phy->port. Add a check for
phy->port after sas_port_alloc() where if it is NULL, it cleans up the
child allocated device and returns NULL. Add another check for
sas_port_add() where if it returns an error code it frees phy->port,
sets it to NULL, cleans up the child allocated device, and returns NULL.

Signed-off-by: Eshaan Deshmukh <eshaan2031@icloud.com>
---
 drivers/scsi/libsas/sas_expander.c | 41 ++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index ab6afbad3..a83493f57 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -911,7 +911,6 @@ static struct domain_device *sas_ex_discover_expander(
 	struct sas_rphy *rphy;
 	struct sas_expander_device *edev;
 	struct asd_sas_port *port;
-	int res;
 
 	if (phy->routing_attr == DIRECT_ROUTING) {
 		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
@@ -925,9 +924,13 @@ 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_put_device;
+	}
 
+	if (sas_port_add(phy->port)) {
+		goto out_free_port;
+	}
 
 	switch (phy->attached_dev_type) {
 	case SAS_EDGE_EXPANDER_DEVICE:
@@ -966,19 +969,29 @@ static struct domain_device *sas_ex_discover_expander(
 	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
 	spin_unlock_irq(&parent->port->dev_list_lock);
 
-	res = sas_discover_expander(child);
-	if (res) {
-		sas_rphy_delete(rphy);
-		spin_lock_irq(&parent->port->dev_list_lock);
-		list_del(&child->dev_list_node);
-		spin_unlock_irq(&parent->port->dev_list_lock);
-		sas_put_device(child);
-		sas_port_delete(phy->port);
-		phy->port = NULL;
-		return NULL;
-	}
+	if (sas_discover_expander(child))
+		goto out_delete_rphy;
+
 	list_add_tail(&child->siblings, &parent->ex_dev.children);
 	return child;
+
+out_free_port:
+	sas_port_free(phy->port);
+	phy->port = NULL;
+
+out_put_device:
+	sas_put_device(child);
+	return NULL;
+
+out_delete_rphy:
+	sas_rphy_delete(rphy);
+	spin_lock_irq(&parent->port->dev_list_lock);
+	list_del(&child->dev_list_node);
+	spin_unlock_irq(&parent->port->dev_list_lock);
+	sas_put_device(child);
+	sas_port_delete(phy->port);
+	phy->port = NULL;
+	return NULL;
 }
 
 static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing check for `sas_expander_alloc()` failure leads to an invalid pointer dereference and kernel panic.
- [High] Ignoring the return value of `sas_rphy_add()` leads to a kernel panic upon teardown.
--

--- Patch [3]: [PATCH v3 3/3] scsi: libsas: Handle errors in  sas_ex_discover_expander() ---
commit ae2723570a72c0c97de94806125a00a3b79e17cd
Author: Eshaan Deshmukh <eshaan2031@icloud.com>

scsi: libsas: Handle errors in sas_ex_discover_expander()
    
This commit improves error handling in sas_ex_discover_expander() by checking
the return values of sas_port_alloc() and sas_port_add(). If either function
fails, it cleans up the child device and port allocations before returning.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index ab6afbad37feb..a83493f574c30 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -911,7 +911,6 @@ static struct domain_device *sas_ex_discover_expander(
>  	struct sas_rphy *rphy;
>  	struct sas_expander_device *edev;
>  	struct asd_sas_port *port;
> -	int res;
>  
>  	if (phy->routing_attr == DIRECT_ROUTING) {
>  		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
> @@ -925,9 +924,13 @@ 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_put_device;
> +	}
>  
> +	if (sas_port_add(phy->port)) {
> +		goto out_free_port;
> +	}
>  
>  	switch (phy->attached_dev_type) {
>  	case SAS_EDGE_EXPANDER_DEVICE:

[Severity: High]
This is a pre-existing issue, but can this lead to an invalid pointer
dereference?

If sas_expander_alloc() fails under memory pressure and returns NULL, it is
assigned to rphy in the switch statement below:

sas_ex_discover_expander() {
    ...
    case SAS_EDGE_EXPANDER_DEVICE:
        rphy = sas_expander_alloc(phy->port, SAS_EDGE_EXPANDER_DEVICE);
    ...
}

The code later blindly offsets and dereferences this:

sas_ex_discover_expander() {
    ...
    edev = rphy_to_expander_device(rphy);
    ...
    edev->level = parent_ex->level + 1;
    ...
}

Since rphy is not at offset 0 within struct sas_expander_device,
container_of creates an invalid pointer for edev. Dereferencing it
causes a kernel panic. Should we check if rphy is NULL before continuing?

[ ... ]

> @@ -966,19 +969,29 @@ static struct domain_device *sas_ex_discover_expander(
>  	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
>  	spin_unlock_irq(&parent->port->dev_list_lock);
>  
> -	res = sas_discover_expander(child);
> -	if (res) {
> -		sas_rphy_delete(rphy);
> -		spin_lock_irq(&parent->port->dev_list_lock);
> -		list_del(&child->dev_list_node);
> -		spin_unlock_irq(&parent->port->dev_list_lock);
> -		sas_put_device(child);
> -		sas_port_delete(phy->port);
> -		phy->port = NULL;
> -		return NULL;
> -	}
> +	if (sas_discover_expander(child))
> +		goto out_delete_rphy;
> +

[Severity: High]
This is a pre-existing issue, but can ignoring the return value of
sas_rphy_add() cause a crash during teardown?

Earlier in this function, sas_rphy_add(rphy) is called without checking if it
was successful. If it fails, device_add() cleans up and sets dev->p to NULL.

If a subsequent function like sas_discover_expander(child) fails here, the
error path goes to out_delete_rphy:

sas_ex_discover_expander() {
    ...
    out_delete_rphy:
        sas_rphy_delete(rphy);
    ...
}

This path unconditionally calls device_del() inside sas_rphy_delete().
Calling device_del() on a device that was never successfully added will
dereference the NULL dev->p, causing a panic. Does sas_rphy_add() need
error handling?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812194808.576586-1-eshaan2031@icloud.com?part=3