Re: [PATCH v9 2/2] scsi: libsas: Add linkrate and sas_addr change detection in rediscover

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch attempts to fix a Use-After-Free by moving `sas_ex_phy_discover()` before the `child_dev` lookup, but `sas_ex_to_dev()` still returns an unprotected, unreferenced pointer, leaving a lockless TOCTOU / UAF race.
--

commit c64b715b8d5d34842e349c246a7de0dc1188f077
Author: Xingui Yang <yangxingui@huawei.com>

scsi: libsas: Add linkrate and sas_addr change detection in rediscover

Introduce sas_dev_is_flutter() and sas_rediscover_ex_phy() to improve
flutter and device replace detection during rediscovery.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index fc6d8f3c9dcae..e27953de2b4e2 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c

[ ... ]

> +static bool sas_dev_is_flutter(struct domain_device *dev, int phy_id,
> +			       u8 *sas_addr, enum sas_device_type type)
> +{
> +	struct expander_device *ex = &dev->ex_dev;
> +	struct ex_phy *phy = &ex->ex_phy[phy_id];
> +	struct domain_device *child_dev;
> +	char *action = "";
> +	int res;
> +
> +	if (SAS_ADDR(sas_addr) != SAS_ADDR(phy->attached_sas_addr) ||
> +	    !dev_type_flutter(type, phy->attached_dev_type))
> +		return false;
> +
> +	res = sas_ex_phy_discover(dev, phy_id);
> +	if (res)
> +		return false;
> +
> +	child_dev = sas_ex_to_dev(dev, phy_id);
> +	if (!child_dev)
> +		goto out;
> +
> +	if (dev_is_sata(child_dev) &&
> +	    phy->attached_dev_type == SAS_SATA_PENDING) {

[Severity: High]
This is a pre-existing issue, but does sas_ex_to_dev() still leave us open
to a use-after-free race here?

It looks like sas_ex_to_dev() calls sas_find_dev_by_rphy() and returns the
unreferenced device pointer without incrementing its reference count via
kref_get().

If a hardware link drop event occurs concurrently, the event workqueue
could trigger sas_deform_port(). Since sas_deform_port() doesn't acquire
the disco_mutex before calling sas_unregister_domain_devices() and
sas_destruct_devices(), it can tear down the device and free its memory.

Because sas_deform_port() only flushes the disco_q after tearing down the
devices, could child_dev be freed by the event workqueue before we
dereference it here in dev_is_sata() or in the sas_addr check below?

> +		action = ", needs recovery";
> +		goto out;
> +	}
> +
> +	if (SAS_ADDR(child_dev->sas_addr) != SAS_ADDR(phy->attached_sas_addr)) {
> +		pr_info("ex %016llx phy%02d sas_addr changed from %016llx to %016llx\n",
> +			SAS_ADDR(dev->sas_addr), phy_id,
> +			SAS_ADDR(child_dev->sas_addr),
> +			SAS_ADDR(phy->attached_sas_addr));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624063230.3264029-1-yangxingui@huawei.com?part=2