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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
.



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux