* [PATCH] afs: Fix memory leak in afs_put_sysnames()
@ 2020-06-01 9:21 Zhihao Cheng
2020-06-01 12:31 ` David Howells
0 siblings, 1 reply; 4+ messages in thread
From: Zhihao Cheng @ 2020-06-01 9:21 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(). Besides, it would be better set net->sysnames
to 'NULL' after net->sysnames being released if afs_put_sysnames()
aims on an afs_sysnames object.
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
Cc: <Stable@vger.kernel.org> # v4.17+
Fixes: 6f8880d8e681557 ("afs: Implement @sys substitution handling")
---
fs/afs/dir.c | 2 +-
fs/afs/internal.h | 2 ++
fs/afs/main.c | 4 ++--
fs/afs/proc.c | 25 ++++++++++++++++++++-----
4 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index d1e1caa23c8b..cb9d8aa91048 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -894,7 +894,7 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry,
*/
ret = NULL;
out_s:
- afs_put_sysnames(subs);
+ afs_put_sysnames_and_null(net);
kfree(buf);
out_p:
key_put(key);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 80255513e230..615dd5f9ad6f 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1093,12 +1093,14 @@ extern void __net_exit afs_proc_cleanup(struct afs_net *);
extern int afs_proc_cell_setup(struct afs_cell *);
extern void afs_proc_cell_remove(struct afs_cell *);
extern void afs_put_sysnames(struct afs_sysnames *);
+extern void afs_put_sysnames_and_null(struct afs_net *);
#else
static inline int afs_proc_init(struct afs_net *net) { return 0; }
static inline void afs_proc_cleanup(struct afs_net *net) {}
static inline int afs_proc_cell_setup(struct afs_cell *cell) { return 0; }
static inline void afs_proc_cell_remove(struct afs_cell *cell) {}
static inline void afs_put_sysnames(struct afs_sysnames *sysnames) {}
+static inline void afs_put_sysnames_and_null(struct afs_net *net) {}
#endif
/*
diff --git a/fs/afs/main.c b/fs/afs/main.c
index c9c45d7078bd..6bf73fc65fb5 100644
--- a/fs/afs/main.c
+++ b/fs/afs/main.c
@@ -132,7 +132,7 @@ static int __net_init afs_net_init(struct net *net_ns)
net->live = false;
afs_proc_cleanup(net);
error_proc:
- afs_put_sysnames(net->sysnames);
+ afs_put_sysnames_and_null(net);
error_sysnames:
net->live = false;
return ret;
@@ -150,7 +150,7 @@ static void __net_exit afs_net_exit(struct net *net_ns)
afs_purge_servers(net);
afs_close_socket(net);
afs_proc_cleanup(net);
- afs_put_sysnames(net->sysnames);
+ afs_put_sysnames_and_null(net);
}
static struct pernet_operations afs_net_ops = {
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 468e1713bce1..26e1e73281a6 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -554,15 +554,30 @@ static int afs_proc_sysname_write(struct file *file, char *buf, size_t size)
goto out;
}
-void afs_put_sysnames(struct afs_sysnames *sysnames)
+static void afs_free_sysnames(struct afs_sysnames *sysnames)
{
int i;
+ for (i = 0; i < sysnames->nr; i++)
+ if (sysnames->subs[i] != afs_init_sysname &&
+ sysnames->subs[i] != sysnames->blank)
+ kfree(sysnames->subs[i]);
+ kfree(sysnames);
+}
+
+void afs_put_sysnames(struct afs_sysnames *sysnames)
+{
+ if (sysnames && refcount_dec_and_test(&sysnames->usage))
+ afs_free_sysnames(sysnames);
+}
+
+void afs_put_sysnames_and_null(struct afs_net *net)
+{
+ struct afs_sysnames *sysnames = net->sysnames;
+
if (sysnames && refcount_dec_and_test(&sysnames->usage)) {
- for (i = 0; i < sysnames->nr; i++)
- if (sysnames->subs[i] != afs_init_sysname &&
- sysnames->subs[i] != sysnames->blank)
- kfree(sysnames->subs[i]);
+ afs_free_sysnames(sysnames);
+ net->sysnames = NULL;
}
}
--
2.25.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] afs: Fix memory leak in afs_put_sysnames()
2020-06-01 9:21 [PATCH] afs: Fix memory leak in afs_put_sysnames() Zhihao Cheng
@ 2020-06-01 12:31 ` David Howells
0 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2020-06-01 12:31 UTC (permalink / raw)
To: Zhihao Cheng; +Cc: dhowells, linux-afs, linux-fsdevel, linux-kernel, yi.zhang
Zhihao Cheng <chengzhihao1@huawei.com> wrote:
> sysnames should be freed after refcnt being decreased to zero in
> afs_put_sysnames().
Good catch.
> Besides, it would be better set net->sysnames to 'NULL' after net->sysnames
> being released if afs_put_sysnames() aims on an afs_sysnames object.
Why? We don't normally clear pointers when cleaning up a struct - and of the
two places this is relevant, in one we fail to set up a namespace and in the
other we're tearing down a namespace. In neither case should the pointer be
accessed again.
> @@ -894,7 +894,7 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry,
> */
> ret = NULL;
> out_s:
> - afs_put_sysnames(subs);
> + afs_put_sysnames_and_null(net);
This is definitely wrong. We obtained a ref 23 lines above and dropped the
lock:
read_lock(&net->sysnames_lock);
subs = net->sysnames;
refcount_inc(&subs->usage);
read_unlock(&net->sysnames_lock);
We are dropping *that* ref, not the one in the struct.
Just adding the missing kfree() call into afs_put_sysnames() should suffice,
thanks.
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] afs: Fix memory leak in afs_put_sysnames()
2020-06-01 11:18 Markus Elfring
@ 2020-06-01 12:35 ` David Howells
0 siblings, 0 replies; 4+ messages in thread
From: David Howells @ 2020-06-01 12:35 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().
>
> * I suggest to use the wording “reference counter”.
Can you use ASCII quotes please? Not all fonts contain these quotes, and
occasionally they got copied into commit messages.
> * Where did you notice a “memory leak” here?
He said "sysnames should be freed after refcnt being decreased to zero in
afs_put_sysnames()".
> * Will the tag “Fixes” become relevant for the commit message?
It is the correct tag.
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] afs: Fix memory leak in afs_put_sysnames()
@ 2020-06-01 11:18 Markus Elfring
2020-06-01 12:35 ` David Howells
0 siblings, 1 reply; 4+ messages in thread
From: Markus Elfring @ 2020-06-01 11:18 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().
* I suggest to use the wording “reference counter”.
* Where did you notice a “memory leak” here?
> Besides, it would be better set net->sysnames
> to 'NULL' after net->sysnames being released if afs_put_sysnames()
> aims on an afs_sysnames object.
* Would you like to consider an adjustment for this information?
* How do you think about to add an imperative wording?
* Will the tag “Fixes” become relevant for the commit message?
Regards,
Markus
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-01 12:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-01 9:21 [PATCH] afs: Fix memory leak in afs_put_sysnames() Zhihao Cheng
2020-06-01 12:31 ` David Howells
2020-06-01 11:18 Markus Elfring
2020-06-01 12:35 ` David Howells
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).