Re: [PATCH v2] afs: Fix memory leak in afs_put_sysnames()

Re: [PATCH v2] afs: Fix memory leak in afs_put_sysnames()

[Markus Elfring]

> sysnames should be freed after refcnt being decreased to zero in
> afs_put_sysnames().

How do you think about a wording variant like the following?

   Release the sysnames object after its reference counter was decreased
   to zero.


Will it matter to mention the size of the data structure "afs_sysnames"?

Regards,
Markus

Re: [PATCH v2] afs: Fix memory leak in afs_put_sysnames()

[David Howells]

Markus Elfring <Markus.Elfring@web.de> wrote:

> > sysnames should be freed after refcnt being decreased to zero in
> > afs_put_sysnames().
> 
> How do you think about a wording variant like the following?
> 
>    Release the sysnames object after its reference counter was decreased
>    to zero.

I would say "reference count" not "reference counter" personally.  I would
also say "afs_sysnames" rather than "sysnames".  Perhaps something like:

	Fix afs_put_sysnames() to actually free the specified afs_sysnames
	object after its reference count has been decreased to zero and its
	contents have been released.

> Will it matter to mention the size of the data structure "afs_sysnames"?

Why is it necessary to do so?

David

Re: [v2] afs: Fix memory leak in afs_put_sysnames()

[Markus Elfring]

> Perhaps something like:
>
> 	Fix afs_put_sysnames() to actually free the specified afs_sysnames
> 	object after its reference count has been decreased to zero and its
> 	contents have been released.

* How do you think about to omit the word "Fix" because of the provided tag?

* Is freeing and releasing an item a duplicate operation anyhow?


>> Will it matter to mention the size of the data structure "afs_sysnames"?
>
> Why is it necessary to do so?

I suggest to express the impact of the missed function call "kfree".

Regards,
Markus

Re: [v2] afs: Fix memory leak in afs_put_sysnames()

[David Howells]

Markus Elfring <Markus.Elfring@web.de> wrote:

> > 	Fix afs_put_sysnames() to actually free the specified afs_sysnames
> > 	object after its reference count has been decreased to zero and its
> > 	contents have been released.
> 
> * How do you think about to omit the word "Fix" because of the provided tag?

Quite often I might put introductory paragraphs before that, so I prefer to
begin the paragraph that states a fix with that verb.  There may also be
auxiliary changes associated with it that aren't directly fixes but need to be
made because of the fix change.

> * Is freeing and releasing an item a duplicate operation anyhow?

You're missing the point.  afs_put_sysnames() does release the things the
object points to (ie. the content), but not the object itself.

> >> Will it matter to mention the size of the data structure "afs_sysnames"?
> >
> > Why is it necessary to do so?
> 
> I suggest to express the impact of the missed function call "kfree".

I would hope that anyone reading the patch could work the impact out for
themselves.  Just specifying the size of a struct isn't all that useful - it
may be wildly variable by arch (eg. 32/64) and config option (eg. lockdep)
anyway.  Add to that rounding and packing details from the memory subsys,
along with the pinning effect of something you can't get rid of.

Of more use would be specifying the frequency or likelyhood of such a leak but
unless it's especially high, it's probably not worth mentioning.

David

Re: [v2] afs: Fix memory leak in afs_put_sysnames()

[Markus Elfring]

>> * Is freeing and releasing an item a duplicate operation anyhow?
>
> You're missing the point.  afs_put_sysnames() does release the things the
> object points to (ie. the content),

It is possible to distinguish the release of system resources for further items.


> but not the object itself.

How does this view fit to the proposed addition "kfree(sysnames);"?
https://lore.kernel.org/linux-fsdevel/20200602013045.321855-1-chengzhihao1@huawei.com/
https://lore.kernel.org/patchwork/patch/1251323/

Regards,
Markus