Re: [PATCH v2] scsi: scsi_transport_sas: fix error handling in sas_rphy_add()

[James Bottomley <jejb@linux.ibm.com>]

On Fri, 2022-11-11 at 22:44 +0800, Yang Yingliang wrote:
> In sas_rphy_add(), if transport_add_device() fails, the device
> is not added, the return value is not checked, it won't goto
> error path, when removing rphy in normal remove path, it causes
> null-ptr-deref, because transport_remove_device() is called to
> remove the device that was not added.
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000108
> 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_rphy_remove+0x50/0x78 [scsi_transport_sas]
>  sas_port_delete+0x30/0x148 [scsi_transport_sas]
>  do_sas_phy_delete+0x78/0x80 [scsi_transport_sas]
>  device_for_each_child+0x68/0xb0
>  sas_remove_children+0x30/0x50 [scsi_transport_sas]
>  sas_rphy_remove+0x38/0x78 [scsi_transport_sas]
>  sas_port_delete+0x30/0x148 [scsi_transport_sas]
>  do_sas_phy_delete+0x78/0x80 [scsi_transport_sas]
>  device_for_each_child+0x68/0xb0
>  sas_remove_children+0x30/0x50 [scsi_transport_sas]
>  sas_remove_host+0x20/0x38 [scsi_transport_sas]
>  scsih_remove+0xd8/0x420 [mpt3sas]
> 
> Fix this by checking and handling return value of
> transport_add_device()
> in sas_rphy_add().
> 
> Fixes: c7ebbbce366c ("[SCSI] SAS transport class")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v1 -> v2:
>   Update commit message.
> ---
>  drivers/scsi/scsi_transport_sas.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c
> index 74b99f2b0b74..accc0afa8f77 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1526,7 +1526,11 @@ int sas_rphy_add(struct sas_rphy *rphy)
>         error = device_add(&rphy->dev);
>         if (error)
>                 return error;
> -       transport_add_device(&rphy->dev);
> +       error = transport_add_device(&rphy->dev);
> +       if (error) {
> +               device_del(&rphy->dev);
> +               return error;
> +       }
>         transport_configure_device(&rphy->dev);
>         if (sas_bsg_initialize(shost, rphy))
>                 printk("fail to a bsg device %s\n", dev_name(&rphy-
> >dev));

There is a slight problem with doing this in that if
transport_device_add() ever fails it's likely because memory pressure
caused the allocation of the internal_container to fail. What that
means is that the visible sysfs attributes don't get added, but
otherwise the rphy is fully functional as far as the driver sees it, so
this condition doesn't have to be a fatal error which kills the device.

There are two ways of handling this:

   1. The above to move the condition from an ignored to a fatal error.
      It's so rare that we almost never see it in practice and if it
      ever happened, the machine is so low on memory that something
      else is bound to fail an allocation and kill the device anyway,
      so treating it as non-fatal likely serves no purpose.
   2. Simply to make the assumption that transport_remove_device() is
      idempotent true by adding a flag in the internal_class to signify
      removal is required. This would preserve current behaviour and
      have the bonus that it only requires a single patch, not one
      patch per transport class object that has this problem.

I'd probably prefer 2. since it's way less work, but others might have
different opinions.

James