clkdev: prevent potential memory leak when used in modules

PATCH In review Andy Shevchenko 2 participants 6 messages May 6, 2015 – May 8, 2015

Subsystems (10)

Arm (32-bit) Arm sub-architectures MIPS Common clk +6 more subsystems

lore

Share & tools

lore

Andy Shevchenko

author

2015-05-06 10:24 UTC #0 lore raw

v4 cover clkdev: prevent potential memory leak when used in modules

Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.

The patch changes the prototype of the clk_register_clkdev() to return a pointer to the allocated resources. The caller should take care of the returned variable and free allocated resources when needed. Together with the main change the users are updated too.

The current change doesn't fix any present memory leak, thus developers and maintainers should take care of it if needed.

The patch has been tested on x86 (*), thus requires to be tested on touched architectures.

(*) On x86 there will be a user, more details at

    http://www.spinics.net/lists/kernel/msg1958587.html.

Changelog v4:
- rebase on recent linux-next
- add Ack from Sylwester Nawrocki

Changelog v3:
- fix compiler warnings
drivers/clk/samsung/clk-pll.c:1304:3: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long int' [-Wformat=] drivers/clk/samsung/clk-s3c2410-dclk.c:323:3: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long int' [-Wformat=] drivers/clk/samsung/clk-s3c2410-dclk.c:243:6: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]

Changelog v2:
- update inplace documentation to mention clkdev_drop() that should be used to free the resources
- mention in cover letter that we have tested it on x86
- append Mika's Reviewed-by tag
- add Russel to the Cc list
- rebase on top of 4.0-rc1

Andy Shevchenko (1):
clkdev: change prototype of clk_register_clkdev()

 arch/arm/mach-vexpress/spc.c           |  5 ++++-
 arch/mips/ath79/clock.c                |  6 +++---
 drivers/clk/clk-bcm2835.c              | 12 +++++++-----
 drivers/clk/clk-max-gen.c              |  9 ++++-----
 drivers/clk/clk-xgene.c                |  6 +++---
 drivers/clk/clkdev.c                   | 15 ++++++++++-----
 drivers/clk/samsung/clk-pll.c          | 15 +++++++++------
 drivers/clk/samsung/clk-s3c2410-dclk.c | 22 +++++++++++----------
 drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
 include/linux/clkdev.h                 |  2 +-
 10 files changed, 73 insertions(+), 54 deletions(-)

Signature · 1 line

2.1.4

Andy Shevchenko

author

2015-05-06 10:24 UTC #1 lore raw

PATCH v4 1/1 clkdev: change prototype of clk_register_clkdev()

Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.

Cc Tomeu Vizoso <[email protected]>

Reviewed-by Mika Westerberg <[email protected]>

Acked-by Sylwester Nawrocki <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/arm/mach-vexpress/spc.c           |  5 ++++-
 arch/mips/ath79/clock.c                |  6 +++---
 drivers/clk/clk-bcm2835.c              | 12 +++++++-----
 drivers/clk/clk-max-gen.c              |  9 ++++-----
 drivers/clk/clk-xgene.c                |  6 +++---
 drivers/clk/clkdev.c                   | 15 ++++++++++-----
 drivers/clk/samsung/clk-pll.c          | 15 +++++++++------
 drivers/clk/samsung/clk-s3c2410-dclk.c | 22 +++++++++++----------
 drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
 include/linux/clkdev.h                 |  2 +-
 10 files changed, 73 insertions(+), 54 deletions(-)

arch/arm/mach-vexpress/spc.c +4 -1

