* Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
@ 2020-05-31 8:03 Markus Elfring
2020-05-31 8:23 ` dinghao.liu
0 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2020-05-31 8:03 UTC (permalink / raw)
To: Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> When gk20a_clk_ctor() returns an error code, pointer "clk"
> should be released.
Such an information is reasonable.
> It's the same when gm20b_clk_new() returns from elsewhere following this call.
I suggest to reconsider the interpretation of the software situation once more.
Can it be that the allocated clock object should be kept usable even after
a successful return from this function?
Would you like to add the tag “Fixes” to the commit message?
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 8:03 [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Markus Elfring
@ 2020-05-31 8:23 ` dinghao.liu
2020-05-31 8:38 ` Markus Elfring
2020-06-02 10:29 ` Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Dan Carpenter
0 siblings, 2 replies; 14+ messages in thread
From: dinghao.liu @ 2020-05-31 8:23 UTC (permalink / raw)
To: Markus Elfring
Cc: dri-devel, nouveau, kernel-janitors, linux-kernel, Ben Skeggs,
David Airlie, Kangjie Lu
>
> > It's the same when gm20b_clk_new() returns from elsewhere following this call.
>
> I suggest to reconsider the interpretation of the software situation once more.
> Can it be that the allocated clock object should be kept usable even after
> a successful return from this function?
>
It's possible that we expect an usable clk pointer, though I could not find
the exact usage yet. For security, I will release this pointer only on error
paths in this function.
>
> Would you like to add the tag “Fixes” to the commit message?
>
Thank you for your advice! I will add this tag in the next version of patch.
Regards,
Dinghao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 8:23 ` dinghao.liu
@ 2020-05-31 8:38 ` Markus Elfring
2020-05-31 8:52 ` dinghao.liu
2020-06-02 10:29 ` Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Dan Carpenter
1 sibling, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2020-05-31 8:38 UTC (permalink / raw)
To: Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> It's possible that we expect an usable clk pointer, though I could not find
> the exact usage yet.
I am curious if another developer would like to add helpful background information.
> For security, I will release this pointer only on error paths in this function.
Do you tend to release objects (which are referenced by pointers)?
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 8:38 ` Markus Elfring
@ 2020-05-31 8:52 ` dinghao.liu
2020-05-31 9:00 ` Markus Elfring
0 siblings, 1 reply; 14+ messages in thread
From: dinghao.liu @ 2020-05-31 8:52 UTC (permalink / raw)
To: Markus Elfring
Cc: dri-devel, nouveau, kernel-janitors, linux-kernel, Ben Skeggs,
David Airlie, Kangjie Lu
>
> > For security, I will release this pointer only on error paths in this function.
>
> Do you tend to release objects (which are referenced by pointers)?
>
I just found that clk is referenced by pclk in this function. When clk is freed,
pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk
in this function and there is no bug here. Thank you for reminding me!
Regards,
Dinghao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 8:52 ` dinghao.liu
@ 2020-05-31 9:00 ` Markus Elfring
2020-05-31 9:15 ` dinghao.liu
0 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2020-05-31 9:00 UTC (permalink / raw)
To: Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> I just found that clk is referenced by pclk in this function. When clk is freed,
> pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk
> in this function and there is no bug here.
Can there be a need to release a clock object after a failed gk20a_clk_ctor() call?
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 9:00 ` Markus Elfring
@ 2020-05-31 9:15 ` dinghao.liu
2020-05-31 9:51 ` Markus Elfring
0 siblings, 1 reply; 14+ messages in thread
From: dinghao.liu @ 2020-05-31 9:15 UTC (permalink / raw)
To: Markus Elfring
Cc: dri-devel, nouveau, kernel-janitors, linux-kernel, Ben Skeggs,
David Airlie, Kangjie Lu
> > I just found that clk is referenced by pclk in this function. When clk is freed,
> > pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk
> > in this function and there is no bug here.
>
> Can there be a need to release a clock object after a failed gk20a_clk_ctor() call?
>
I think this mainly depends on pclk pointer. It seems that the caller of
gm20b_clk_new() always expects pclk to be allocated unless it returns -ENOMEM,
which means kzalloc() failed. If gk20a_clk_ctor() never returns such an error
code, we may need not to release this clock object.
Regards,
Dinghao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 9:15 ` dinghao.liu
@ 2020-05-31 9:51 ` Markus Elfring
2020-05-31 10:42 ` dinghao.liu
0 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2020-05-31 9:51 UTC (permalink / raw)
To: Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> If gk20a_clk_ctor() never returns such an error code,
> we may need not to release this clock object.
Would you like to achieve complete exception handling
also for this function implementation?
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 9:51 ` Markus Elfring
@ 2020-05-31 10:42 ` dinghao.liu
2020-05-31 12:14 ` drm/nouveau/clk/gm20b: Fix memory leaks after failed gk20a_clk_ctor() calls Markus Elfring
0 siblings, 1 reply; 14+ messages in thread
From: dinghao.liu @ 2020-05-31 10:42 UTC (permalink / raw)
To: Markus Elfring
Cc: dri-devel, nouveau, kernel-janitors, linux-kernel, Ben Skeggs,
David Airlie, Kangjie Lu
> > If gk20a_clk_ctor() never returns such an error code,
> > we may need not to release this clock object.
>
> Would you like to achieve complete exception handling
> also for this function implementation?
>
It seems that it's possible to get -ENOMEM from gk20a_clk_ctor().
The call chain is as follows:
gk20a_clk_ctor() <- nvkm_clk_ctor() <- nvkm_notify_init()
When nvkm_notify_init() returns -ENOMEM, all of its callers (and
callers of callers) will be influenced if there is a failed
kzalloc inside which.
In this case, maybe we should check the return value of
gk20a_clk_ctor() and release clk if it returns -ENOMEM.
And many other functions also have the same issue (e.g.,
gm20b_clk_new_speedo0). Do you have any idea about this
problem?
Regards,
Dinghao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: drm/nouveau/clk/gm20b: Fix memory leaks after failed gk20a_clk_ctor() calls
2020-05-31 10:42 ` dinghao.liu
@ 2020-05-31 12:14 ` Markus Elfring
0 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2020-05-31 12:14 UTC (permalink / raw)
To: Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> In this case, maybe we should check the return value of
> gk20a_clk_ctor() and release clk if it returns -ENOMEM.
All error situations (including failed memory allocations) can matter here.
> And many other functions also have the same issue
> (e.g. gm20b_clk_new_speedo0).
I recommend to increase the error detection and improve the desired
exception handling accordingly.
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-05-31 8:23 ` dinghao.liu
2020-05-31 8:38 ` Markus Elfring
@ 2020-06-02 10:29 ` Dan Carpenter
2020-06-02 11:10 ` Markus Elfring
1 sibling, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2020-06-02 10:29 UTC (permalink / raw)
To: dinghao.liu
Cc: Markus Elfring, dri-devel, nouveau, kernel-janitors,
linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
The original patch was basically fine. Just add a Fixes tag and resend.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-06-02 10:29 ` Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Dan Carpenter
@ 2020-06-02 11:10 ` Markus Elfring
2020-06-02 15:39 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Markus Elfring @ 2020-06-02 11:10 UTC (permalink / raw)
To: Dan Carpenter, Dinghao Liu, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> The original patch was basically fine.
I propose to reconsider the interpretation of the software situation once more.
* Should the allocated clock object be kept usable even after
a successful return from this function?
* How much do “destructor” calls matter here for (sub)devices?
> Just add a Fixes tag and resend.
This tag can help also.
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-06-02 11:10 ` Markus Elfring
@ 2020-06-02 15:39 ` Dan Carpenter
2020-06-03 2:21 ` dinghao.liu
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2020-06-02 15:39 UTC (permalink / raw)
To: Markus Elfring
Cc: Dinghao Liu, dri-devel, nouveau, kernel-janitors, linux-kernel,
Ben Skeggs, David Airlie, Kangjie Lu
On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote:
> > The original patch was basically fine.
>
> I propose to reconsider the interpretation of the software situation once more.
>
> * Should the allocated clock object be kept usable even after
> a successful return from this function?
Heh. You're right. The patch is freeing "clk" on the success path so
that doesn't work.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
2020-06-02 15:39 ` Dan Carpenter
@ 2020-06-03 2:21 ` dinghao.liu
2020-06-03 5:04 ` drm/nouveau/clk/gm20b: Understanding challenges around gm20b_clk_new() Markus Elfring
0 siblings, 1 reply; 14+ messages in thread
From: dinghao.liu @ 2020-06-03 2:21 UTC (permalink / raw)
To: Dan Carpenter
Cc: Markus Elfring, dri-devel, nouveau, kernel-janitors,
linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote:
> > > The original patch was basically fine.
> >
> > I propose to reconsider the interpretation of the software situation once more.
> >
> > * Should the allocated clock object be kept usable even after
> > a successful return from this function?
>
> Heh. You're right. The patch is freeing "clk" on the success path so
> that doesn't work.
>
Ben has explained this problem:
https://lore.kernel.org/patchwork/patch/1249592/
Since the caller will check "pclk" on failure, we don't need to free
"clk" in gm20b_clk_new() and I think this patch is no longer needed.
Regards,
Dinghao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: drm/nouveau/clk/gm20b: Understanding challenges around gm20b_clk_new()
2020-06-03 2:21 ` dinghao.liu
@ 2020-06-03 5:04 ` Markus Elfring
0 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2020-06-03 5:04 UTC (permalink / raw)
To: Dinghao Liu, Dan Carpenter, dri-devel, nouveau
Cc: kernel-janitors, linux-kernel, Ben Skeggs, David Airlie, Kangjie Lu
> Ben has explained this problem:
> https://lore.kernel.org/patchwork/patch/1249592/
> Since the caller will check "pclk" on failure, we don't need to free
> "clk" in gm20b_clk_new() and I think this patch is no longer needed.
* I am curious if it can become easier to see the relationships for
these variables according to mentioned “destructor” calls.
* Did you notice opportunities to improve source code analysis
(or software documentation) accordingly?
Regards,
Markus
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-03 5:05 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-31 8:03 [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Markus Elfring
2020-05-31 8:23 ` dinghao.liu
2020-05-31 8:38 ` Markus Elfring
2020-05-31 8:52 ` dinghao.liu
2020-05-31 9:00 ` Markus Elfring
2020-05-31 9:15 ` dinghao.liu
2020-05-31 9:51 ` Markus Elfring
2020-05-31 10:42 ` dinghao.liu
2020-05-31 12:14 ` drm/nouveau/clk/gm20b: Fix memory leaks after failed gk20a_clk_ctor() calls Markus Elfring
2020-06-02 10:29 ` Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new() Dan Carpenter
2020-06-02 11:10 ` Markus Elfring
2020-06-02 15:39 ` Dan Carpenter
2020-06-03 2:21 ` dinghao.liu
2020-06-03 5:04 ` drm/nouveau/clk/gm20b: Understanding challenges around gm20b_clk_new() Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).