Re: [PATCH v2 1/4] driver core: Make parameter check consistent for API cluster device_(for_each|find)_child()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Aug 21, 2024 at 10:44:27PM +0800, Zijun Hu wrote:
> On 2024/8/20 22:14, Ira Weiny wrote:
> > Zijun Hu wrote:
> >> On 2024/8/20 20:53, Ira Weiny wrote:
> >>> Zijun Hu wrote:
> >>>> From: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>
> >>>> The following API cluster takes the same type parameter list, but do not
> >>>> have consistent parameter check as shown below.
> >>>>
> >>>> device_for_each_child(struct device *parent, ...)  // check (!parent->p)
> >>>> device_for_each_child_reverse(struct device *parent, ...) // same as above
> >>>> device_find_child(struct device *parent, ...)      // check (!parent)
> >>>>
> >>>
> >>> Seems reasonable.
> >>>
> >>> What about device_find_child_by_name()?
> >>>
> >>
> >> Plan to simplify this API implementation by * atomic * API
> >> device_find_child() as following:
> >>
> >> https://lore.kernel.org/all/20240811-simply_api_dfcbn-v2-1-d0398acdc366@quicinc.com
> >> struct device *device_find_child_by_name(struct device *parent,
> >>  					 const char *name)
> >> {
> >> 	return device_find_child(parent, name, device_match_name);
> >> }
> > 
> > Ok.  Thanks.
> > 
> >>
> >>>> Fixed by using consistent check (!parent || !parent->p) for the cluster.
> >>>>
> >>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>> ---
> >>>>  drivers/base/core.c | 6 +++---
> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> 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)
> >>>
> >>> Perhaps this was just a typo which should have been.
> >>>
> >>> 	if (!parent->p)
> >>> ?
> >>>
> >> maybe, but the following device_find_child_by_name() also use (!parent).
> >>
> >>> I think there is an expectation that none of these are called with a NULL
> >>> parent.
> >>>
> >>
> >> this patch aim is to make these atomic APIs have consistent checks as
> >> far as possible, that will make other patches within this series more
> >> acceptable.
> >>
> >> i combine two checks to (!parent || !parent->p) since i did not know
> >> which is better.
> > 
> > I'm not entirely clear either.  But checking the member p makes more sense
> > to me than the parent parameter.  I would expect that iterating the
> > children of a device must be done only when the parent device is not NULL.
> > 
> > parent->p is more subtle.  I'm unclear why the API would need to allow
> > that to run without error.
> > 
> i prefer (!parent || !parent->p) with below reasons:
> 
> 1)
> original API authors have such concern that either (!parent) or
> (!parent->p) maybe happen since they are checked, all their concerns
> can be covered by (!parent || !parent->p).

Wait, a device's parent can NOT be NULL except for some special cases
when it is being created.

And the ->p check is for internal stuff, meaning it has been initialized
and registered properly, again, these functions should never be called
for a device that this has not happened on.  So if they are, crashing is
fine as this should never have gotten through a development cycle.

The ->p checks were added way after the initial driver core was created,
as evolution of moving things out of struct device to prevent drivers
from touching those fields.  They were added add-hoc where needed and
probably not everywhere as you are finding out.

By adding these "robust" checks, we are making it harder for new code to
be written at the expense of doing checks that we "know" are never
going to happen in normal operation.  It's a trade off.  Only add them
when you KNOW they will be needed please.

thanks,

greg k-h