[PATCH RFC 0/2] clk: Fix potential race condition in clk_get()

[PATCH RFC 0/2] clk: Fix potential race condition in clk_get()

[Sylwester Nawrocki]

This series depends on my patches adding clk deregistration support [1].
It attempts to fix a race condition in the clock lookup from device tree,
which surfaced during further testing.

In the original clkdev code there is no issues since clock lookup and
__clk_get() are being called with the clocks list mutex held.

In the device tree related part of clk_get() function those operations
are separated which turned out to be unsafe.

The first patch adds the clock providers list locking helpers and
an unlocked version of of_clk_get_from_provider() function.

The second one moves __clk_get() call from clk_get() to of_clk_get()
and modifies of_clk_get() so the clock lookup and __clk_get()
operations are both done with the clocks lists mutex held.

This race condition issue is only present when __clk_get() is
implemented and is only visible with my previous patchset [1] applied.

[1] http://www.mail-archive.com/linux-kernel at vger.kernel.org/msg481861.html

Sylwester Nawrocki (2):
  clk: Provide not locked variant of of_clk_get_from_provider()
  clkdev: Fix race condition in clock lookup from device tree

 drivers/clk/clk.c    |   36 ++++++++++++++++++++++++++++--------
 drivers/clk/clkdev.c |   10 ++++++++--
 include/linux/clk.h  |    3 +++
 3 files changed, 39 insertions(+), 10 deletions(-)

--
1.7.9.5

[PATCH RFC 1/2] clk: Provide not locked variant of of_clk_get_from_provider()

[Sylwester Nawrocki]

Add helper functions for the of_clk_providers list locking and
an unlocked variant of of_clk_get_from_provider().
These functions are intended to be used in the clkdev to avoid
race condition in the device tree based clock look up in clk_get().

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++--------
 include/linux/clk.h |    3 +++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0d4c982..8b8c152 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2143,7 +2143,18 @@ static const struct of_device_id __clk_of_table_sentinel
 	__used __section(__clk_of_table_end);
 
 static LIST_HEAD(of_clk_providers);
-static DEFINE_MUTEX(of_clk_lock);
+static DEFINE_MUTEX(of_clk_mutex);
+
+/* of_clk_provider list locking helpers */
+void of_clk_lock(void)
+{
+	mutex_lock(&of_clk_mutex);
+}
+
+void of_clk_unlock(void)
+{
+	mutex_unlock(&of_clk_mutex);
+}
 
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
 				     void *data)
@@ -2187,9 +2198,9 @@ int of_clk_add_provider(struct device_node *np,
 	cp->data = data;
 	cp->get = clk_src_get;
 
-	mutex_lock(&of_clk_lock);
+	mutex_lock(&of_clk_mutex);
 	list_add(&cp->link, &of_clk_providers);
-	mutex_unlock(&of_clk_lock);
+	mutex_unlock(&of_clk_mutex);
 	pr_info("Added clock from %s\n", np->full_name);
 
 	return 0;
@@ -2204,7 +2215,7 @@ void of_clk_del_provider(struct device_node *np)
 {
 	struct of_clk_provider *cp;
 
-	mutex_lock(&of_clk_lock);
+	mutex_lock(&of_clk_mutex);
 	list_for_each_entry(cp, &of_clk_providers, link) {
 		if (cp->node == np) {
 			list_del(&cp->link);
@@ -2213,24 +2224,33 @@ void of_clk_del_provider(struct device_node *np)
 			break;
 		}
 	}
-	mutex_unlock(&of_clk_lock);
+	mutex_unlock(&of_clk_mutex);
 }
 EXPORT_SYMBOL_GPL(of_clk_del_provider);
 
-struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
+struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
 	struct of_clk_provider *provider;
 	struct clk *clk = ERR_PTR(-ENOENT);
 
 	/* Check if we have such a provider in our array */
-	mutex_lock(&of_clk_lock);
 	list_for_each_entry(provider, &of_clk_providers, link) {
 		if (provider->node == clkspec->np)
 			clk = provider->get(clkspec, provider->data);
 		if (!IS_ERR(clk))
 			break;
 	}
-	mutex_unlock(&of_clk_lock);
+
+	return clk;
+}
+
+struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
+{
+	struct clk *clk;
+
+	mutex_lock(&of_clk_mutex);
+	clk = __of_clk_get_from_provider(clkspec);
+	mutex_unlock(&of_clk_mutex);
 
 	return clk;
 }
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..ea6822e 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -368,6 +368,9 @@ struct of_phandle_args;
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
+struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
+void of_clk_lock(void);
+void of_clk_unlock(void);
 #else
 static inline struct clk *of_clk_get(struct device_node *np, int index)
 {
-- 
1.7.9.5

[PATCH RFC 2/2] clkdev: Fix race condition in clock lookup from device tree

[Sylwester Nawrocki]

There is currently a race condition in the device tree part of clk_get()
function, since the pointer returned from of_clk_get_by_name() may become
invalid before __clk_get() call. For example, due to the clock provider
driver remove() callback being called in between of_clk_get_by_name()
and __clk_get().

Fix this by doing both the look up and __clk_get() operations with the
clock providers list mutex held. This ensures that the clock pointer
returned from __of_clk_get_from_provider() call and passed to __clk_get()
is valid, as long as the clock supplier module first removes its clock
provider instance and then does clk_unregister() on the corresponding
clocks.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/clk/clkdev.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 442a313..4f5c4ef 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -39,7 +39,13 @@ struct clk *of_clk_get(struct device_node *np, int index)
 	if (rc)
 		return ERR_PTR(rc);

-	clk = of_clk_get_from_provider(&clkspec);
+	of_clk_lock();
+	clk = __of_clk_get_from_provider(&clkspec);
+
+	if (!IS_ERR(clk) && !__clk_get(clk))
+		clk = ERR_PTR(-ENOENT);
+
+	of_clk_unlock();
 	of_node_put(clkspec.np);
 	return clk;
 }
@@ -157,7 +163,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)

 	if (dev) {
 		clk = of_clk_get_by_name(dev->of_node, con_id);
-		if (!IS_ERR(clk) && __clk_get(clk))
+		if (!IS_ERR(clk))
 			return clk;
 	}

--
1.7.9.5

[PATCH RFC 1/2] clk: Provide not locked variant of of_clk_get_from_provider()

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-08-09 09:34:05)
> Add helper functions for the of_clk_providers list locking and
> an unlocked variant of of_clk_get_from_provider().
> These functions are intended to be used in the clkdev to avoid
> race condition in the device tree based clock look up in clk_get().
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Looks good to me.

Russell,

Any objections?

Regards,
Mike

