| Message ID | 20190404215344.6330-2-sboyd@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Rewrite clk parent handling | expand |
Hello Stephen, Thanks for taking care of this! On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > We recently introduced a change to support devm clk lookups. That > change > introduced a code-path that used clk_find() without holding the > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' > list and so we need to prevent the list from being modified while > iterating over it by holding the mutex. Similarly, we don't need to > hold > the 'clocks_mutex' besides when we're dereferencing the clk_lookup > pointer /// Snip > -out: > +static struct clk_lookup *clk_find(const char *dev_id, const char > *con_id) > +{ > + struct clk_lookup *cl; > + > + mutex_lock(&clocks_mutex); > + cl = __clk_find(dev_id, con_id); > mutex_unlock(&clocks_mutex); > > - return cl ? clk : ERR_PTR(-ENOENT); > + return cl; > +} I am not an expert on this but reading commit message abowe and seeing the code for clk_find() looks a bit scary. If I understand it correctly, the clocks_mutex should be held when dereferencing the clk_lookup returned by clk_find. The clk_find implementation drops the lock before returning - which makes me think I miss something here. How can the caller ever safely dereference returned clk_lookup pointer? Just reading abowe makes me think that lock should be taken by whoever is calling the clk_find, and dropped only after caller has used the found clk_lookup for whatever caller intends to use it. Maybe I am missing something? Br, Matti Vaittinen
Quoting Vaittinen, Matti (2019-04-04 23:51:43) > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > We recently introduced a change to support devm clk lookups. That > > change > > introduced a code-path that used clk_find() without holding the > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' > > list and so we need to prevent the list from being modified while > > iterating over it by holding the mutex. Similarly, we don't need to > > hold > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup > > pointer > > /// Snip > > > -out: > > +static struct clk_lookup *clk_find(const char *dev_id, const char > > *con_id) > > +{ > > + struct clk_lookup *cl; > > + > > + mutex_lock(&clocks_mutex); > > + cl = __clk_find(dev_id, con_id); > > mutex_unlock(&clocks_mutex); > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > + return cl; > > +} > > I am not an expert on this but reading commit message abowe and seeing > the code for clk_find() looks a bit scary. If I understand it > correctly, the clocks_mutex should be held when dereferencing the > clk_lookup returned by clk_find. The clk_find implementation drops the > lock before returning - which makes me think I miss something here. How > can the caller ever safely dereference returned clk_lookup pointer? > Just reading abowe makes me think that lock should be taken by whoever > is calling the clk_find, and dropped only after caller has used the > found clk_lookup for whatever caller intends to use it. Maybe I am > missing something? > The only user after this patch (devm) is doing a pointer comparison so it looks OK. But yes, in general there shouldn't be users of clk_find() that dereference the pointer because there isn't any protection besides the mutex.
On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote: > Quoting Vaittinen, Matti (2019-04-04 23:51:43) > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > > We recently introduced a change to support devm clk lookups. That > > > change > > > introduced a code-path that used clk_find() without holding the > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' > > > list and so we need to prevent the list from being modified while > > > iterating over it by holding the mutex. Similarly, we don't need to > > > hold > > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup > > > pointer > > > > /// Snip > > > > > -out: > > > +static struct clk_lookup *clk_find(const char *dev_id, const char > > > *con_id) > > > +{ > > > + struct clk_lookup *cl; > > > + > > > + mutex_lock(&clocks_mutex); > > > + cl = __clk_find(dev_id, con_id); > > > mutex_unlock(&clocks_mutex); > > > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > > + return cl; > > > +} > > > > I am not an expert on this but reading commit message abowe and seeing > > the code for clk_find() looks a bit scary. If I understand it > > correctly, the clocks_mutex should be held when dereferencing the > > clk_lookup returned by clk_find. The clk_find implementation drops the > > lock before returning - which makes me think I miss something here. How > > can the caller ever safely dereference returned clk_lookup pointer? > > Just reading abowe makes me think that lock should be taken by whoever > > is calling the clk_find, and dropped only after caller has used the > > found clk_lookup for whatever caller intends to use it. Maybe I am > > missing something? > > > > The only user after this patch (devm) is doing a pointer comparison so > it looks OK. But yes, in general there shouldn't be users of clk_find() > that dereference the pointer because there isn't any protection besides > the mutex. If the only (intended) user for clk_find is devm_clk_release_clkdev, then we might want to write it in devm_clk_release_clkdev - just to avoid similar errors (as I did with devm) in the future? I might even consider renaming __clk_find to clk_find or to clk_find_unsafe - but that's all just nitpicking :) Go with what you like to maintain :D Best regards Matti Vaittinen
Quoting Matti Vaittinen (2019-04-08 03:49:41) > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote: > > Quoting Vaittinen, Matti (2019-04-04 23:51:43) > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > > > We recently introduced a change to support devm clk lookups. That > > > > change > > > > introduced a code-path that used clk_find() without holding the > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' > > > > list and so we need to prevent the list from being modified while > > > > iterating over it by holding the mutex. Similarly, we don't need to > > > > hold > > > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup > > > > pointer > > > > > > /// Snip > > > > > > > -out: > > > > +static struct clk_lookup *clk_find(const char *dev_id, const char > > > > *con_id) > > > > +{ > > > > + struct clk_lookup *cl; > > > > + > > > > + mutex_lock(&clocks_mutex); > > > > + cl = __clk_find(dev_id, con_id); > > > > mutex_unlock(&clocks_mutex); > > > > > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > > > + return cl; > > > > +} > > > > > > I am not an expert on this but reading commit message abowe and seeing > > > the code for clk_find() looks a bit scary. If I understand it > > > correctly, the clocks_mutex should be held when dereferencing the > > > clk_lookup returned by clk_find. The clk_find implementation drops the > > > lock before returning - which makes me think I miss something here. How > > > can the caller ever safely dereference returned clk_lookup pointer? > > > Just reading abowe makes me think that lock should be taken by whoever > > > is calling the clk_find, and dropped only after caller has used the > > > found clk_lookup for whatever caller intends to use it. Maybe I am > > > missing something? > > > > > > > The only user after this patch (devm) is doing a pointer comparison so > > it looks OK. But yes, in general there shouldn't be users of clk_find() > > that dereference the pointer because there isn't any protection besides > > the mutex. > > If the only (intended) user for clk_find is devm_clk_release_clkdev, > then we might want to write it in devm_clk_release_clkdev - just to > avoid similar errors (as I did with devm) in the future? I might even > consider renaming __clk_find to clk_find or to clk_find_unsafe - but > that's all just nitpicking :) Go with what you like to maintain :D > Sure. I was thinking along the same lines after you asked.
On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote: > Quoting Matti Vaittinen (2019-04-08 03:49:41) > > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote: > > > Quoting Vaittinen, Matti (2019-04-04 23:51:43) > > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > > > > We recently introduced a change to support devm clk lookups. That > > > > > change > > > > > introduced a code-path that used clk_find() without holding the > > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' > > > > > list and so we need to prevent the list from being modified while > > > > > iterating over it by holding the mutex. Similarly, we don't need to > > > > > hold > > > > > the 'clocks_mutex' besides when we're dereferencing the clk_lookup > > > > > pointer > > > > > > > > /// Snip > > > > > > > > > -out: > > > > > +static struct clk_lookup *clk_find(const char *dev_id, const char > > > > > *con_id) > > > > > +{ > > > > > + struct clk_lookup *cl; > > > > > + > > > > > + mutex_lock(&clocks_mutex); > > > > > + cl = __clk_find(dev_id, con_id); > > > > > mutex_unlock(&clocks_mutex); > > > > > > > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > > > > + return cl; > > > > > +} > > > > > > > > I am not an expert on this but reading commit message abowe and seeing > > > > the code for clk_find() looks a bit scary. If I understand it > > > > correctly, the clocks_mutex should be held when dereferencing the > > > > clk_lookup returned by clk_find. The clk_find implementation drops the > > > > lock before returning - which makes me think I miss something here. How > > > > can the caller ever safely dereference returned clk_lookup pointer? > > > > Just reading abowe makes me think that lock should be taken by whoever > > > > is calling the clk_find, and dropped only after caller has used the > > > > found clk_lookup for whatever caller intends to use it. Maybe I am > > > > missing something? > > > > > > > > > > The only user after this patch (devm) is doing a pointer comparison so > > > it looks OK. But yes, in general there shouldn't be users of clk_find() > > > that dereference the pointer because there isn't any protection besides > > > the mutex. > > > > If the only (intended) user for clk_find is devm_clk_release_clkdev, > > then we might want to write it in devm_clk_release_clkdev - just to > > avoid similar errors (as I did with devm) in the future? I might even > > consider renaming __clk_find to clk_find or to clk_find_unsafe - but > > that's all just nitpicking :) Go with what you like to maintain :D > > > > Sure. I was thinking along the same lines after you asked. This is rubbish. The reason clk_find() doesn't take the lock is that you _need_ to hold the lock while you dereference the clk_lookup data. The lock isn't protecting just the lookup, it protects what you do with the result of the lookup as well. So, as I say, adding locking inside clk_find() is completely misunderstanding the locking here.
On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin wrote: > On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote: > > Quoting Matti Vaittinen (2019-04-08 03:49:41) > > > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote: > > > > Quoting Vaittinen, Matti (2019-04-04 23:51:43) > > > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > > > > > We recently introduced a change to support devm clk > > > > > > lookups. That > > > > > > change > > > > > > introduced a code-path that used clk_find() without holding > > > > > > the > > > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the > > > > > > 'clocks' > > > > > > list and so we need to prevent the list from being modified > > > > > > while > > > > > > iterating over it by holding the mutex. Similarly, we don't > > > > > > need to > > > > > > hold > > > > > > the 'clocks_mutex' besides when we're dereferencing the > > > > > > clk_lookup > > > > > > pointer > > > > > > > > > > /// Snip > > > > > > > > > > > -out: > > > > > > +static struct clk_lookup *clk_find(const char *dev_id, > > > > > > const char > > > > > > *con_id) > > > > > > +{ > > > > > > + struct clk_lookup *cl; > > > > > > + > > > > > > + mutex_lock(&clocks_mutex); > > > > > > + cl = __clk_find(dev_id, con_id); > > > > > > mutex_unlock(&clocks_mutex); > > > > > > > > > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > > > > > + return cl; > > > > > > +} > > > > > > > > > > I am not an expert on this but reading commit message abowe > > > > > and seeing > > > > > the code for clk_find() looks a bit scary. If I understand it > > > > > correctly, the clocks_mutex should be held when dereferencing > > > > > the > > > > > clk_lookup returned by clk_find. The clk_find implementation > > > > > drops the > > > > > lock before returning - which makes me think I miss something > > > > > here. How > > > > > can the caller ever safely dereference returned clk_lookup > > > > > pointer? > > > > > Just reading abowe makes me think that lock should be taken > > > > > by whoever > > > > > is calling the clk_find, and dropped only after caller has > > > > > used the > > > > > found clk_lookup for whatever caller intends to use it. Maybe > > > > > I am > > > > > missing something? > > > > > > > > > > > > > The only user after this patch (devm) is doing a pointer > > > > comparison so > > > > it looks OK. But yes, in general there shouldn't be users of > > > > clk_find() > > > > that dereference the pointer because there isn't any protection > > > > besides > > > > the mutex. > > > > > > If the only (intended) user for clk_find is > > > devm_clk_release_clkdev, > > > then we might want to write it in devm_clk_release_clkdev - just > > > to > > > avoid similar errors (as I did with devm) in the future? I might > > > even > > > consider renaming __clk_find to clk_find or to clk_find_unsafe - > > > but > > > that's all just nitpicking :) Go with what you like to maintain > > > :D > > > > > > > Sure. I was thinking along the same lines after you asked. > > This is rubbish. The reason clk_find() doesn't take the lock is that > you _need_ to hold the lock while you dereference the clk_lookup > data. I think we all agreed on this already. Stephen pointed out that the current user(s) of clk_find() do _not_ dereference the pointer. > The lock isn't protecting just the lookup, it protects what you do > with > the result of the lookup as well. And we agreed on this too. Br, Matti Vaittinen
On Tue, Apr 09, 2019 at 05:31:48AM +0000, Vaittinen, Matti wrote: > On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin > wrote: > > On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote: > > > Quoting Matti Vaittinen (2019-04-08 03:49:41) > > > > On Fri, Apr 05, 2019 at 01:37:24PM -0700, Stephen Boyd wrote: > > > > > Quoting Vaittinen, Matti (2019-04-04 23:51:43) > > > > > > On Thu, 2019-04-04 at 14:53 -0700, Stephen Boyd wrote: > > > > > > > We recently introduced a change to support devm clk > > > > > > > lookups. That > > > > > > > change > > > > > > > introduced a code-path that used clk_find() without holding > > > > > > > the > > > > > > > 'clocks_mutex'. Unfortunately, clk_find() iterates over the > > > > > > > 'clocks' > > > > > > > list and so we need to prevent the list from being modified > > > > > > > while > > > > > > > iterating over it by holding the mutex. Similarly, we don't > > > > > > > need to > > > > > > > hold > > > > > > > the 'clocks_mutex' besides when we're dereferencing the > > > > > > > clk_lookup > > > > > > > pointer > > > > > > > > > > > > /// Snip > > > > > > > > > > > > > -out: > > > > > > > +static struct clk_lookup *clk_find(const char *dev_id, > > > > > > > const char > > > > > > > *con_id) > > > > > > > +{ > > > > > > > + struct clk_lookup *cl; > > > > > > > + > > > > > > > + mutex_lock(&clocks_mutex); > > > > > > > + cl = __clk_find(dev_id, con_id); > > > > > > > mutex_unlock(&clocks_mutex); > > > > > > > > > > > > > > - return cl ? clk : ERR_PTR(-ENOENT); > > > > > > > + return cl; > > > > > > > +} > > > > > > > > > > > > I am not an expert on this but reading commit message abowe > > > > > > and seeing > > > > > > the code for clk_find() looks a bit scary. If I understand it > > > > > > correctly, the clocks_mutex should be held when dereferencing > > > > > > the > > > > > > clk_lookup returned by clk_find. The clk_find implementation > > > > > > drops the > > > > > > lock before returning - which makes me think I miss something > > > > > > here. How > > > > > > can the caller ever safely dereference returned clk_lookup > > > > > > pointer? > > > > > > Just reading abowe makes me think that lock should be taken > > > > > > by whoever > > > > > > is calling the clk_find, and dropped only after caller has > > > > > > used the > > > > > > found clk_lookup for whatever caller intends to use it. Maybe > > > > > > I am > > > > > > missing something? > > > > > > > > > > > > > > > > The only user after this patch (devm) is doing a pointer > > > > > comparison so > > > > > it looks OK. But yes, in general there shouldn't be users of > > > > > clk_find() > > > > > that dereference the pointer because there isn't any protection > > > > > besides > > > > > the mutex. > > > > > > > > If the only (intended) user for clk_find is > > > > devm_clk_release_clkdev, > > > > then we might want to write it in devm_clk_release_clkdev - just > > > > to > > > > avoid similar errors (as I did with devm) in the future? I might > > > > even > > > > consider renaming __clk_find to clk_find or to clk_find_unsafe - > > > > but > > > > that's all just nitpicking :) Go with what you like to maintain > > > > :D > > > > > > > > > > Sure. I was thinking along the same lines after you asked. > > > > This is rubbish. The reason clk_find() doesn't take the lock is that > > you _need_ to hold the lock while you dereference the clk_lookup > > data. > > I think we all agreed on this already. Stephen pointed out that the > current user(s) of clk_find() do _not_ dereference the pointer. Which is actually another incorrect statement - clk_get_sys() dereferences it, but Stephen's patch does rearrange the code there.
Quoting Russell King - ARM Linux admin (2019-04-09 01:57:17) > On Tue, Apr 09, 2019 at 05:31:48AM +0000, Vaittinen, Matti wrote: > > On Mon, 2019-04-08 at 23:21 +0100, Russell King - ARM Linux admin > > wrote: > > > On Mon, Apr 08, 2019 at 10:00:02AM -0700, Stephen Boyd wrote: > > > > > > > > > > > > > Sure. I was thinking along the same lines after you asked. > > > > > > This is rubbish. The reason clk_find() doesn't take the lock is that > > > you _need_ to hold the lock while you dereference the clk_lookup > > > data. > > > > I think we all agreed on this already. Stephen pointed out that the > > current user(s) of clk_find() do _not_ dereference the pointer. > > Which is actually another incorrect statement - clk_get_sys() > dereferences it, but Stephen's patch does rearrange the code there. > I've split this patch into two. One to fix the locking of the new clk_find() user and the other to introduce a clk_find_hw() API that lets me use it later on in this series. I'll resend the series with this update.
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 8c4435c53f09..db82eee8e209 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -36,7 +36,7 @@ static DEFINE_MUTEX(clocks_mutex); * Then we take the most specific entry - with the following * order of precedence: dev+con > dev only > con only. */ -static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) +static struct clk_lookup *__clk_find(const char *dev_id, const char *con_id) { struct clk_lookup *p, *cl = NULL; int match, best_found = 0, best_possible = 0; @@ -46,6 +46,8 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) if (con_id) best_possible += 1; + lockdep_assert_held(&clocks_mutex); + list_for_each_entry(p, &clocks, node) { match = 0; if (p->dev_id) { @@ -70,25 +72,37 @@ static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) return cl; } -static struct clk *__clk_get_sys(struct device *dev, const char *dev_id, - const char *con_id) +static struct clk_hw *clk_find_hw(const char *dev_id, const char *con_id) { struct clk_lookup *cl; - struct clk *clk = NULL; + struct clk_hw *hw = ERR_PTR(-ENOENT); mutex_lock(&clocks_mutex); + cl = __clk_find(dev_id, con_id); + if (cl) + hw = cl->clk_hw; + mutex_unlock(&clocks_mutex); - cl = clk_find(dev_id, con_id); - if (!cl) - goto out; + return hw; +} - clk = clk_hw_create_clk(dev, cl->clk_hw, dev_id, con_id); - if (IS_ERR(clk)) - cl = NULL; -out: +static struct clk_lookup *clk_find(const char *dev_id, const char *con_id) +{ + struct clk_lookup *cl; + + mutex_lock(&clocks_mutex); + cl = __clk_find(dev_id, con_id); mutex_unlock(&clocks_mutex); - return cl ? clk : ERR_PTR(-ENOENT); + return cl; +} + +static struct clk *__clk_get_sys(struct device *dev, const char *dev_id, + const char *con_id) +{ + struct clk_hw *hw = clk_find_hw(dev_id, con_id); + + return clk_hw_create_clk(dev, hw, dev_id, con_id); } struct clk *clk_get_sys(const char *dev_id, const char *con_id)
We recently introduced a change to support devm clk lookups. That change introduced a code-path that used clk_find() without holding the 'clocks_mutex'. Unfortunately, clk_find() iterates over the 'clocks' list and so we need to prevent the list from being modified while iterating over it by holding the mutex. Similarly, we don't need to hold the 'clocks_mutex' besides when we're dereferencing the clk_lookup pointer we find or while we're modifying/iterating the 'clocks' list. Let's ensure that we have the mutex held while iterating the list and fix the callers of clk_find() to only hold the mutex as long as they need to. This should fix this race and also nicely move clk creation outside of the 'clocks_mutex'. Fixes: 3eee6c7d119c ("clkdev: add managed clkdev lookup registration") Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Jerome Brunet <jbrunet@baylibre.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Jeffrey Hugo <jhugo@codeaurora.org> Cc: Chen-Yu Tsai <wens@csie.org> Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clkdev.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)