On Wed, Aug 18, 2021 at 7:07 AM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
>
> +Saravana
>
> On Wed, Aug 18, 2021 at 8:26 AM Wentao_Liang <Wentao_Liang_g@xxxxxxx> wrote:
> >
> > In line 1423 (#1), of_link_to_phandle() is called. In the function
> > (line 1140, #2), "of_node_put(sup_np);" drops the reference to phandle
> > and may cause phandle to be released. However, after the function
> > returns, the phandle is subsequently dropped again (line 1424, #3) by
> > the same put function. Double putting the phandle can lead to an
> > incorrect reference count.
> >
> > We believe that the first put of the phandle is unnecessary (#3). We
> > can fix the above bug by removing the redundant "of_node_put()" in line
> > 1423.
> >
> > 1401 static int of_link_property(struct device_node *con_np,
> > const char *prop_name)
> > 1402 {
> > ...
> > 1409 while (!matched && s->parse_prop) {
> > ...
> > 1414
> > 1415 while ((phandle = s->parse_prop(con_np, prop_name, i))) {
> > ...
> > //#1 phandle is dropped in this function
> > 1423 of_link_to_phandle(con_dev_np, phandle);
> >
> > 1424 //#3 the second drop to phandle
> > of_node_put(phandle);
> >
> > 1425 of_node_put(con_dev_np);
> > 1426 }
> > ...
> > 1428 }
> > 1429 return 0;
> > 1430 }
> >
> > 1095 static int of_link_to_phandle(struct device_node *con_np,
> > 1096 struct device_node *sup_np)
> > 1097 {
> > 1098 struct device *sup_dev;
> > 1099 struct device_node *tmp_np = sup_np;
> > ...
> > 1140 of_node_put(sup_np); //#2 the first drop to phandle
> > // (unnecessary)
> > 1141
> > 1142 return 0;
> > 1143 }
> >
> > Signed-off-by: Wentao_Liang <Wentao_Liang_g@xxxxxxx>
> > ---
> > drivers/of/property.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 6c028632f425..408fdde1a20c 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1137,7 +1137,6 @@ static int of_link_to_phandle(struct device_node *con_np,
> > put_device(sup_dev);
> >
> > fwnode_link_add(of_fwnode_handle(con_np), of_fwnode_handle(sup_np));
> > - of_node_put(sup_np);
Hi Wentao,
Thanks for noticing and reporting the bug! Your analysis is correct,
but the fix is definitely wrong. For one, the reference to the node
phandle is pointing to can be dropped in of_link_to_phandle() when it
calls of_get_compat_node(). It could also be dropped in one of the
error paths. So, now you'll be incorrectly dropping the reference for
the wrong node. Let me send out a fix and mention you as the
reporter.
Thanks,
Saravana
