fixes for adt7410

PATCH In review Hartmut Knaack 2 participants 5 messages Jul 15, 2012 – Jul 16, 2012 IIO

lore

Share & tools

lore

Hartmut Knaack

author

2012-07-15 20:40 UTC #0 lore raw

PATCH 1/2 fixes for adt7410

This patch adds some checks prior to accessing device attribute lists in industrialio-event.c.

Signed-off-by Harmut Knaack <[email protected]>

---

drivers/iio/industrialio-event.c +11 -7

index b49059d..fe68e90 100644
@@ -425,11 +425,13 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
                *attrcount_orig);
     attrn = attrcount_orig;
     /* Add all elements from the list. */
-    list_for_each_entry(p,
-                &indio_dev->event_interface->dev_attr_list,
-                l)
-        indio_dev->event_interface->group.attrs[attrn++] =
-            &p->dev_attr.attr;
+    if (indio_dev->event_interface->dev_attr_list.next)
+        list_for_each_entry(p,
+                &indio_dev->event_interface->dev_attr_list,
+                l)
+            indio_dev->event_interface->group.attrs[attrn++] =
+                &p->dev_attr.attr;
+
     indio_dev->groups[indio_dev->groupcounter++] =
         &indio_dev->event_interface->group;

@@ -447,7 +449,9 @@ void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 {
     if (indio_dev->event_interface == NULL)
         return;
-    __iio_remove_event_config_attrs(indio_dev);
-    kfree(indio_dev->event_interface->group.attrs);
+    if (indio_dev->event_interface->dev_attr_list.next)
+        __iio_remove_event_config_attrs(indio_dev);
+    if (indio_dev->event_interface->group.attrs)
+        kfree(indio_dev->event_interface->group.attrs);
     kfree(indio_dev->event_interface);
 }

Lars-Peter Clausen

2012-07-15 20:59 UTC #1 lore raw

Quoted 9 lines · Hartmut Knaack

This patch adds some checks prior to accessing device attribute lists in industrialio-event.c.
Signed-off-by Harmut Knaack <[email protected]>
---
drivers/iio/industrialio-event.c +0 -0
index b49059d..fe68e90 100644

drivers/iio/industrialio-event.c int iio_device_register_eventset(struct iio_dev *indio_dev) · line 428

Quoted 9 lines

drivers/iio/industrialio-event.c +1 -5

@@ -425,11 +425,13 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
                *attrcount_orig);
     attrn = attrcount_orig;
     /* Add all elements from the list. */
-    list_for_each_entry(p,
-                &indio_dev->event_interface->dev_attr_list,
-                l)
-        indio_dev->event_interface->group.attrs[attrn++] =
-            &p->dev_attr.attr;
+    if (indio_dev->event_interface->dev_attr_list.next)

next should never be NULL. If it is this means that INIT_LIST_HEAD has not been called on the list and the correct fix is to make sure that it is called.

Quoted 10 lines

drivers/iio/industrialio-event.c +6 -0

+        list_for_each_entry(p,
+                &indio_dev->event_interface->dev_attr_list,
+                l)
+            indio_dev->event_interface->group.attrs[attrn++] =
+                &p->dev_attr.attr;
+
     indio_dev->groups[indio_dev->groupcounter++] =
         &indio_dev->event_interface->group;

drivers/iio/industrialio-event.c void iio_device_unregister_eventset(struct iio_dev *indio_dev) · line 453

Quoted 7 lines

drivers/iio/industrialio-event.c +2 -2

@@ -447,7 +449,9 @@ void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 {
     if (indio_dev->event_interface == NULL)
         return;
-    __iio_remove_event_config_attrs(indio_dev);
-    kfree(indio_dev->event_interface->group.attrs);
+    if (indio_dev->event_interface->dev_attr_list.next)
+        __iio_remove_event_config_attrs(indio_dev);

Same here.

drivers/iio/industrialio-event.c void iio_device_unregister_eventset(struct iio_dev *indio_dev) · line 455

Quoted 2 lines

drivers/iio/industrialio-event.c +2 -0

+    if (indio_dev->event_interface->group.attrs)
+        kfree(indio_dev->event_interface->group.attrs);

kfree does a check for NULL itself.

Quoted 7 lines

drivers/iio/industrialio-event.c +0 -1
     kfree(indio_dev->event_interface);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hartmut Knaack

author

2012-07-15 21:50 UTC #2 lore raw

Quoted 13 lines · Lars-Peter Clausen

Quoted 12 lines · Hartmut Knaack
This patch adds some checks prior to accessing device attribute lists in industrialio-event.c.
Signed-off-by Harmut Knaack <[email protected]>
---
drivers/iio/industrialio-event.c +0 -0
index b49059d..fe68e90 100644
@@ -425,11 +425,13 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
                *attrcount_orig);
     attrn = attrcount_orig;
     /* Add all elements from the list. */

Quoted 8 lines

drivers/iio/industrialio-event.c int iio_device_register_eventset(struct iio_dev *indio_dev) · line 428
Quoted 6 lines
drivers/iio/industrialio-event.c +1 -5
-    list_for_each_entry(p,
-                &indio_dev->event_interface->dev_attr_list,
-                l)
-        indio_dev->event_interface->group.attrs[attrn++] =
-            &p->dev_attr.attr;
+    if (indio_dev->event_interface->dev_attr_list.next)
next should never be NULL. If it is this means that INIT_LIST_HEAD has not been called on the list and the correct fix is to make sure that it is called.

