Skip to contentSkip to conversation

mm/damon/dbgfs: fix memory leak when using debugfs_lookup()

PATCH In review Greg Kroah-Hartman 3 participants 3 messages Sep 2, 2022

Subsystems (3)
Stable branch Memory management DAMON
lore
Share & tools
lore

Greg Kroah-Hartman

author maintainer
#0 lore raw

PATCH 1/1 mm/damon/dbgfs: fix memory leak when using debugfs_lookup()

When calling debugfs_lookup() the result must have dput() called on it, otherwise the memory will leak over time. Fix this up by properly calling dput().

Cc SeongJae Park <[email protected]>
Cc Andrew Morton <[email protected]>
Cc [email protected]
Cc [email protected]
Cc stable <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>
---
 mm/damon/dbgfs.c | 1 +
 1 file changed, 1 insertion(+)
mm/damon/dbgfs.c +1 -0
index cb8a7e9926a4..dfb6ef8c83e0 100644
@@ -915,6 +915,7 @@ static int dbgfs_rm_context(char *name)
 		new_ctxs[j++] = dbgfs_ctxs[i];
 	}
 
+	dput(dir);
 	kfree(dbgfs_dirs);
 	kfree(dbgfs_ctxs);
Signature · 1 line
2.37.3
maintainer
#1 lore raw
Quoted 6 lines · On Fri, 2 Sep 2022 14:56:31 +0200 Greg Kroah-Hartman…

When calling debugfs_lookup() the result must have dput() called on it, otherwise the memory will leak over time. Fix this up by properly calling dput().

...

Fixes 75c1c2b53c78b, I assume.

mm/damon/dbgfs.c static int dbgfs_rm_context(char *name) · line 921

Quoted 10 lines
mm/damon/dbgfs.c +1 -0
@@ -915,6 +915,7 @@ static int dbgfs_rm_context(char *name)
 		new_ctxs[j++] = dbgfs_ctxs[i];
 	}
 
+	dput(dir);
 	kfree(dbgfs_dirs);
 	kfree(dbgfs_ctxs);

dput() is also needed if either of the kmalloc_array() calls fail? Maybe something like

mm/damon/dbgfs.c renamed from mm/damon/dbgfs.c~a +11 -6
@@ -884,6 +884,7 @@ static int dbgfs_rm_context(char *name)
 	struct dentry *root, *dir, **new_dirs;
 	struct damon_ctx **new_ctxs;
 	int i, j;
+	int ret = 0;
 
 	if (damon_nr_running_ctxs())
 		return -EBUSY;

@@ -899,14 +900,12 @@ static int dbgfs_rm_context(char *name)
 	new_dirs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_dirs),
 			GFP_KERNEL);
 	if (!new_dirs)
-		return -ENOMEM;
+		goto out_dput;
 
 	new_ctxs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_ctxs),
 			GFP_KERNEL);
-	if (!new_ctxs) {
-		kfree(new_dirs);
-		return -ENOMEM;
-	}
+	if (!new_ctxs)
+		goto out_new_dirs;
 
 	for (i = 0, j = 0; i < dbgfs_nr_ctxs; i++) {
 		if (dbgfs_dirs[i] == dir) {

@@ -925,7 +924,13 @@ static int dbgfs_rm_context(char *name)
 	dbgfs_ctxs = new_ctxs;
 	dbgfs_nr_ctxs--;
 
-	return 0;
+	goto out_dput;
+
+out_new_dirs:
+	kfree(new_dirs);
+out_dput:
+	dput(dir);
+	return ret;
 }
 
 static ssize_t dbgfs_rm_context_write(struct file *file,

_

maintainer
#2 lore raw

Hi Andrew and Greg!

Quoted 5 lines · On Fri, 2 Sep 2022 09:17:03 -0700 Andrew Morton <[email protected]>…
Quoted 3 lines · On Fri, 2 Sep 2022 14:56:31 +0200 Greg Kroah-Hartman…

When calling debugfs_lookup() the result must have dput() called on it, otherwise the memory will leak over time. Fix this up by properly calling dput().

Thank you for finding this bug and posting this patch, Greg!

Quoted 5 lines
Quoted 3 lines

...

Fixes 75c1c2b53c78b, I assume.

Correct, Andrew.

Fixes 75c1c2b53c78b ("mm/damon/dbgfs: support multiple contexts")
Cc <[email protected]> # 5.15.x
Quoted 7 lines
Quoted 7 lines
mm/damon/dbgfs.c +0 -0
@@ -915,6 +915,7 @@ static int dbgfs_rm_context(char *name)
 		new_ctxs[j++] = dbgfs_ctxs[i];
 	}
Quoted 7 lines

mm/damon/dbgfs.c static int dbgfs_rm_context(char *name) · line 935

Quoted 4 lines
mm/damon/dbgfs.c +1 -0
+	dput(dir);
 	kfree(dbgfs_dirs);
 	kfree(dbgfs_ctxs);

dput() is also needed if either of the kmalloc_array() calls fail? Maybe something like

Good catch.

Quoted 12 lines
mm/damon/dbgfs.c renamed from mm/damon/dbgfs.c~a +1 -0
@@ -884,6 +884,7 @@ static int dbgfs_rm_context(char *name)
 	struct dentry *root, *dir, **new_dirs;
 	struct damon_ctx **new_ctxs;
 	int i, j;
+	int ret = 0;
 
 	if (damon_nr_running_ctxs())
 		return -EBUSY;

mm/damon/dbgfs.c static int dbgfs_rm_context(char *name) · line 903

Quoted 5 lines
mm/damon/dbgfs.c renamed from mm/damon/dbgfs.c~a +1 -1
@@ -899,14 +900,12 @@ static int dbgfs_rm_context(char *name)
 	new_dirs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_dirs),
 			GFP_KERNEL);
 	if (!new_dirs)
-		return -ENOMEM;
+		goto out_dput;

Shouldn't we also do 'ret = -ENOMEM;' before 'godo out_dput'?

mm/damon/dbgfs.c static int dbgfs_rm_context(char *name) · line 908

Quoted 9 lines
mm/damon/dbgfs.c renamed from mm/damon/dbgfs.c~a +2 -4
 	new_ctxs = kmalloc_array(dbgfs_nr_ctxs - 1, sizeof(*dbgfs_ctxs),
 			GFP_KERNEL);
-	if (!new_ctxs) {
-		kfree(new_dirs);
-		return -ENOMEM;
-	}
+	if (!new_ctxs)
+		goto out_new_dirs;

ditto.

Quoted 18 lines
mm/damon/dbgfs.c renamed from mm/damon/dbgfs.c~a +7 -1
 	for (i = 0, j = 0; i < dbgfs_nr_ctxs; i++) {
 		if (dbgfs_dirs[i] == dir) {

@@ -925,7 +924,13 @@ static int dbgfs_rm_context(char *name)
 	dbgfs_ctxs = new_ctxs;
 	dbgfs_nr_ctxs--;
 
-	return 0;
+	goto out_dput;
+
+out_new_dirs:
+	kfree(new_dirs);
+out_dput:
+	dput(dir);
+	return ret;
 }
 
 static ssize_t dbgfs_rm_context_write(struct file *file,
Quoted 1 line

_

Thanks,
SJ