Skip to contentSkip to conversation

damon/sysfs: Fix possible memleak on damon_sysfs_add_target.

PATCH Superseded Levi Yun 2 participants 2 messages Sep 25, 2022

Subsystems (2)
Memory management DAMON

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Levi Yun

author
#0 lore raw

PATCH 1/1 damon/sysfs: Fix possible memleak on damon_sysfs_add_target.

When damon_sysfs_add_target couldn't find proper task, New allocated damon_target structure isn't registered yet, So, it's impossible to free new allocated one by damon_sysfs_destroy_targets.

By calling additional damon_free_target when find_get_pid function, Fix possible memory leak.

Signed-off-by Levi Yun <[email protected]>
---
 mm/damon/sysfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
mm/damon/sysfs.c +4 -1
index 7488e27c87c3..28ed07d26d55 100644
@@ -2184,8 +2184,11 @@ static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target,
 		return -ENOMEM;
 	if (damon_target_has_pid(ctx)) {
 		t->pid = find_get_pid(sys_target->pid);
-		if (!t->pid)
+		if (!t->pid) {
+			damon_free_target(t);
+
 			goto destroy_targets_out;
+		}
 	}
 	damon_add_target(ctx, t);
 	err = damon_sysfs_set_regions(t, sys_target->regions);
Signature · 1 line
2.35.1
maintainer
#1 lore raw
Quoted 4 lines · On Sun, 25 Sep 2022 23:02:57 +0900 Levi Yun <[email protected]> wrote:

When damon_sysfs_add_target couldn't find proper task, New allocated damon_target structure isn't registered yet, So, it's impossible to free new allocated one by damon_sysfs_destroy_targets.

Good finding, thanks!

Quoted 5 lines

By calling additional damon_free_target when find_get_pid function, Fix possible memory leak.

Signed-off-by Levi Yun <[email protected]>

Could we add relevant 'Fixes: ' and 'Cc: <[email protected]>' tags?

Quoted 9 lines
---
 mm/damon/sysfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
mm/damon/sysfs.c +0 -0
index 7488e27c87c3..28ed07d26d55 100644

mm/damon/sysfs.c static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target, · line 2189

Quoted 7 lines
mm/damon/sysfs.c +3 -1
@@ -2184,8 +2184,11 @@ static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target,
 		return -ENOMEM;
 	if (damon_target_has_pid(ctx)) {
 		t->pid = find_get_pid(sys_target->pid);
-		if (!t->pid)
+		if (!t->pid) {
+			damon_free_target(t);
+

Seems unnecessary new line?

mm/damon/sysfs.c static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target, · line 2192

Quoted 3 lines
mm/damon/sysfs.c +1 -0
 			goto destroy_targets_out;
+		}
 	}

Looks good to me, but... How about simply doing 'damon_add_target()' before 'if (damon_target_has_pid())', like below?

```
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 455215a5c059..9f1219a67e3f 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -2172,12 +2172,12 @@ static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target,

        if (!t)
                return -ENOMEM;
+       damon_add_target(ctx, t);
        if (damon_target_has_pid(ctx)) {
                t->pid = find_get_pid(sys_target->pid);
                if (!t->pid)
                        goto destroy_targets_out;
        }
-       damon_add_target(ctx, t);
        err = damon_sysfs_set_regions(t, sys_target->regions);
        if (err)
                goto destroy_targets_out;
```

Thanks,
SJ

Quoted 6 lines
mm/damon/sysfs.c +0 -0
 	damon_add_target(ctx, t);
 	err = damon_sysfs_set_regions(t, sys_target->regions);
Signature · 1 line
2.35.1