afs: Fix memory leak in afs_put_sysnames()

Commit Message

Zhihao Cheng June 1, 2020, 9:21 a.m. UTC

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 <[e-mail]>
Cc: <[e-mail]> # 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(-)

Comments

David Howells June 1, 2020, 12:31 p.m. UTC | #1

Zhihao Cheng <[e-mail]> 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

Patch

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;
 	}
 }