index f61158c..0477007 100644
@@ -568,6 +568,8 @@ static int __init ve_spc_clk_init(void)
 
 	for_each_possible_cpu(cpu) {
 		struct device *cpu_dev = get_cpu_device(cpu);
+		struct clk_lookup *cl;
+
 		if (!cpu_dev) {
 			pr_warn("failed to get cpu%d device\n", cpu);
 			continue;

@@ -577,7 +579,8 @@ static int __init ve_spc_clk_init(void)
 			pr_warn("failed to register cpu%d clock\n", cpu);
 			continue;
 		}
-		if (clk_register_clkdev(clk, NULL, dev_name(cpu_dev))) {
+		cl = clk_register_clkdev(clk, NULL, dev_name(cpu_dev));
+		if (IS_ERR(cl)) {
 			pr_warn("failed to register cpu%d clock lookup\n", cpu);
 			continue;
 		}

arch/mips/ath79/clock.c +3 -3

index 26479f4..db5ed95 100644
@@ -35,7 +35,7 @@ struct clk {
 static void __init ath79_add_sys_clkdev(const char *id, unsigned long rate)
 {
 	struct clk *clk;
-	int err;
+	struct clk_lookup *cl;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)

@@ -43,8 +43,8 @@ static void __init ath79_add_sys_clkdev(const char *id, unsigned long rate)
 
 	clk->rate = rate;
 
-	err = clk_register_clkdev(clk, id, NULL);
-	if (err)
+	cl = clk_register_clkdev(clk, id, NULL);
+	if (IS_ERR(cl))
 		panic("unable to register %s clock device", id);
 }

drivers/clk/clk-bcm2835.c +7 -5

index 6b950ca..6243527 100644
@@ -30,7 +30,7 @@
 void __init bcm2835_init_clocks(void)
 {
 	struct clk *clk;
-	int ret;
+	struct clk_lookup *cl;
 
 	clk = clk_register_fixed_rate(NULL, "sys_pclk", NULL, CLK_IS_ROOT,
 					250000000);

@@ -46,15 +46,17 @@ void __init bcm2835_init_clocks(void)
 					3000000);
 	if (IS_ERR(clk))
 		pr_err("uart0_pclk not registered\n");
-	ret = clk_register_clkdev(clk, NULL, "20201000.uart");
-	if (ret)
+
+	cl = clk_register_clkdev(clk, NULL, "20201000.uart");
+	if (IS_ERR(cl))
 		pr_err("uart0_pclk alias not registered\n");
 
 	clk = clk_register_fixed_rate(NULL, "uart1_pclk", NULL, CLK_IS_ROOT,
 					125000000);
 	if (IS_ERR(clk))
 		pr_err("uart1_pclk not registered\n");
-	ret = clk_register_clkdev(clk, NULL, "20215000.uart");
-	if (ret)
+
+	cl = clk_register_clkdev(clk, NULL, "20215000.uart");
+	if (IS_ERR(cl))
 		pr_err("uart1_pclk alias not registered\n");
 }

drivers/clk/clk-max-gen.c +4 -5

index 6505049..cd19136 100644
@@ -92,16 +92,15 @@ static struct clk *max_gen_clk_register(struct device *dev,
 {
 	struct clk *clk;
 	struct clk_hw *hw = &max_gen->hw;
-	int ret;
+	struct clk_lookup *cl;
 
 	clk = devm_clk_register(dev, hw);
 	if (IS_ERR(clk))
 		return clk;
 
-	ret = clk_register_clkdev(clk, hw->init->name, NULL);
-
-	if (ret)
-		return ERR_PTR(ret);
+	cl = clk_register_clkdev(clk, hw->init->name, NULL);
+	if (IS_ERR(cl))
+		return ERR_CAST(cl);
 
 	return clk;
 }

drivers/clk/clk-xgene.c +3 -3

index dd8a62d..ce5bf4d 100644
@@ -401,8 +401,8 @@ static struct clk *xgene_register_clk(struct device *dev,
 {
 	struct xgene_clk *apmclk;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
-	int rc;
 
 	/* allocate the APM clock structure */
 	apmclk = kzalloc(sizeof(*apmclk), GFP_KERNEL);

@@ -431,8 +431,8 @@ static struct clk *xgene_register_clk(struct device *dev,
 	}
 
 	/* Register the clock for lookup */
-	rc = clk_register_clkdev(clk, name, NULL);
-	if (rc != 0) {
+	cl = clk_register_clkdev(clk, name, NULL);
+	if (IS_ERR(cl)) {
 		pr_err("%s: could not register lookup clk %s\n",
 			__func__, name);
 	}

drivers/clk/clkdev.c +10 -5

index 1fcb6ef..c8f67ca 100644
@@ -320,29 +320,34 @@ EXPORT_SYMBOL(clkdev_drop);
  * clkdev.
  *
  * To make things easier for mass registration, we detect error clks
- * from a previous clk_register() call, and return the error code for
+ * from a previous clk_register() call, and return the error pointer for
  * those.  This is to permit this function to be called immediately
  * after clk_register().
+ *
+ * Return:
+ * A pointer to the allocated struct clk_lookup is returned on success, or
+ * error pointer otherwise. The caller may use clkdev_drop() to free the
+ * allocated resources.
  */
-int clk_register_clkdev(struct clk *clk, const char *con_id,
+struct clk_lookup *clk_register_clkdev(struct clk *clk, const char *con_id,
 	const char *dev_fmt, ...)
 {
 	struct clk_lookup *cl;
 	va_list ap;
 
 	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+		return ERR_CAST(clk);
 
 	va_start(ap, dev_fmt);
 	cl = vclkdev_alloc(clk, con_id, dev_fmt, ap);
 	va_end(ap);
 
 	if (!cl)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	clkdev_add(cl);
 
-	return 0;
+	return cl;
 }
 EXPORT_SYMBOL(clk_register_clkdev);

drivers/clk/samsung/clk-pll.c +9 -6

index 9d70e5c..e213e35 100644
@@ -927,6 +927,7 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
 {
 	struct samsung_clk_pll2550x *pll;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);

@@ -952,7 +953,8 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
 		kfree(pll);
 	}
 
-	if (clk_register_clkdev(clk, name, NULL))
+	cl = clk_register_clkdev(clk, name, NULL);
+	if (IS_ERR(cl))
 		pr_err("%s: failed to register lookup for %s", __func__, name);
 
 	return clk;

@@ -1161,8 +1163,9 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
-	int ret, len;
+	int len;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
 	if (!pll) {

@@ -1296,10 +1299,10 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 	if (!pll_clk->alias)
 		return;
 
-	ret = clk_register_clkdev(clk, pll_clk->alias, pll_clk->dev_name);
-	if (ret)
-		pr_err("%s: failed to register lookup for %s : %d",
-			__func__, pll_clk->name, ret);
+	cl = clk_register_clkdev(clk, pll_clk->alias, pll_clk->dev_name);
+	if (IS_ERR(cl))
+		pr_err("%s: failed to register lookup for %s : %ld",
+			__func__, pll_clk->name, PTR_ERR(cl));
 }
 
 void __init samsung_clk_register_pll(struct samsung_clk_provider *ctx,

drivers/clk/samsung/clk-s3c2410-dclk.c +12 ~~-10~~

index f4f29ed6..6f07b23 100644
@@ -238,8 +238,9 @@ static int s3c24xx_dclk_probe(struct platform_device *pdev)
 	struct s3c24xx_dclk *s3c24xx_dclk;
 	struct resource *mem;
 	struct clk **clk_table;
+	struct clk_lookup *cl;
 	struct s3c24xx_dclk_drv_data *dclk_variant;
-	int ret, i;
+	int ret = 0, i;
 
 	s3c24xx_dclk = devm_kzalloc(&pdev->dev, sizeof(*s3c24xx_dclk),
 				    GFP_KERNEL);

@@ -309,17 +310,18 @@ static int s3c24xx_dclk_probe(struct platform_device *pdev)
 			goto err_clk_register;
 		}
 
-	ret = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_CLKOUT0],
+	cl = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_CLKOUT0],
 					  "clkout0", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_CLKOUT1],
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_CLKOUT1],
 					  "clkout1", NULL);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register aliases, %d\n", ret);
