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

[Yang Yingliang <yangyingliang@huawei.com>]

On 2022/11/18 17:18, John Garry wrote:
> On 18/11/2022 03:11, Yang Yingliang wrote:
>>>>> );
>>> 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.
>> Current some callers ignore the return value of 
>> transport_add_device(), if it fails,
>> it will cause null-ptr-deref in transport_remove_device().
>>
>> James suggested that add some check in transport_remove_device(), so 
>> all can
>> be fix in one patch.
>>
>> Do you have any suggestion for this ?
>
> Personally I prefer 1. However did you develop a prototype patch for 
> how 2. would look? And how many changes are still required for 1.?
For 1, in total, there are 8 places need be checked
in drivers/scsi/scsi_transport_sas.c, 2 places
in drivers/scsi/scsi_sysfs.c, 3 places
in drivers/scsi/scsi_transport_fc.c, 2 places
in drivers/scsi/scsi_transport_srp.c, 1 place

For 2, I think we can use device_is_registered() to check if add 
operation is successful, may be like this (not test yet):

diff --git a/drivers/base/transport_class.c b/drivers/base/transport_class.c
index ccc86206e508..ac41be7b724e 100644
--- a/drivers/base/transport_class.c
+++ b/drivers/base/transport_class.c
@@ -227,9 +227,11 @@ static int transport_remove_classdev(struct 
attribute_container *cont,
          tclass->remove(tcont, dev, classdev);

      if (tclass->remove != anon_transport_dummy_function) {
-        if (tcont->statistics)
-            sysfs_remove_group(&classdev->kobj, tcont->statistics);
-        attribute_container_class_device_del(classdev);
+        if (device_is_registered(classdev)) {
+            if (tcont->statistics)
+                sysfs_remove_group(&classdev->kobj, tcont->statistics);
+            attribute_container_class_device_del(classdev);
+        }
      }

      return 0;

Thanks,
Yang
>
> Thanks,
> John
> .