Re: [PATCH] scsi: libsas: fix error handling in sas_phy_add()

[John Garry <john.g.garry@xxxxxxxxxx>]

On 05/11/2022 07:17, Yang Yingliang wrote:

This is not libsas.

BTW, before I go further, note this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n312

> If transport_add_device() fails in sas_phy_add(), but it's not handled,
> it will lead kernel crash because of trying to delete not added device
> in transport_remove_device() called from sas_remove_host().
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000108
> CPU: 61 PID: 42829 Comm: rmmod Kdump: loaded Tainted: G        W          6.1.0-rc1+ #173
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : device_del+0x54/0x3d0
> lr : device_del+0x37c/0x3d0
> Call trace:
>   device_del+0x54/0x3d0
>   attribute_container_class_device_del+0x28/0x38
>   transport_remove_classdev+0x6c/0x80
>   attribute_container_device_trigger+0x108/0x110
>   transport_remove_device+0x28/0x38
>   sas_phy_delete+0x30/0x60 [scsi_transport_sas]
>   do_sas_phy_delete+0x6c/0x80 [scsi_transport_sas]
>   device_for_each_child+0x68/0xb0
>   sas_remove_children+0x40/0x50 [scsi_transport_sas]
>   sas_remove_host+0x20/0x38 [scsi_transport_sas]
>   hisi_sas_remove+0x40/0x68 [hisi_sas_main]
>   hisi_sas_v2_remove+0x20/0x30 [hisi_sas_v2_hw]
>   platform_remove+0x2c/0x60
>
> Fix this by checking and handling return value of transport_add_device()
> in sas_phy_add(). transport_destroy_device() has been called in sas_phy_free()
> in the error path, so it's no need to call it here.

"there's no need", rather than "it's no need". And I don't know why you 
bother even mentioning about transport_destroy_device().

> Fixes: c7ebbbce366c ("[SCSI] SAS transport class")
> Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
> ---
>   drivers/scsi/scsi_transport_sas.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 2f88c61216ee..cb364a7c6097 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -723,8 +723,11 @@ int sas_phy_add(struct sas_phy *phy)
>   
>   	error = device_add(&phy->dev);
>   	if (!error) {

personally I think that the following looks better:

int sas_phy_add(struct sas_phy *phy)
{
	int error;

	error = device_add(&phy->dev);
	if (error)
		return error;

	error = transport_add_device(&phy->dev);
	if (error) {
		device_del(&phy->dev);
		return error;
	}
	transport_configure_device(&phy->dev);

	return 0;
}
EXPORT_SYMBOL(sas_phy_add);

> -		transport_add_device(&phy->dev);
> -		transport_configure_device(&phy->dev);
> +		error = transport_add_device(&phy->dev);
> +		if (!error)
> +			transport_configure_device(&phy->dev);
> +		else
> +			device_del(&phy->dev);
>   	}
>   
>   	return error;