| Message ID | 20220130210210.549877-3-daniel.lezcano@linaro.org |
|---|---|
| State | Accepted |
| Commit | 690de0b4013f6f35bc9fced12746b9f396c471ae |
| Headers | show |
| Series | [v1,1/7] powercap/dtpm: Change locking scheme | expand |
On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > When the node is virtual there is no release function associated which > can free the memory. > > Free the memory when no 'ops' exists. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/powercap/dtpm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c > index 0b0121c37a1b..7bddd25a6767 100644 > --- a/drivers/powercap/dtpm.c > +++ b/drivers/powercap/dtpm.c > @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) > > if (dtpm->ops) > dtpm->ops->release(dtpm); > + else > + kfree(dtpm); > This doesn't look correct. Below you check dtpm against "root", which may be after its memory has been freed. If the ->release() function should be responsible for freeing the dtpm, it needs to be called after the check below. > if (root == dtpm) > root = NULL; > > - kfree(dtpm); > - > return 0; > } > Kind regards Uffe
On 16/02/2022 17:22, Ulf Hansson wrote: > On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> When the node is virtual there is no release function associated which >> can free the memory. >> >> Free the memory when no 'ops' exists. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/powercap/dtpm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c >> index 0b0121c37a1b..7bddd25a6767 100644 >> --- a/drivers/powercap/dtpm.c >> +++ b/drivers/powercap/dtpm.c >> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) >> >> if (dtpm->ops) >> dtpm->ops->release(dtpm); >> + else >> + kfree(dtpm); >> > > This doesn't look correct. Below you check dtpm against "root", which > may be after its memory has been freed. > > If the ->release() function should be responsible for freeing the > dtpm, it needs to be called after the check below. It is harmless, 'root' is not dereferenced but used as an ID Moreover, in the patch 5/7 it is moved out this function. >> if (root == dtpm) >> root = NULL; >> >> - kfree(dtpm); >> - >> return 0; >> } >> > > Kind regards > Uffe
On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 16/02/2022 17:22, Ulf Hansson wrote: > > On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> When the node is virtual there is no release function associated which > >> can free the memory. > >> > >> Free the memory when no 'ops' exists. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> --- > >> drivers/powercap/dtpm.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c > >> index 0b0121c37a1b..7bddd25a6767 100644 > >> --- a/drivers/powercap/dtpm.c > >> +++ b/drivers/powercap/dtpm.c > >> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) > >> > >> if (dtpm->ops) > >> dtpm->ops->release(dtpm); > >> + else > >> + kfree(dtpm); > >> > > > > This doesn't look correct. Below you check dtpm against "root", which > > may be after its memory has been freed. > > > > If the ->release() function should be responsible for freeing the > > dtpm, it needs to be called after the check below. > > It is harmless, 'root' is not dereferenced but used as an ID > > Moreover, in the patch 5/7 it is moved out this function. Right. It just looks a bit odd here. > > > >> if (root == dtpm) > >> root = NULL; > >> > >> - kfree(dtpm); So then why doesn't this kfree do the job already? kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data. > >> - > >> return 0; > >> } > >> Kind regards Uffe
On 17/02/2022 14:17, Ulf Hansson wrote: > On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 16/02/2022 17:22, Ulf Hansson wrote: >>> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >>>> >>>> When the node is virtual there is no release function associated which >>>> can free the memory. >>>> >>>> Free the memory when no 'ops' exists. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/powercap/dtpm.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c >>>> index 0b0121c37a1b..7bddd25a6767 100644 >>>> --- a/drivers/powercap/dtpm.c >>>> +++ b/drivers/powercap/dtpm.c >>>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) >>>> >>>> if (dtpm->ops) >>>> dtpm->ops->release(dtpm); >>>> + else >>>> + kfree(dtpm); >>>> >>> >>> This doesn't look correct. Below you check dtpm against "root", which >>> may be after its memory has been freed. >>> >>> If the ->release() function should be responsible for freeing the >>> dtpm, it needs to be called after the check below. >> >> It is harmless, 'root' is not dereferenced but used as an ID >> >> Moreover, in the patch 5/7 it is moved out this function. > > Right. It just looks a bit odd here. > >> >> >>>> if (root == dtpm) >>>> root = NULL; >>>> >>>> - kfree(dtpm); > > So then why doesn't this kfree do the job already? > > kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data. The description is confusing. Actually, there is a double kfree. When there is a ops->release, the kfree is done there and again a few lines after. The issue was introduced with the change where dtpm had a private data field to store the backend specific structure and was converted to a backend specific structure containing a dtpm node [1]. So this function was calling release from the dtpm backend which was freeing the specific data in the dtpm->private and then here was freeing the dtpm. Now, the backend frees the structure which contains the dtpm structure, so when returning from ops->release(), dtpm is already free. I should change the description and add a Fixes tag to the change described above. [1] https://lore.kernel.org/r/20210312130411.29833-4-daniel.lezcano@linaro.org
On Thu, 17 Feb 2022 at 14:54, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 17/02/2022 14:17, Ulf Hansson wrote: > > On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> On 16/02/2022 17:22, Ulf Hansson wrote: > >>> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >>>> > >>>> When the node is virtual there is no release function associated which > >>>> can free the memory. > >>>> > >>>> Free the memory when no 'ops' exists. > >>>> > >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >>>> --- > >>>> drivers/powercap/dtpm.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c > >>>> index 0b0121c37a1b..7bddd25a6767 100644 > >>>> --- a/drivers/powercap/dtpm.c > >>>> +++ b/drivers/powercap/dtpm.c > >>>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) > >>>> > >>>> if (dtpm->ops) > >>>> dtpm->ops->release(dtpm); > >>>> + else > >>>> + kfree(dtpm); > >>>> > >>> > >>> This doesn't look correct. Below you check dtpm against "root", which > >>> may be after its memory has been freed. > >>> > >>> If the ->release() function should be responsible for freeing the > >>> dtpm, it needs to be called after the check below. > >> > >> It is harmless, 'root' is not dereferenced but used as an ID > >> > >> Moreover, in the patch 5/7 it is moved out this function. > > > > Right. It just looks a bit odd here. > > > >> > >> > >>>> if (root == dtpm) > >>>> root = NULL; > >>>> > >>>> - kfree(dtpm); > > > > So then why doesn't this kfree do the job already? > > > > kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data. > > The description is confusing. > > Actually, there is a double kfree. When there is a ops->release, the > kfree is done there and again a few lines after. > > The issue was introduced with the change where dtpm had a private data > field to store the backend specific structure and was converted to a > backend specific structure containing a dtpm node [1]. > > So this function was calling release from the dtpm backend which was > freeing the specific data in the dtpm->private and then here was freeing > the dtpm. Now, the backend frees the structure which contains the dtpm > structure, so when returning from ops->release(), dtpm is already free. > > I should change the description and add a Fixes tag to the change > described above. Does ops->release() also resets the "dtpm" pointer to NULL? If not, it's good practice that it should, right? In that case, we would be calling "kfree(NULL);" the second time, which is perfectly fine. > > [1] > https://lore.kernel.org/r/20210312130411.29833-4-daniel.lezcano@linaro.org > Kind regards Uffe
On 17/02/2022 16:45, Ulf Hansson wrote: [ ... ] > Does ops->release() also resets the "dtpm" pointer to NULL? If not, > it's good practice that it should, right? > > In that case, we would be calling "kfree(NULL);" the second time, > which is perfectly fine. So you suggest to replace: if (ops->release) ops->release(dtpm); else kfree(dtpm); By: if (ops->release) { ops->release(dtpm); dtpm = NULL; } kfree(dtpm); ?
On Fri, 18 Feb 2022 at 14:18, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 17/02/2022 16:45, Ulf Hansson wrote: > > [ ... ] > > > Does ops->release() also resets the "dtpm" pointer to NULL? If not, > > it's good practice that it should, right? > > > > In that case, we would be calling "kfree(NULL);" the second time, > > which is perfectly fine. > > So you suggest to replace: > > if (ops->release) > ops->release(dtpm); > else > kfree(dtpm); > > By: > > if (ops->release) { > ops->release(dtpm); > dtpm = NULL; > } > I don't have a strong opinion how to code this. What I was trying to point out was that if ->ops->release() frees the memory it could/should also reset the pointer to NULL. And if that is already done, the kfree below is harmless and there would be nothing to "fix". > kfree(dtpm); > > ? Kind regards Uffe
On 22/02/2022 16:55, Ulf Hansson wrote: > On Fri, 18 Feb 2022 at 14:18, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 17/02/2022 16:45, Ulf Hansson wrote: >> >> [ ... ] >> >>> Does ops->release() also resets the "dtpm" pointer to NULL? If not, >>> it's good practice that it should, right? >>> >>> In that case, we would be calling "kfree(NULL);" the second time, >>> which is perfectly fine. >> >> So you suggest to replace: >> >> if (ops->release) >> ops->release(dtpm); >> else >> kfree(dtpm); >> >> By: >> >> if (ops->release) { >> ops->release(dtpm); >> dtpm = NULL; >> } >> > > I don't have a strong opinion how to code this. > > What I was trying to point out was that if ->ops->release() frees the > memory it could/should also reset the pointer to NULL No it can't because it is not a pointer, it is contained by the backend specific structure. eg. struct dtpm_cpu { struct dtpm dtpm; }; the release frees a dtpm_cpu structure. > And if that is already done, the kfree below is harmless and there > would be nothing to "fix". > >> kfree(dtpm); >> >> ? > > Kind regards > Uffe
diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c index 0b0121c37a1b..7bddd25a6767 100644 --- a/drivers/powercap/dtpm.c +++ b/drivers/powercap/dtpm.c @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz) if (dtpm->ops) dtpm->ops->release(dtpm); + else + kfree(dtpm); if (root == dtpm) root = NULL; - kfree(dtpm); - return 0; }
When the node is virtual there is no release function associated which can free the memory. Free the memory when no 'ops' exists. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/powercap/dtpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)