| Message ID | a8060fe5b23929e2e5c9bf5735e63b302124f66c.1578077228.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [1/2] opp: fix of_node leak for unsupported entries | expand |
On 03-01-20, 20:36, Michał Mirosław wrote: > When parsing OPP v2 table, unsupported entries return NULL from > _opp_add_static_v2(). Please fix that and return something sensible from there. > In this case node reference is leaked. > Make _opp_add_static_v2() always assume ownership of the reference > to fix this. The ownership lies with the routine which took the reference in the first place.
Discard my earlier reply, it wasn't accurate/correct. On 03-01-20, 20:36, Michał Mirosław wrote: > When parsing OPP v2 table, unsupported entries return NULL from > _opp_add_static_v2(). Right, as we don't want parsing to fail here. > In this case node reference is leaked. Why do you think so ?
On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote: > Discard my earlier reply, it wasn't accurate/correct. > > On 03-01-20, 20:36, Michał Mirosław wrote: > > When parsing OPP v2 table, unsupported entries return NULL from > > _opp_add_static_v2(). > > Right, as we don't want parsing to fail here. > > > In this case node reference is leaked. > > Why do you think so ? for_each_available_child_of_node() returns nodes with refcount increased, and _opp_add_static_v2() returning NULL does not store the pointer anywhere - the created (temporary) OPP structure is freed, but _opp_free() does not release node at opp->np. I guess maybe the _opp_free() should be fixed instead? Best Regards, Michał Mirosław
On 07-01-20, 15:04, Michał Mirosław wrote: > On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote: > > Discard my earlier reply, it wasn't accurate/correct. > > > > On 03-01-20, 20:36, Michał Mirosław wrote: > > > When parsing OPP v2 table, unsupported entries return NULL from > > > _opp_add_static_v2(). > > > > Right, as we don't want parsing to fail here. > > > > > In this case node reference is leaked. > > > > Why do you think so ? > > for_each_available_child_of_node() returns nodes with refcount > increased I believe it also drops the refcount of the previous node everytime the loop goes to the next element. Else we would be required do that from within that loop itself, isn't it ?
On Wed, Jan 08, 2020 at 01:03:38PM +0530, Viresh Kumar wrote: > On 07-01-20, 15:04, Michał Mirosław wrote: > > On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote: > > > Discard my earlier reply, it wasn't accurate/correct. > > > > > > On 03-01-20, 20:36, Michał Mirosław wrote: > > > > When parsing OPP v2 table, unsupported entries return NULL from > > > > _opp_add_static_v2(). > > > > > > Right, as we don't want parsing to fail here. > > > > > > > In this case node reference is leaked. > > > > > > Why do you think so ? > > > > for_each_available_child_of_node() returns nodes with refcount > > increased > > I believe it also drops the refcount of the previous node everytime the loop > goes to the next element. Else we would be required do that from within that > loop itself, isn't it ? Indeed it is! This means that _opp_add_static_v2() is storing a pointer to a node without taking a reference to it. Is there something else that guarantees the node won't disappear later? Best Regards, Michał Mirosław
On 08-01-20, 11:38, Michał Mirosław wrote: > On Wed, Jan 08, 2020 at 01:03:38PM +0530, Viresh Kumar wrote: > > On 07-01-20, 15:04, Michał Mirosław wrote: > > > On Tue, Jan 07, 2020 at 12:06:16PM +0530, Viresh Kumar wrote: > > > > Discard my earlier reply, it wasn't accurate/correct. > > > > > > > > On 03-01-20, 20:36, Michał Mirosław wrote: > > > > > When parsing OPP v2 table, unsupported entries return NULL from > > > > > _opp_add_static_v2(). > > > > > > > > Right, as we don't want parsing to fail here. > > > > > > > > > In this case node reference is leaked. > > > > > > > > Why do you think so ? > > > > > > for_each_available_child_of_node() returns nodes with refcount > > > increased > > > > I believe it also drops the refcount of the previous node everytime the loop > > goes to the next element. Else we would be required do that from within that > > loop itself, isn't it ? > > Indeed it is! This means that _opp_add_static_v2() is storing a pointer > to a node without taking a reference to it. Is there something else that > guarantees the node won't disappear later? We do store them, but don't use them other than comparing the value of the pointer itself which is harmless.
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 1cbb58240b80..fba515e432a4 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -555,8 +555,10 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, bool rate_not_available = false; new_opp = _opp_allocate(opp_table); - if (!new_opp) - return ERR_PTR(-ENOMEM); + if (!new_opp) { + ret = -ENOMEM; + goto free_node; + } ret = of_property_read_u64(np, "opp-hz", &rate); if (ret < 0) { @@ -646,6 +648,8 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, _of_opp_free_required_opps(opp_table, new_opp); free_opp: _opp_free(new_opp); +free_node: + of_node_put(np); return ERR_PTR(ret); } @@ -677,7 +681,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table) ret = PTR_ERR(opp); dev_err(dev, "%s: Failed to add OPP, %d\n", __func__, ret); - of_node_put(np); return ret; } else if (opp) { count++;
When parsing OPP v2 table, unsupported entries return NULL from _opp_add_static_v2(). In this case node reference is leaked. Make _opp_add_static_v2() always assume ownership of the reference to fix this. Commit 7978db344719 ("PM / OPP: Add missing of_node_put(np)") already fixed this for the error returns. The leak remained for filtered-out entries from the initial code commit. The Fixes-tagged commit is just a last one that altered the code around. Fixes: 11e1a1648298 ("opp: Don't decrement uninitialized list_kref") Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- drivers/opp/of.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)