* Re: [PATCH v2] afs: Fix memory leak in afs_put_sysnames()
@ 2020-06-01 14:40 Markus Elfring
2020-06-01 17:08 ` David Howells
0 siblings, 1 reply; 6+ messages in thread
From: Markus Elfring @ 2020-06-01 14:40 UTC (permalink / raw)
To: Zhihao Cheng, linux-afs, linux-fsdevel
Cc: linux-kernel, David Howells, Yi Zhang
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] afs: Fix memory leak in afs_put_sysnames()
2020-06-01 14:40 [PATCH v2] afs: Fix memory leak in afs_put_sysnames() Markus Elfring
@ 2020-06-01 17:08 ` David Howells
2020-06-01 18:04 ` [v2] " Markus Elfring
2020-06-01 18:52 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: David Howells @ 2020-06-01 17:08 UTC (permalink / raw)
To: Markus Elfring
Cc: dhowells, Zhihao Cheng, linux-afs, linux-fsdevel, linux-kernel, Yi Zhang
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2] afs: Fix memory leak in afs_put_sysnames()
2020-06-01 17:08 ` David Howells
@ 2020-06-01 18:04 ` Markus Elfring
2020-06-01 18:52 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: Markus Elfring @ 2020-06-01 18:04 UTC (permalink / raw)
To: David Howells, linux-afs, linux-fsdevel
Cc: Zhihao Cheng, Yi Zhang, linux-kernel
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v2] afs: Fix memory leak in afs_put_sysnames()
2020-06-01 17:08 ` David Howells
2020-06-01 18:04 ` [v2] " Markus Elfring
@ 2020-06-01 18:52 ` David Howells
2020-06-02 5:51 ` Markus Elfring
1 sibling, 1 reply; 6+ messages in thread
From: David Howells @ 2020-06-01 18:52 UTC (permalink / raw)
To: Markus Elfring
Cc: dhowells, linux-afs, linux-fsdevel, Zhihao Cheng, Yi Zhang, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] afs: Fix memory leak in afs_put_sysnames()
@ 2020-06-01 13:27 Zhihao Cheng
0 siblings, 0 replies; 6+ messages in thread
From: Zhihao Cheng @ 2020-06-01 13:27 UTC (permalink / raw)
To: linux-afs, linux-fsdevel, linux-kernel; +Cc: dhowells, yi.zhang
sysnames should be freed after refcnt being decreased to zero in
afs_put_sysnames().
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Cc: <Stable@vger.kernel.org> # v4.17+
Fixes: 6f8880d8e681557 ("afs: Implement @sys substitution handling")
---
fs/afs/proc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 468e1713bce1..6f34c84a0fd0 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -563,6 +563,7 @@ void afs_put_sysnames(struct afs_sysnames *sysnames)
if (sysnames->subs[i] != afs_init_sysname &&
sysnames->subs[i] != sysnames->blank)
kfree(sysnames->subs[i]);
+ kfree(sysnames);
}
}
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-02 5:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-01 14:40 [PATCH v2] afs: Fix memory leak in afs_put_sysnames() Markus Elfring
2020-06-01 17:08 ` David Howells
2020-06-01 18:04 ` [v2] " Markus Elfring
2020-06-01 18:52 ` David Howells
2020-06-02 5:51 ` Markus Elfring
-- strict thread matches above, loose matches on Subject: below --
2020-06-01 13:27 [PATCH v2] " Zhihao Cheng
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).