Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Markus Elfring]

> 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

Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[dinghao.liu]

PiAKPiA+IEl0J3MgdGhlIHNhbWUgd2hlbiBnbTIwYl9jbGtfbmV3KCkgcmV0dXJucyBmcm9tIGVs
c2V3aGVyZSBmb2xsb3dpbmcgdGhpcyBjYWxsLgo+IAo+IEkgc3VnZ2VzdCB0byByZWNvbnNpZGVy
IHRoZSBpbnRlcnByZXRhdGlvbiBvZiB0aGUgc29mdHdhcmUgc2l0dWF0aW9uIG9uY2UgbW9yZS4K
PiBDYW4gaXQgYmUgdGhhdCB0aGUgYWxsb2NhdGVkIGNsb2NrIG9iamVjdCBzaG91bGQgYmUga2Vw
dCB1c2FibGUgZXZlbiBhZnRlcgo+IGEgc3VjY2Vzc2Z1bCByZXR1cm4gZnJvbSB0aGlzIGZ1bmN0
aW9uPwo+IAoKSXQncyBwb3NzaWJsZSB0aGF0IHdlIGV4cGVjdCBhbiB1c2FibGUgY2xrIHBvaW50
ZXIsIHRob3VnaCBJIGNvdWxkIG5vdCBmaW5kCnRoZSBleGFjdCB1c2FnZSB5ZXQuIEZvciBzZWN1
cml0eSwgSSB3aWxsIHJlbGVhc2UgdGhpcyBwb2ludGVyIG9ubHkgb24gZXJyb3IgCnBhdGhzIGlu
IHRoaXMgZnVuY3Rpb24uCgo+IAo+IFdvdWxkIHlvdSBsaWtlIHRvIGFkZCB0aGUgdGFnIOKAnEZp
eGVz4oCdIHRvIHRoZSBjb21taXQgbWVzc2FnZT8KPiAKClRoYW5rIHlvdSBmb3IgeW91ciBhZHZp
Y2UhIEkgd2lsbCBhZGQgdGhpcyB0YWcgaW4gdGhlIG5leHQgdmVyc2lvbiBvZiBwYXRjaC4KClJl
Z2FyZHMsCkRpbmdoYW8

Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Markus Elfring]

> 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

Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[dinghao.liu]

PiAKPiA+IEZvciBzZWN1cml0eSwgSSB3aWxsIHJlbGVhc2UgdGhpcyBwb2ludGVyIG9ubHkgb24g
ZXJyb3IgcGF0aHMgaW4gdGhpcyBmdW5jdGlvbi4KPiAKPiBEbyB5b3UgdGVuZCB0byByZWxlYXNl
IG9iamVjdHMgKHdoaWNoIGFyZSByZWZlcmVuY2VkIGJ5IHBvaW50ZXJzKT8KPiAKCkkganVzdCBm
b3VuZCB0aGF0IGNsayBpcyByZWZlcmVuY2VkIGJ5IHBjbGsgaW4gdGhpcyBmdW5jdGlvbi4gV2hl
biBjbGsgaXMgZnJlZWQsIApwY2xrIHdpbGwgYmUgYWxsb2NhdGVkIGluIGdtMjBiX2Nsa19uZXdf
c3BlZWRvMCgpLiBUaHVzIHdlIHNob3VsZCBub3QgcmVsZWFzZSBjbGsKaW4gdGhpcyBmdW5jdGlv
biBhbmQgdGhlcmUgaXMgbm8gYnVnIGhlcmUuIFRoYW5rIHlvdSBmb3IgcmVtaW5kaW5nIG1lIQoK
UmVnYXJkcywKRGluZ2hhbw=

Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Markus Elfring]

> 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

Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[dinghao.liu]

PiA+IEkganVzdCBmb3VuZCB0aGF0IGNsayBpcyByZWZlcmVuY2VkIGJ5IHBjbGsgaW4gdGhpcyBm
dW5jdGlvbi4gV2hlbiBjbGsgaXMgZnJlZWQsCj4gPiBwY2xrIHdpbGwgYmUgYWxsb2NhdGVkIGlu
IGdtMjBiX2Nsa19uZXdfc3BlZWRvMCgpLiBUaHVzIHdlIHNob3VsZCBub3QgcmVsZWFzZSBjbGsK
PiA+IGluIHRoaXMgZnVuY3Rpb24gYW5kIHRoZXJlIGlzIG5vIGJ1ZyBoZXJlLgo+IAo+IENhbiB0
aGVyZSBiZSBhIG5lZWQgdG8gcmVsZWFzZSBhIGNsb2NrIG9iamVjdCBhZnRlciBhIGZhaWxlZCBn
azIwYV9jbGtfY3RvcigpIGNhbGw/Cj4gCgpJIHRoaW5rIHRoaXMgbWFpbmx5IGRlcGVuZHMgb24g
cGNsayBwb2ludGVyLiBJdCBzZWVtcyB0aGF0IHRoZSBjYWxsZXIgb2YgCmdtMjBiX2Nsa19uZXco
KSBhbHdheXMgZXhwZWN0cyBwY2xrIHRvIGJlIGFsbG9jYXRlZCB1bmxlc3MgaXQgcmV0dXJucyAt
RU5PTUVNLAp3aGljaCBtZWFucyBremFsbG9jKCkgZmFpbGVkLiBJZiBnazIwYV9jbGtfY3Rvcigp
IG5ldmVyIHJldHVybnMgc3VjaCBhbiBlcnJvciAKY29kZSwgd2UgbWF5IG5lZWQgbm90IHRvIHJl
bGVhc2UgdGhpcyBjbG9jayBvYmplY3QuCgpSZWdhcmRzLApEaW5naGFv

Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Markus Elfring]

> 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

Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[dinghao.liu]

PiA+IElmIGdrMjBhX2Nsa19jdG9yKCkgbmV2ZXIgcmV0dXJucyBzdWNoIGFuIGVycm9yIGNvZGUs
Cj4gPiB3ZSBtYXkgbmVlZCBub3QgdG8gcmVsZWFzZSB0aGlzIGNsb2NrIG9iamVjdC4KPiAKPiBX
b3VsZCB5b3UgbGlrZSB0byBhY2hpZXZlIGNvbXBsZXRlIGV4Y2VwdGlvbiBoYW5kbGluZwo+IGFs
c28gZm9yIHRoaXMgZnVuY3Rpb24gaW1wbGVtZW50YXRpb24/Cj4gCgpJdCBzZWVtcyB0aGF0IGl0
J3MgcG9zc2libGUgdG8gZ2V0IC1FTk9NRU0gZnJvbSBnazIwYV9jbGtfY3RvcigpLgpUaGUgY2Fs
bCBjaGFpbiBpcyBhcyBmb2xsb3dzOgpnazIwYV9jbGtfY3RvcigpIDwtIG52a21fY2xrX2N0b3Io
KSA8LSBudmttX25vdGlmeV9pbml0KCkKCldoZW4gbnZrbV9ub3RpZnlfaW5pdCgpIHJldHVybnMg
LUVOT01FTSwgYWxsIG9mIGl0cyBjYWxsZXJzIChhbmQgCmNhbGxlcnMgb2YgY2FsbGVycykgd2ls
bCBiZSBpbmZsdWVuY2VkIGlmIHRoZXJlIGlzIGEgZmFpbGVkCmt6YWxsb2MgaW5zaWRlIHdoaWNo
LiAKCkluIHRoaXMgY2FzZSwgbWF5YmUgd2Ugc2hvdWxkIGNoZWNrIHRoZSByZXR1cm4gdmFsdWUg
b2YgCmdrMjBhX2Nsa19jdG9yKCkgYW5kIHJlbGVhc2UgY2xrIGlmIGl0IHJldHVybnMgLUVOT01F
TS4gCkFuZCBtYW55IG90aGVyIGZ1bmN0aW9ucyBhbHNvIGhhdmUgdGhlIHNhbWUgaXNzdWUgKGUu
Zy4sCmdtMjBiX2Nsa19uZXdfc3BlZWRvMCkuIERvIHlvdSBoYXZlIGFueSBpZGVhIGFib3V0IHRo
aXMgCnByb2JsZW0/CgpSZWdhcmRzLApEaW5naGFv

Re: drm/nouveau/clk/gm20b: Fix memory leaks after failed gk20a_clk_ctor() calls

[Markus Elfring]

> 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

Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Dan Carpenter]

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

regards,
dan carpenter

Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Markus Elfring]

> 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

Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[Dan Carpenter]

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

Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()

[dinghao.liu]

Cj4gT24gVHVlLCBKdW4gMDIsIDIwMjAgYXQgMDE6MTA6MzRQTSArMDIwMCwgTWFya3VzIEVsZnJp
bmcgd3JvdGU6Cj4gPiA+IFRoZSBvcmlnaW5hbCBwYXRjaCB3YXMgYmFzaWNhbGx5IGZpbmUuCj4g
PiAKPiA+IEkgcHJvcG9zZSB0byByZWNvbnNpZGVyIHRoZSBpbnRlcnByZXRhdGlvbiBvZiB0aGUg
c29mdHdhcmUgc2l0dWF0aW9uIG9uY2UgbW9yZS4KPiA+IAo+ID4gKiBTaG91bGQgdGhlIGFsbG9j
YXRlZCBjbG9jayBvYmplY3QgYmUga2VwdCB1c2FibGUgZXZlbiBhZnRlcgo+ID4gICBhIHN1Y2Nl
c3NmdWwgcmV0dXJuIGZyb20gdGhpcyBmdW5jdGlvbj8KPiAKPiBIZWguICBZb3UncmUgcmlnaHQu
ICBUaGUgcGF0Y2ggaXMgZnJlZWluZyAiY2xrIiBvbiB0aGUgc3VjY2VzcyBwYXRoIHNvCj4gdGhh
dCBkb2Vzbid0IHdvcmsuCj4gCgpCZW4gaGFzIGV4cGxhaW5lZCB0aGlzIHByb2JsZW06Cmh0dHBz
Oi8vbG9yZS5rZXJuZWwub3JnL3BhdGNod29yay9wYXRjaC8xMjQ5NTkyLwpTaW5jZSB0aGUgY2Fs
bGVyIHdpbGwgY2hlY2sgInBjbGsiIG9uIGZhaWx1cmUsIHdlIGRvbid0IG5lZWQgdG8gZnJlZQoi
Y2xrIiBpbiBnbTIwYl9jbGtfbmV3KCkgYW5kIEkgdGhpbmsgdGhpcyBwYXRjaCBpcyBubyBsb25n
ZXIgbmVlZGVkLgoKUmVnYXJkcywKRGluZ2hhbw=

Re: drm/nouveau/clk/gm20b: Understanding challenges around gm20b_clk_new()

[Markus Elfring]

> 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