dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Ben Skeggs, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Ben Skeggs, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Ben Skeggs, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Ben Skeggs, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Markus Elfring, Ben Skeggs, Kangjie Lu

The original patch was basically fine.  Just add a Fixes tag and resend.

regards,
dan carpenter

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Ben Skeggs, Dinghao Liu, 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, nouveau, kernel-janitors, linux-kernel, dri-devel,
	Markus Elfring, Ben Skeggs, 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ 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: David Airlie, Kangjie Lu, kernel-janitors, linux-kernel, Ben Skeggs

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-06-03  7: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).