Re: [PATCH] clk: core: Copy connection id

[sb...@codeaurora.org]

On 03/02, Leonard Crestez wrote:
> On Tue, 2017-02-28 at 00:05 -0800, sb...@codeaurora.org wrote:
> > Sure. clk_get_sys() could be called and then we could have
> > something sprintf the dev_id there. A quick grep doesn't show any
> > place where that happens though so it seems safe right now.
> > 
> > That said, it would be nice to clearly document that we don't
> > expect dev_id to be freed or changed during the lifetime of the
> > clk structure, but we do allow con_id to change. Perhaps the copy
> > shows that, but a comment would also be useful so we don't have
> > people wondering why dev_id isn't copied as well.
> 
> This should be mentioned on the public documentation for clk_get_sys,
> clk_get and devm_clk_get, right? These seem to be the public entry
> points to the clk subsystem and users are expected to read their docs.

Sure. Except those are not just implemented for the common clk
framework, so the wording will need to be generic. Also we have
some more entry points like of_clk_get() too that would need a
note.

> 
> Do you want me to resend the patch with these notes?

No. I've applied this current patch to clk-fixes.

> 
> I'm not comfortable adding to documentation when I don't fully
> understand the system myself. I only discovered this while looking into
> unrelated driver issues.

Ok. I can take care of sending the patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[sb...@codeaurora.org]

On 03/02, Leonard Crestez wrote:
> On Tue, 2017-02-28 at 00:05 -0800, sb...@codeaurora.org wrote:
> > Sure. clk_get_sys() could be called and then we could have
> > something sprintf the dev_id there. A quick grep doesn't show any
> > place where that happens though so it seems safe right now.
> > 
> > That said, it would be nice to clearly document that we don't
> > expect dev_id to be freed or changed during the lifetime of the
> > clk structure, but we do allow con_id to change. Perhaps the copy
> > shows that, but a comment would also be useful so we don't have
> > people wondering why dev_id isn't copied as well.
> 
> This should be mentioned on the public documentation for clk_get_sys,
> clk_get and devm_clk_get, right? These seem to be the public entry
> points to the clk subsystem and users are expected to read their docs.

Sure. Except those are not just implemented for the common clk
framework, so the wording will need to be generic. Also we have
some more entry points like of_clk_get() too that would need a
note.

> 
> Do you want me to resend the patch with these notes?

No. I've applied this current patch to clk-fixes.

> 
> I'm not comfortable adding to documentation when I don't fully
> understand the system myself. I only discovered this while looking into
> unrelated driver issues.

Ok. I can take care of sending the patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

On Tue, 2017-02-28 at 00:05 -0800, sb...@codeaurora.org wrote:
> On 02/25, Leonard Crestez wrote:
> > 
> > On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> > > 
> > > On 02/20, Leonard Crestez wrote:
> > > > 
> > > > Some drivers use sprintf to build clk connection id names but
> > > > the
> > > > clk
> > > > core will save those strings and occasionally print them back.
> > > > Duplicate
> > > > the con_id strings instead of fixing all the users.
> > > Good catch. What about dev_id though? That could also have the
> > > same problem if some device is removed and we're still holding a
> > > reference to the kobject's name. This is probably more rare than
> > > what is happening here, but still seems possible that we might
> > > trip over that later.
> > A device should normally free the clks it uses before it is
> > destroyed.
> > This means that if dev_id is pointing to freed memory then the clk
> > itself was probably leaked, right?
> Sure. clk_get_sys() could be called and then we could have
> something sprintf the dev_id there. A quick grep doesn't show any
> place where that happens though so it seems safe right now.
> 
> That said, it would be nice to clearly document that we don't
> expect dev_id to be freed or changed during the lifetime of the
> clk structure, but we do allow con_id to change. Perhaps the copy
> shows that, but a comment would also be useful so we don't have
> people wondering why dev_id isn't copied as well.

This should be mentioned on the public documentation for clk_get_sys,
clk_get and devm_clk_get, right? These seem to be the public entry
points to the clk subsystem and users are expected to read their docs.

Do you want me to resend the patch with these notes?

I'm not comfortable adding to documentation when I don't fully
understand the system myself. I only discovered this while looking into
unrelated driver issues.

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

