[1/2] opp: fix of_node leak for unsupported entries
diff mbox series

Message ID a8060fe5b23929e2e5c9bf5735e63b302124f66c.1578077228.git.mirq-linux@rere.qmqm.pl
State New, archived
Delegated to: viresh kumar
Headers show
Series
  • [1/2] opp: fix of_node leak for unsupported entries
Related show

Commit Message

Michał Mirosław Jan. 3, 2020, 7:36 p.m. UTC
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(-)

Comments

Viresh Kumar Jan. 7, 2020, 6:31 a.m. UTC | #1
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.
Viresh Kumar Jan. 7, 2020, 6:36 a.m. UTC | #2
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 ?
Michał Mirosław Jan. 7, 2020, 2:04 p.m. UTC | #3
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
Viresh Kumar Jan. 8, 2020, 7:33 a.m. UTC | #4
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 ?
Michał Mirosław Jan. 8, 2020, 10:38 a.m. UTC | #5
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
Viresh Kumar Jan. 8, 2020, 10:44 a.m. UTC | #6
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.

Patch
diff mbox series

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++;