Exactly. If the channels-attributes have not been set, INIT_LIST_HEAD (called through __iio_add_event_config_attrs) is not called. So, should we just stop instantiating the device if it does not have channel information?

Quoted 36 lines

Quoted 11 lines
drivers/iio/industrialio-event.c +6 -0
+        list_for_each_entry(p,
+                &indio_dev->event_interface->dev_attr_list,
+                l)
+            indio_dev->event_interface->group.attrs[attrn++] =
+                &p->dev_attr.attr;
+
     indio_dev->groups[indio_dev->groupcounter++] =
         &indio_dev->event_interface->group;
 
drivers/iio/industrialio-event.c void iio_device_unregister_eventset(struct iio_dev *indio_dev) · line 453
Quoted 7 lines
drivers/iio/industrialio-event.c +2 -2
@@ -447,7 +449,9 @@ void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 {
     if (indio_dev->event_interface == NULL)
         return;
-    __iio_remove_event_config_attrs(indio_dev);
-    kfree(indio_dev->event_interface->group.attrs);
+    if (indio_dev->event_interface->dev_attr_list.next)
+        __iio_remove_event_config_attrs(indio_dev);
Same here.
drivers/iio/industrialio-event.c void iio_device_unregister_eventset(struct iio_dev *indio_dev) · line 455
Quoted 2 lines
drivers/iio/industrialio-event.c +2 -0
+    if (indio_dev->event_interface->group.attrs)
+        kfree(indio_dev->event_interface->group.attrs);
kfree does a check for NULL itself.
Quoted 7 lines
drivers/iio/industrialio-event.c +0 -1
     kfree(indio_dev->event_interface);
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lars-Peter Clausen

2012-07-16 10:00 UTC #3 lore raw

Quoted 22 lines · Hartmut Knaack

Quoted 13 lines · Lars-Peter Clausen
Quoted 12 lines · Hartmut Knaack
This patch adds some checks prior to accessing device attribute lists in industrialio-event.c.
Signed-off-by Harmut Knaack <[email protected]>
---
drivers/iio/industrialio-event.c +0 -0
index b49059d..fe68e90 100644
@@ -425,11 +425,13 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
                *attrcount_orig);
     attrn = attrcount_orig;
     /* Add all elements from the list. */
Quoted 8 lines
drivers/iio/industrialio-event.c int iio_device_register_eventset(struct iio_dev *indio_dev) · line 428
Quoted 6 lines
drivers/iio/industrialio-event.c +1 -5
-    list_for_each_entry(p,
-                &indio_dev->event_interface->dev_attr_list,
-                l)
-        indio_dev->event_interface->group.attrs[attrn++] =
-            &p->dev_attr.attr;
+    if (indio_dev->event_interface->dev_attr_list.next)
next should never be NULL. If it is this means that INIT_LIST_HEAD has not been called on the list and the correct fix is to make sure that it is called.

Quoted 1 line

Exactly. If the channels-attributes have not been set, INIT_LIST_HEAD (called through __iio_add_event_config_attrs) is not called. So, should we just stop instantiating the device if it does not have channel information?

The correct fix is to call INIT_LIST_HEAD right after the struct which contains the list has been allocated. Sascha already sent a patch which does this as well. See: http://www.spinics.net/lists/linux-iio/msg05946.html

- Lars

Hartmut Knaack

author

2012-07-16 19:26 UTC #4 lore raw

Quoted 23 lines · Lars-Peter Clausen

Quoted 22 lines · Hartmut Knaack
Quoted 21 lines · Lars-Peter Clausen
Quoted 9 lines · Hartmut Knaack
This patch adds some checks prior to accessing device attribute lists in industrialio-event.c.
Signed-off-by Harmut Knaack <[email protected]>
---
drivers/iio/industrialio-event.c +0 -0
index b49059d..fe68e90 100644
drivers/iio/industrialio-event.c int iio_device_register_eventset(struct iio_dev *indio_dev) · line 428
Quoted 9 lines
drivers/iio/industrialio-event.c +1 -5
@@ -425,11 +425,13 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
                *attrcount_orig);
     attrn = attrcount_orig;
     /* Add all elements from the list. */
-    list_for_each_entry(p,
-                &indio_dev->event_interface->dev_attr_list,
-                l)
-        indio_dev->event_interface->group.attrs[attrn++] =
-            &p->dev_attr.attr;
+    if (indio_dev->event_interface->dev_attr_list.next)
next should never be NULL. If it is this means that INIT_LIST_HEAD has not been called on the list and the correct fix is to make sure that it is called.

Quoted 7 lines

Quoted 1 line
Exactly. If the channels-attributes have not been set, INIT_LIST_HEAD (called through __iio_add_event_config_attrs) is not called. So, should we just stop instantiating the device if it does not have channel information?
The correct fix is to call INIT_LIST_HEAD right after the struct which contains the list has been allocated. Sascha already sent a patch which does this as well. See: http://www.spinics.net/lists/linux-iio/msg05946.html
- Lars

Alright, never mind then. I'm looking forward to see those changes in mainline kernel :-)

Hartmut