+	if (IS_ERR(cl)) {
+		dev_err(&pdev->dev, "failed to register aliases, %ld\n",
+			PTR_ERR(cl));
 		goto err_clk_register;
 	}

drivers/clk/samsung/clk.c +20 ~~-15~~

index 9e1f88c..eb52ab1 100644
@@ -102,7 +102,8 @@ void __init samsung_clk_register_alias(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	if (!ctx->clk_data.clks) {
 		pr_err("%s: clock table missing\n", __func__);

@@ -123,8 +124,8 @@ void __init samsung_clk_register_alias(struct samsung_clk_provider *ctx,
 			continue;
 		}
 
-		ret = clk_register_clkdev(clk, list->alias, list->dev_name);
-		if (ret)
+		cl = clk_register_clkdev(clk, list->alias, list->dev_name);
+		if (IS_ERR(cl))
 			pr_err("%s: failed to register lookup %s\n",
 					__func__, list->alias);
 	}

@@ -135,7 +136,8 @@ void __init samsung_clk_register_fixed_rate(struct samsung_clk_provider *ctx,
 		struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_fixed_rate(NULL, list->name,

@@ -152,8 +154,8 @@ void __init samsung_clk_register_fixed_rate(struct samsung_clk_provider *ctx,
 		 * Unconditionally add a clock lookup for the fixed rate clocks.
 		 * There are not many of these on any of Samsung platforms.
 		 */
-		ret = clk_register_clkdev(clk, list->name, NULL);
-		if (ret)
+		cl = clk_register_clkdev(clk, list->name, NULL);
+		if (IS_ERR(cl))
 			pr_err("%s: failed to register clock lookup for %s",
 				__func__, list->name);
 	}

@@ -185,7 +187,8 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_mux(NULL, list->name, list->parent_names,

@@ -202,9 +205,9 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 						list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 						__func__, list->alias);
 		}

@@ -217,7 +220,8 @@ void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		if (list->table)

@@ -241,9 +245,9 @@ void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 						list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 						__func__, list->alias);
 		}