On Tue, 2017-02-28 at 00:05 -0800, sb...@codeaurora.org wrote:
> On 02/25, Leonard Crestez wrote:
> > 
> > On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> > > 
> > > On 02/20, Leonard Crestez wrote:
> > > > 
> > > > Some drivers use sprintf to build clk connection id names but
> > > > the
> > > > clk
> > > > core will save those strings and occasionally print them back.
> > > > Duplicate
> > > > the con_id strings instead of fixing all the users.
> > > Good catch. What about dev_id though? That could also have the
> > > same problem if some device is removed and we're still holding a
> > > reference to the kobject's name. This is probably more rare than
> > > what is happening here, but still seems possible that we might
> > > trip over that later.
> > A device should normally free the clks it uses before it is
> > destroyed.
> > This means that if dev_id is pointing to freed memory then the clk
> > itself was probably leaked, right?
> Sure. clk_get_sys() could be called and then we could have
> something sprintf the dev_id there. A quick grep doesn't show any
> place where that happens though so it seems safe right now.
> 
> That said, it would be nice to clearly document that we don't
> expect dev_id to be freed or changed during the lifetime of the
> clk structure, but we do allow con_id to change. Perhaps the copy
> shows that, but a comment would also be useful so we don't have
> people wondering why dev_id isn't copied as well.

This should be mentioned on the public documentation for clk_get_sys,
clk_get and devm_clk_get, right? These seem to be the public entry
points to the clk subsystem and users are expected to read their docs.

Do you want me to resend the patch with these notes?

I'm not comfortable adding to documentation when I don't fully
understand the system myself. I only discovered this while looking into
unrelated driver issues.

Re: [PATCH] clk: core: Copy connection id

[sb...@codeaurora.org]

On 02/25, Leonard Crestez wrote:
> On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> > On 02/20, Leonard Crestez wrote:
> > > Some drivers use sprintf to build clk connection id names but the
> > > clk
> > > core will save those strings and occasionally print them back.
> > > Duplicate
> > > the con_id strings instead of fixing all the users.
> > 
> > Good catch. What about dev_id though? That could also have the
> > same problem if some device is removed and we're still holding a
> > reference to the kobject's name. This is probably more rare than
> > what is happening here, but still seems possible that we might
> > trip over that later.
> 
> A device should normally free the clks it uses before it is destroyed.
> This means that if dev_id is pointing to freed memory then the clk
> itself was probably leaked, right?

Sure. clk_get_sys() could be called and then we could have
something sprintf the dev_id there. A quick grep doesn't show any
place where that happens though so it seems safe right now.

That said, it would be nice to clearly document that we don't
expect dev_id to be freed or changed during the lifetime of the
clk structure, but we do allow con_id to change. Perhaps the copy
shows that, but a comment would also be useful so we don't have
people wondering why dev_id isn't copied as well.

> 
> This is obvious misuse of the API, not like sprintf-ing a con_id in a
> complex driver. I don't really think it's worth copying strings for it.
> 

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[sb...@codeaurora.org]

On 02/25, Leonard Crestez wrote:
> On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> > On 02/20, Leonard Crestez wrote:
> > > Some drivers use sprintf to build clk connection id names but the
> > > clk
> > > core will save those strings and occasionally print them back.
> > > Duplicate
> > > the con_id strings instead of fixing all the users.
> > 
> > Good catch. What about dev_id though? That could also have the
> > same problem if some device is removed and we're still holding a
> > reference to the kobject's name. This is probably more rare than
> > what is happening here, but still seems possible that we might
> > trip over that later.
> 
> A device should normally free the clks it uses before it is destroyed.
> This means that if dev_id is pointing to freed memory then the clk
> itself was probably leaked, right?

Sure. clk_get_sys() could be called and then we could have
something sprintf the dev_id there. A quick grep doesn't show any
place where that happens though so it seems safe right now.

That said, it would be nice to clearly document that we don't
expect dev_id to be freed or changed during the lifetime of the
clk structure, but we do allow con_id to change. Perhaps the copy
shows that, but a comment would also be useful so we don't have
people wondering why dev_id isn't copied as well.

> 
> This is obvious misuse of the API, not like sprintf-ing a con_id in a
> complex driver. I don't really think it's worth copying strings for it.
> 

Ok.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> On 02/20, Leonard Crestez wrote:
> > Some drivers use sprintf to build clk connection id names but the
> > clk
> > core will save those strings and occasionally print them back.
> > Duplicate
> > the con_id strings instead of fixing all the users.
> 
> Good catch. What about dev_id though? That could also have the
> same problem if some device is removed and we're still holding a
> reference to the kobject's name. This is probably more rare than
> what is happening here, but still seems possible that we might
> trip over that later.

A device should normally free the clks it uses before it is destroyed.
This means that if dev_id is pointing to freed memory then the clk
itself was probably leaked, right?

This is obvious misuse of the API, not like sprintf-ing a con_id in a
complex driver. I don't really think it's worth copying strings for it.

--
Regards,
Leonard

Re: [PATCH] clk: core: Copy connection id

[Leonard Crestez]

On Fri, 2017-02-24 at 12:44 -0800, Stephen Boyd wrote:
> On 02/20, Leonard Crestez wrote:
> > Some drivers use sprintf to build clk connection id names but the
> > clk
> > core will save those strings and occasionally print them back.
> > Duplicate
> > the con_id strings instead of fixing all the users.
> 
> Good catch. What about dev_id though? That could also have the
> same problem if some device is removed and we're still holding a
> reference to the kobject's name. This is probably more rare than
> what is happening here, but still seems possible that we might
> trip over that later.

A device should normally free the clks it uses before it is destroyed.
This means that if dev_id is pointing to freed memory then the clk
itself was probably leaked, right?

This is obvious misuse of the API, not like sprintf-ing a con_id in a
complex driver. I don't really think it's worth copying strings for it.

--
Regards,
Leonard

Re: [PATCH] clk: core: Copy connection id

[Stephen Boyd]

On 02/20, Leonard Crestez wrote:
> Some drivers use sprintf to build clk connection id names but the clk
> core will save those strings and occasionally print them back. Duplicate
> the con_id strings instead of fixing all the users.
> 
> Signed-off-by: Leonard Crestez 
> ---
>  drivers/clk/clk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Some examples of using sprintf for con_id include:
> drivers/mfd/omap-usb-host.c
> drivers/tty/serial/samsung.c
> sound/soc/fsl/fsl_asrc.c
> 
> There are lots more. They are difficult to find and "fixing" them on the
> consumer side requires nasty code to keep track of the allocated clkname.

Good catch. What about dev_id though? That could also have the
same problem if some device is removed and we're still holding a
reference to the kobject's name. This is probably more rare than
what is happening here, but still seems possible that we might
trip over that later.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: core: Copy connection id

[Stephen Boyd]

On 02/20, Leonard Crestez wrote:
> Some drivers use sprintf to build clk connection id names but the clk
> core will save those strings and occasionally print them back. Duplicate
> the con_id strings instead of fixing all the users.
> 
> Signed-off-by: Leonard Crestez 
> ---
>  drivers/clk/clk.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Some examples of using sprintf for con_id include:
> drivers/mfd/omap-usb-host.c
> drivers/tty/serial/samsung.c
> sound/soc/fsl/fsl_asrc.c
> 
> There are lots more. They are difficult to find and "fixing" them on the
> consumer side requires nasty code to keep track of the allocated clkname.

Good catch. What about dev_id though? That could also have the
same problem if some device is removed and we're still holding a
reference to the kobject's name. This is probably more rare than
what is happening here, but still seems possible that we might
trip over that later.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] clk: core: Copy connection id

[Leonard Crestez]

Some drivers use sprintf to build clk connection id names but the clk
core will save those strings and occasionally print them back. Duplicate
the con_id strings instead of fixing all the users.

Signed-off-by: Leonard Crestez 
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Some examples of using sprintf for con_id include:
drivers/mfd/omap-usb-host.c
drivers/tty/serial/samsung.c
sound/soc/fsl/fsl_asrc.c

There are lots more. They are difficult to find and "fixing" them on the
consumer side requires nasty code to keep track of the allocated clkname.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0fb39fe..67201f6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2502,7 +2502,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const 
char *dev_id,
 
clk->core = hw->core;
clk->dev_id = dev_id;
-   clk->con_id = con_id;
+   clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
clk->max_rate = ULONG_MAX;
 
clk_prepare_lock();
@@ -2518,6 +2518,7 @@ void __clk_free_clk(struct clk *clk)
hlist_del(>clks_node);
clk_prepare_unlock();
 
+   kfree_const(clk->con_id);
kfree(clk);
 }
 
-- 
2.7.4

[PATCH] clk: core: Copy connection id

[Leonard Crestez]

Some drivers use sprintf to build clk connection id names but the clk
core will save those strings and occasionally print them back. Duplicate
the con_id strings instead of fixing all the users.

Signed-off-by: Leonard Crestez 
---
 drivers/clk/clk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Some examples of using sprintf for con_id include:
drivers/mfd/omap-usb-host.c
drivers/tty/serial/samsung.c
sound/soc/fsl/fsl_asrc.c

There are lots more. They are difficult to find and "fixing" them on the
consumer side requires nasty code to keep track of the allocated clkname.

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0fb39fe..67201f6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2502,7 +2502,7 @@ struct clk *__clk_create_clk(struct clk_hw *hw, const 
char *dev_id,
 
clk->core = hw->core;
clk->dev_id = dev_id;
-   clk->con_id = con_id;
+   clk->con_id = kstrdup_const(con_id, GFP_KERNEL);
clk->max_rate = ULONG_MAX;
 
clk_prepare_lock();
@@ -2518,6 +2518,7 @@ void __clk_free_clk(struct clk *clk)
hlist_del(>clks_node);
clk_prepare_unlock();
 
+   kfree_const(clk->con_id);
kfree(clk);
 }
 
-- 
2.7.4