> ---
>  drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++--------
>  include/linux/clk.h |    3 +++
>  2 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0d4c982..8b8c152 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2143,7 +2143,18 @@ static const struct of_device_id __clk_of_table_sentinel
>         __used __section(__clk_of_table_end);
>  
>  static LIST_HEAD(of_clk_providers);
> -static DEFINE_MUTEX(of_clk_lock);
> +static DEFINE_MUTEX(of_clk_mutex);
> +
> +/* of_clk_provider list locking helpers */
> +void of_clk_lock(void)
> +{
> +       mutex_lock(&of_clk_mutex);
> +}
> +
> +void of_clk_unlock(void)
> +{
> +       mutex_unlock(&of_clk_mutex);
> +}
>  
>  struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
>                                      void *data)
> @@ -2187,9 +2198,9 @@ int of_clk_add_provider(struct device_node *np,
>         cp->data = data;
>         cp->get = clk_src_get;
>  
> -       mutex_lock(&of_clk_lock);
> +       mutex_lock(&of_clk_mutex);
>         list_add(&cp->link, &of_clk_providers);
> -       mutex_unlock(&of_clk_lock);
> +       mutex_unlock(&of_clk_mutex);
>         pr_info("Added clock from %s\n", np->full_name);
>  
>         return 0;
> @@ -2204,7 +2215,7 @@ void of_clk_del_provider(struct device_node *np)
>  {
>         struct of_clk_provider *cp;
>  
> -       mutex_lock(&of_clk_lock);
> +       mutex_lock(&of_clk_mutex);
>         list_for_each_entry(cp, &of_clk_providers, link) {
>                 if (cp->node == np) {
>                         list_del(&cp->link);
> @@ -2213,24 +2224,33 @@ void of_clk_del_provider(struct device_node *np)
>                         break;
>                 }
>         }
> -       mutex_unlock(&of_clk_lock);
> +       mutex_unlock(&of_clk_mutex);
>  }
>  EXPORT_SYMBOL_GPL(of_clk_del_provider);
>  
> -struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec)
>  {
>         struct of_clk_provider *provider;
>         struct clk *clk = ERR_PTR(-ENOENT);
>  
>         /* Check if we have such a provider in our array */
> -       mutex_lock(&of_clk_lock);
>         list_for_each_entry(provider, &of_clk_providers, link) {
>                 if (provider->node == clkspec->np)
>                         clk = provider->get(clkspec, provider->data);
>                 if (!IS_ERR(clk))
>                         break;
>         }
> -       mutex_unlock(&of_clk_lock);
> +
> +       return clk;
> +}
> +
> +struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
> +{
> +       struct clk *clk;
> +
> +       mutex_lock(&of_clk_mutex);
> +       clk = __of_clk_get_from_provider(clkspec);
> +       mutex_unlock(&of_clk_mutex);
>  
>         return clk;
>  }
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 9a6d045..ea6822e 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -368,6 +368,9 @@ struct of_phandle_args;
>  struct clk *of_clk_get(struct device_node *np, int index);
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
> +void of_clk_lock(void);
> +void of_clk_unlock(void);
>  #else
>  static inline struct clk *of_clk_get(struct device_node *np, int index)
>  {
> -- 
> 1.7.9.5

[PATCH RFC 2/2] clkdev: Fix race condition in clock lookup from device tree

[Mike Turquette]

Quoting Sylwester Nawrocki (2013-08-09 09:34:06)
> There is currently a race condition in the device tree part of clk_get()
> function, since the pointer returned from of_clk_get_by_name() may become
> invalid before __clk_get() call. For example, due to the clock provider
> driver remove() callback being called in between of_clk_get_by_name()
> and __clk_get().
> 
> Fix this by doing both the look up and __clk_get() operations with the
> clock providers list mutex held. This ensures that the clock pointer
> returned from __of_clk_get_from_provider() call and passed to __clk_get()
> is valid, as long as the clock supplier module first removes its clock
> provider instance and then does clk_unregister() on the corresponding
> clocks.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Reviewed-by: Mike Turquette <mturquette@linaro.org>

I can take this through the clk tree if Russell's ACKs.

Regards,
Mike

> ---
>  drivers/clk/clkdev.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 442a313..4f5c4ef 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -39,7 +39,13 @@ struct clk *of_clk_get(struct device_node *np, int index)
>         if (rc)
>                 return ERR_PTR(rc);
> 
> -       clk = of_clk_get_from_provider(&clkspec);
> +       of_clk_lock();
> +       clk = __of_clk_get_from_provider(&clkspec);
> +
> +       if (!IS_ERR(clk) && !__clk_get(clk))
> +               clk = ERR_PTR(-ENOENT);
> +
> +       of_clk_unlock();
>         of_node_put(clkspec.np);
>         return clk;
>  }
> @@ -157,7 +163,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
> 
>         if (dev) {
>                 clk = of_clk_get_by_name(dev->of_node, con_id);
> -               if (!IS_ERR(clk) && __clk_get(clk))
> +               if (!IS_ERR(clk))
>                         return clk;
>         }
> 
> --
> 1.7.9.5

[PATCH RFC 1/2] clk: Provide not locked variant of of_clk_get_from_provider()

[Russell King - ARM Linux]