@@ -256,7 +260,8 @@ void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_gate(NULL, list->name, list->parent_name,

@@ -270,9 +275,9 @@ void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 							list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 					__func__, list->alias);
 		}

include/linux/clkdev.h +1 -1

index 94bad77..87ace2c 100644
@@ -40,7 +40,7 @@ void clkdev_drop(struct clk_lookup *cl);
 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, char *, struct device *);
 
-int clk_register_clkdev(struct clk *, const char *, const char *, ...);
+struct clk_lookup *clk_register_clkdev(struct clk *, const char *, const char *, ...);
 int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t);
 
 #ifdef CONFIG_COMMON_CLK

Signature · 1 line

2.1.4

Russell King - ARM Linux

2015-05-06 11:10 UTC #2 lore raw

Re: [PATCH v4 1/1] clkdev: change prototype of clk_register_clkdev()

Quoted 3 lines · Andy Shevchenko

Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.

clk_register_clkdev() is there to assist mass clock registrations, which typically happen in platform code. It's there to simplify the "I need to register this clock which I'm not going to release again".

I don't see any of these locations trying to unregister their clk from clkdev, so I doubt this patch is needed.

Where a module wants to remove its clk from clkdev, it should register its clk with clkdev_create() and remove it with clkdev_drop().

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Andy Shevchenko

author

2015-05-06 12:34 UTC #3 lore raw

Re: [PATCH v4 1/1] clkdev: change prototype of clk_register_clkdev()

Quoted 11 lines · Russell King - ARM Linux

Quoted 3 lines · Andy Shevchenko
Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.
clk_register_clkdev() is there to assist mass clock registrations, which typically happen in platform code. It's there to simplify the "I need to register this clock which I'm not going to release again".
I don't see any of these locations trying to unregister their clk from clkdev, so I doubt this patch is needed.

We are doing the driver which will use this (as I mentioned in the cover letter).

Quoted 4 lines

Where a module wants to remove its clk from clkdev, it should register its clk with clkdev_create() and remove it with clkdev_drop().

You are talking about something in the wild? I can't find clkdev_create() neither in current clk.git nor in linux-next.git.

Otherwise we have (and there are places with same case) to use clkdev_alloc() + clkdev_add() which is few lines more of code.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Russell King - ARM Linux

2015-05-06 12:51 UTC #4 lore raw

Re: [PATCH v4 1/1] clkdev: change prototype of clk_register_clkdev()

Quoted 17 lines · Andy Shevchenko

Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Andy Shevchenko
Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.
clk_register_clkdev() is there to assist mass clock registrations, which typically happen in platform code. It's there to simplify the "I need to register this clock which I'm not going to release again".
I don't see any of these locations trying to unregister their clk from clkdev, so I doubt this patch is needed.
We are doing the driver which will use this (as I mentioned in the cover letter).
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
Where a module wants to remove its clk from clkdev, it should register its clk with clkdev_create() and remove it with clkdev_drop().
You are talking about something in the wild? I can't find clkdev_create() neither in current clk.git nor in linux-next.git.

It'll be in linux-next RSN.

Search lakml for "Fix fallout from per-user struct clk patches"

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Andy Shevchenko

author

2015-05-08 11:01 UTC #5 lore raw

Re: [PATCH v4 1/1] clkdev: change prototype of clk_register_clkdev()

Quoted 22 lines · Russell King - ARM Linux

Quoted 21 lines · Andy Shevchenko
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Andy Shevchenko
Since clk_register_clkdev() is exported for modules the caller should get a pointer to the allocated resources. Otherwise the memory leak is guaranteed on the ->remove() stage.
clk_register_clkdev() is there to assist mass clock registrations, which typically happen in platform code. It's there to simplify the "I need to register this clock which I'm not going to release again".
I don't see any of these locations trying to unregister their clk from clkdev, so I doubt this patch is needed.
We are doing the driver which will use this (as I mentioned in the cover letter).
Quoted 4 lines
Where a module wants to remove its clk from clkdev, it should register its clk with clkdev_create() and remove it with clkdev_drop().

Quoted 7 lines

Quoted 2 lines
You are talking about something in the wild? I can't find clkdev_create() neither in current clk.git nor in linux-next.git.
It'll be in linux-next RSN.
Search lakml for "Fix fallout from per-user struct clk patches"

Thanks, we will use it, though the documentation might say that there is a risk of memory leak in case of usage clk_register_clkdev().

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy