| Message ID | 20240811-const_dfc_prepare-v1-1-d67cc416b3d3@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: Prevent device_find_child() from modifying caller's match data | expand |
On Sun, Aug 11, 2024 at 08:18:07AM +0800, Zijun Hu wrote: > From: Zijun Hu <quic_zijuhu@quicinc.com> > > Add simple parameter checks for APIs device_(for_each|find)_child() and > device_for_each_child_reverse(). Ok, but why? Who is calling this with NULL as a parent pointer? Remember, changelog text describes _why_ not just _what_ you are doing. thanks, greg k-h
On 8/13/2024 5:44 PM, Greg Kroah-Hartman wrote: > On Sun, Aug 11, 2024 at 08:18:07AM +0800, Zijun Hu wrote: >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> Add simple parameter checks for APIs device_(for_each|find)_child() and >> device_for_each_child_reverse(). > > Ok, but why? Who is calling this with NULL as a parent pointer? > > Remember, changelog text describes _why_ not just _what_ you are doing. > For question why ? The main purpose of this change is to make these APIs have *CONSISTENT* parameter checking (!parent || !parent->p) currently, 2 of them have checking (!parent->p), the other have checking (!parent), the are INCONSISTENT. For question who ? device_find_child() have had such checking (!parent), that maybe mean original author has concern that parent may be NULL. Moreover, these are core driver APIs, it is worthy checking input parameter strictly. will correct commit message with v2. > thanks, > > greg k-h
On Tue, Aug 13, 2024 at 06:00:30PM +0800, quic_zijuhu wrote: > On 8/13/2024 5:44 PM, Greg Kroah-Hartman wrote: > > On Sun, Aug 11, 2024 at 08:18:07AM +0800, Zijun Hu wrote: > >> From: Zijun Hu <quic_zijuhu@quicinc.com> > >> > >> Add simple parameter checks for APIs device_(for_each|find)_child() and > >> device_for_each_child_reverse(). > > > > Ok, but why? Who is calling this with NULL as a parent pointer? > > > > Remember, changelog text describes _why_ not just _what_ you are doing. > > > > For question why ? > > The main purpose of this change is to make these APIs have *CONSISTENT* > parameter checking (!parent || !parent->p) > > currently, 2 of them have checking (!parent->p), the other have checking > (!parent), the are INCONSISTENT. > > > For question who ? > device_find_child() have had such checking (!parent), that maybe mean > original author has concern that parent may be NULL. > > Moreover, these are core driver APIs, it is worthy checking input > parameter strictly. Not always, no, don't check for things that will not happen, otherwise you are checking for no reason at all. thanks, greg k-h
diff --git a/drivers/base/core.c b/drivers/base/core.c index 1688e76cb64b..b1dd8c5590dc 100644 --- a/drivers/base/core.c +++ b/drivers/base/core.c @@ -4004,7 +4004,7 @@ int device_for_each_child(struct device *parent, void *data, struct device *child; int error = 0; - if (!parent->p) + if (!parent || !parent->p) return 0; klist_iter_init(&parent->p->klist_children, &i); @@ -4034,7 +4034,7 @@ int device_for_each_child_reverse(struct device *parent, void *data, struct device *child; int error = 0; - if (!parent->p) + if (!parent || !parent->p) return 0; klist_iter_init(&parent->p->klist_children, &i); @@ -4068,7 +4068,7 @@ struct device *device_find_child(struct device *parent, void *data, struct klist_iter i; struct device *child; - if (!parent) + if (!parent || !parent->p) return NULL; klist_iter_init(&parent->p->klist_children, &i);