In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and
'dst' are allcoted by kstrdup_const(). According to doc: "Strings
allocated by kstrdup_const should be freed by kfree_const". So
'parent->name', 'parent->fw_name' and 'dst' should be freed.
Signed-off-by: Gen Zhang <[email protected]>
---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756..85c4d3f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
if (!dst)
return -ENOMEM;
+ kfree_const(dst);
return 0;
}
@@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
kfree_const(parents[i].name);
kfree_const(parents[i].fw_name);
} while (--i >= 0);
+ kfree_const(parent->name);
+ kfree_const(parent->fw_name);
kfree(parents);
return ret;
---
On 31. 05. 19, 3:14, Gen Zhang wrote:
> In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and
> 'dst' are allcoted by kstrdup_const(). According to doc: "Strings
> allocated by kstrdup_const should be freed by kfree_const". So
> 'parent->name', 'parent->fw_name' and 'dst' should be freed.
>
> Signed-off-by: Gen Zhang <[email protected]>
> ---
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756..85c4d3f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> if (!dst)
> return -ENOMEM;
>
> + kfree_const(dst);
So you are now returning a freed pointer in dst_p?
> return 0;
> }
>
> @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> kfree_const(parents[i].name);
> kfree_const(parents[i].fw_name);
> } while (--i >= 0);
> + kfree_const(parent->name);
> + kfree_const(parent->fw_name);
Both of them were just freed in the loop above, no?
thanks,
--
js
suse labs
On Wed, Jun 05, 2019 at 08:38:00AM +0200, Jiri Slaby wrote:
> On 31. 05. 19, 3:14, Gen Zhang wrote:
> > In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and
> > 'dst' are allcoted by kstrdup_const(). According to doc: "Strings
> > allocated by kstrdup_const should be freed by kfree_const". So
> > 'parent->name', 'parent->fw_name' and 'dst' should be freed.
> >
> > Signed-off-by: Gen Zhang <[email protected]>
> > ---
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index aa51756..85c4d3f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> > if (!dst)
> > return -ENOMEM;
> >
> > + kfree_const(dst);
>
> So you are now returning a freed pointer in dst_p?
Thanks for your reply. I re-examined the code, and this kfree is
incorrect and it should be deleted.
>
> > return 0;
> > }
> >
> > @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> > kfree_const(parents[i].name);
> > kfree_const(parents[i].fw_name);
> > } while (--i >= 0);
> > + kfree_const(parent->name);
> > + kfree_const(parent->fw_name);
>
> Both of them were just freed in the loop above, no?
for (i = 0, parent = parents; i < num_parents; i++, parent++)
Is 'parent' the same as the one from the loop above?
Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
kfree_const()?
Thanks
Gen
>
> thanks,
> --
> js
> suse labs
Quoting Gen Zhang (2019-06-05 09:00:43)
> On Wed, Jun 05, 2019 at 08:38:00AM +0200, Jiri Slaby wrote:
> > On 31. 05. 19, 3:14, Gen Zhang wrote:
> > > In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and
> > > 'dst' are allcoted by kstrdup_const(). According to doc: "Strings
> > > allocated by kstrdup_const should be freed by kfree_const". So
> > > 'parent->name', 'parent->fw_name' and 'dst' should be freed.
> > >
> > > Signed-off-by: Gen Zhang <[email protected]>
> > > ---
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index aa51756..85c4d3f 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> > > if (!dst)
> > > return -ENOMEM;
> > >
> > > + kfree_const(dst);
> >
> > So you are now returning a freed pointer in dst_p?
> Thanks for your reply. I re-examined the code, and this kfree is
> incorrect and it should be deleted.
> >
> > > return 0;
> > > }
> > >
> > > @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> > > kfree_const(parents[i].name);
> > > kfree_const(parents[i].fw_name);
> > > } while (--i >= 0);
> > > + kfree_const(parent->name);
> > > + kfree_const(parent->fw_name);
> >
> > Both of them were just freed in the loop above, no?
> for (i = 0, parent = parents; i < num_parents; i++, parent++)
> Is 'parent' the same as the one from the loop above?
Yes. Did it change somehow?
>
> Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
> kfree_const()?
>
Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
they're NULL by virtue of the kcalloc and then kfree_const() does
nothing.
I'm having a hard time following what this patch is trying to fix. It
looks unnecessary though so I'm going to drop it from the clk review
queue.
On Thu, Jun 06, 2019 at 01:16:45PM -0700, Stephen Boyd wrote:
> Quoting Gen Zhang (2019-06-05 09:00:43)
> > On Wed, Jun 05, 2019 at 08:38:00AM +0200, Jiri Slaby wrote:
> > > On 31. 05. 19, 3:14, Gen Zhang wrote:
> > > > In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and
> > > > 'dst' are allcoted by kstrdup_const(). According to doc: "Strings
> > > > allocated by kstrdup_const should be freed by kfree_const". So
> > > > 'parent->name', 'parent->fw_name' and 'dst' should be freed.
> > > >
> > > > Signed-off-by: Gen Zhang <[email protected]>
> > > > ---
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index aa51756..85c4d3f 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> > > > if (!dst)
> > > > return -ENOMEM;
> > > >
> > > > + kfree_const(dst);
> > >
> > > So you are now returning a freed pointer in dst_p?
> > Thanks for your reply. I re-examined the code, and this kfree is
> > incorrect and it should be deleted.
> > >
> > > > return 0;
> > > > }
> > > >
> > > > @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> > > > kfree_const(parents[i].name);
> > > > kfree_const(parents[i].fw_name);
> > > > } while (--i >= 0);
> > > > + kfree_const(parent->name);
> > > > + kfree_const(parent->fw_name);
> > >
> > > Both of them were just freed in the loop above, no?
> > for (i = 0, parent = parents; i < num_parents; i++, parent++)
> > Is 'parent' the same as the one from the loop above?
>
> Yes. Did it change somehow?
parent++?
>
> >
> > Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
> > kfree_const()?
> >
>
> Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
> they're NULL by virtue of the kcalloc and then kfree_const() does
> nothing.
I re-examined clk_cpy_name(). They are the second parameter of
clk_cpy_name(). The first parameter is allocated, not the second one.
So 'parent->name' and 'parent->fw_name' should be freed, not
'parents[i].name' or 'parents[i].fw_name'. Am I totally misunderstanding
this clk_cpy_name()? :-(
Thanks
Gen
>
> I'm having a hard time following what this patch is trying to fix. It
> looks unnecessary though so I'm going to drop it from the clk review
> queue.
>
On 07. 06. 19, 3:52, Gen Zhang wrote:
>>>>> @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
>>>>> kfree_const(parents[i].name);
>>>>> kfree_const(parents[i].fw_name);
>>>>> } while (--i >= 0);
>>>>> + kfree_const(parent->name);
>>>>> + kfree_const(parent->fw_name);
>>>>
>>>> Both of them were just freed in the loop above, no?
>>> for (i = 0, parent = parents; i < num_parents; i++, parent++)
>>> Is 'parent' the same as the one from the loop above?
>>
>> Yes. Did it change somehow?
> parent++?
parent++ is done after the loop body. Or what do you mean?
>>> Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
>>> kfree_const()?
>>>
>>
>> Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
>> they're NULL by virtue of the kcalloc and then kfree_const() does
>> nothing.
> I re-examined clk_cpy_name(). They are the second parameter of
> clk_cpy_name(). The first parameter is allocated, not the second one.
> So 'parent->name' and 'parent->fw_name' should be freed, not
> 'parents[i].name' or 'parents[i].fw_name'. Am I totally misunderstanding
> this clk_cpy_name()? :-(
The second parameter (the source) is parent_data[i].*, not parents[i].*
(the destination). parent->fw_name and parent->name are properly freed
in the do {} while loop as parents[i].name and parents[i].fw_name, given
i hasn't changed yet. I am not sure what you mean at all. Are you
uncertain about the C code flow?
thanks,
--
js
suse labs
On Fri, Jun 07, 2019 at 11:10:37AM +0200, Jiri Slaby wrote:
> On 07. 06. 19, 3:52, Gen Zhang wrote:
> >>>>> @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> >>>>> kfree_const(parents[i].name);
> >>>>> kfree_const(parents[i].fw_name);
> >>>>> } while (--i >= 0);
> >>>>> + kfree_const(parent->name);
> >>>>> + kfree_const(parent->fw_name);
> >>>>
> >>>> Both of them were just freed in the loop above, no?
> >>> for (i = 0, parent = parents; i < num_parents; i++, parent++)
> >>> Is 'parent' the same as the one from the loop above?
> >>
> >> Yes. Did it change somehow?
> > parent++?
>
> parent++ is done after the loop body. Or what do you mean?
>
> >>> Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
> >>> kfree_const()?
> >>>
> >>
> >> Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
> >> they're NULL by virtue of the kcalloc and then kfree_const() does
> >> nothing.
> > I re-examined clk_cpy_name(). They are the second parameter of
> > clk_cpy_name(). The first parameter is allocated, not the second one.
> > So 'parent->name' and 'parent->fw_name' should be freed, not
> > 'parents[i].name' or 'parents[i].fw_name'. Am I totally misunderstanding
> > this clk_cpy_name()? :-(
>
> The second parameter (the source) is parent_data[i].*, not parents[i].*
> (the destination). parent->fw_name and parent->name are properly freed
> in the do {} while loop as parents[i].name and parents[i].fw_name, given
> i hasn't changed yet. I am not sure what you mean at all. Are you
> uncertain about the C code flow?
>
> thanks,
> --
> js
> suse labs
Thanks your patient explainaton. I think I need some time to figure out
this part of code.
Thanks
Gen