On Mon, Aug 19, 2013 at 12:41:32PM -0700, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2013-08-09 09:34:05)
> > Add helper functions for the of_clk_providers list locking and
> > an unlocked variant of of_clk_get_from_provider().
> > These functions are intended to be used in the clkdev to avoid
> > race condition in the device tree based clock look up in clk_get().
> > 
> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Looks good to me.
> 
> Russell,
> 
> Any objections?

Yes.

> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 9a6d045..ea6822e 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -368,6 +368,9 @@ struct of_phandle_args;
> >  struct clk *of_clk_get(struct device_node *np, int index);
> >  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
> >  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
> > +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
> > +void of_clk_lock(void);
> > +void of_clk_unlock(void);

Here's the thing.  Yes, I know we have a directory called 'include'
where header files live, but there is nothing layed down which requires
all header files to be under a directory called 'include' - especially
when it's stuff which should _not_ be shared with the rest of the
kernel.

In this case, these three functions are merely exported from
drivers/clk/clk.c to drivers/clk/clkdev.c - which is in the very same
directory.  It is not intended that these functions be used for any
other purpose.

So, why not put them in a header file in drivers/clk/ which both these
files can include?

I *really* wish that people would get out of their mind that everything
has to live in a header file in include/ or arch/arm/include/ and local
includes are bad.  Local includes are a very good thing, they aid in
reducing the visibility of stuff which is not meant to be visible to
the entire kernel.

Just look at things like fs/mount.h to see what I mean.  Or the various
driver header files which contain definitions only for use by their
associated drivers in the drivers/ subtree.

[PATCH RFC 1/2] clk: Provide not locked variant of of_clk_get_from_provider()

[Sylwester Nawrocki]

On 08/19/2013 09:50 PM, Russell King - ARM Linux wrote:
> On Mon, Aug 19, 2013 at 12:41:32PM -0700, Mike Turquette wrote:
>> Quoting Sylwester Nawrocki (2013-08-09 09:34:05)
>>> Add helper functions for the of_clk_providers list locking and
>>> an unlocked variant of of_clk_get_from_provider().
>>> These functions are intended to be used in the clkdev to avoid
>>> race condition in the device tree based clock look up in clk_get().
>>>
>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> Looks good to me.
>>
>> Russell,
>>
>> Any objections?
> 
> Yes.
> 
>>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>>> index 9a6d045..ea6822e 100644
>>> --- a/include/linux/clk.h
>>> +++ b/include/linux/clk.h
>>> @@ -368,6 +368,9 @@ struct of_phandle_args;
>>>  struct clk *of_clk_get(struct device_node *np, int index);
>>>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>>>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>>> +struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
>>> +void of_clk_lock(void);
>>> +void of_clk_unlock(void);
> 
> Here's the thing.  Yes, I know we have a directory called 'include'
> where header files live, but there is nothing layed down which requires
> all header files to be under a directory called 'include' - especially
> when it's stuff which should _not_ be shared with the rest of the
> kernel.
> 
> In this case, these three functions are merely exported from
> drivers/clk/clk.c to drivers/clk/clkdev.c - which is in the very same
> directory.  It is not intended that these functions be used for any
> other purpose.
> 
> So, why not put them in a header file in drivers/clk/ which both these
> files can include?

Yes, that's certainly a good idea. In fact it bothered me a bit that
those functions are unnecessarily exposed like that. But I somehow missed
a separate header could be simply added. Thanks for the suggestion.

I'm going to post v2 of $subject series and the one adding clk_unregister()
implementation including corrections for those couple issues pointed out in
the reviews. Would appreciate to know you opinion on those and perhaps have
your Ack so Mike could take whole series into his tree.

> I *really* wish that people would get out of their mind that everything
> has to live in a header file in include/ or arch/arm/include/ and local
> includes are bad.  Local includes are a very good thing, they aid in
> reducing the visibility of stuff which is not meant to be visible to
> the entire kernel.
> 
> Just look at things like fs/mount.h to see what I mean.  Or the various
> driver header files which contain definitions only for use by their
> associated drivers in the drivers/ subtree.

--
Thanks,
Sylwester