clk: Fix potential race condition in clk_get()

RFC PATCH New Sylwester Nawrocki 3 participants 7 messages Aug 9, 2013 – Aug 20, 2013

Subsystems (3)

Common clk Clk API Arm/clkdev support

lore

Share & tools

lore

Sylwester Nawrocki

author

2013-08-09 16:34 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++--------
 include/linux/clk.h |    3 +++
 2 files changed, 31 insertions(+), 8 deletions(-)

drivers/clk/clk.c +28 -8

index 0d4c982..8b8c152 100644
@@ -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;
 }

include/linux/clk.h +3 -0

index 9a6d045..ea6822e 100644
@@ -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)
 {

Signature · 1 line

1.7.9.5

Mike Turquette

2013-08-19 19:41 UTC #2 lore raw

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

Quoting Sylwester Nawrocki (2013-08-09 09:34:05)

Quoted 7 lines

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 <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Looks good to me.

Russell,

Any objections?

Regards,
Mike

Quoted 104 lines

---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++--------
 include/linux/clk.h |    3 +++
 2 files changed, 31 insertions(+), 8 deletions(-)

drivers/clk/clk.c +28 -8

index 0d4c982..8b8c152 100644
@@ -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;
 }

include/linux/clk.h +3 -0

index 9a6d045..ea6822e 100644
@@ -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)
 {

Signature · 1 line

1.7.9.5

Russell King - ARM Linux

2013-08-19 19:50 UTC #3 lore raw

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

Quoted 6 lines · Mike Turquette

Quoting Sylwester Nawrocki (2013-08-09 09:34:05)
Quoted 5 lines
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().

Quoted 8 lines

Quoted 2 lines
Signed-off-by Sylwester Nawrocki <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
Looks good to me.
Russell,
Any objections?

Yes.

Quoted 11 lines

include/linux/clk.h struct of_phandle_args; · line 373

Quoted 11 lines

include/linux/clk.h +3 -0

index 9a6d045..ea6822e 100644
@@ -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.

Sylwester Nawrocki

author

2013-08-20 17:31 UTC #4 lore raw

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

Quoted 36 lines · Russell King - ARM Linux

Quoted 6 lines · Mike Turquette
Quoting Sylwester Nawrocki (2013-08-09 09:34:05)
Quoted 5 lines
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().
Quoted 8 lines
Quoted 2 lines
Signed-off-by Sylwester Nawrocki <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
Looks good to me.
Russell,
Any objections?
Yes.
Quoted 11 lines
include/linux/clk.h struct of_phandle_args; · line 373
Quoted 11 lines
include/linux/clk.h +3 -0
index 9a6d045..ea6822e 100644
@@ -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.

Quoted 7 lines

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.

Quoted 9 lines

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

Sylwester Nawrocki

author

2013-08-09 16:34 UTC #5 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/clk/clkdev.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/clk/clkdev.c +8 -2

index 442a313..4f5c4ef 100644
@@ -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

Mike Turquette

2013-08-19 19:42 UTC #6 lore raw

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

Quoting Sylwester Nawrocki (2013-08-09 09:34:06)

Quoted 13 lines

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.

Quoted 2 lines

Signed-off-by Sylwester Nawrocki <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>

Reviewed-by Mike Turquette <[email protected]>

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

Regards,
Mike

Quoted 34 lines

---
 drivers/clk/clkdev.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/clk/clkdev.c +8 -3

index 442a313..4f5c4ef 100644
@@ -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