On Sun, 25 Sep 2022 23:02:57 +0900 Levi Yun <ppbuk5246@xxxxxxxxx> 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!
>
> By calling additional damon_free_target when find_get_pid function,
> Fix possible memory leak.
>
> Signed-off-by: Levi Yun <ppbuk5246@xxxxxxxxx>
Could we add relevant 'Fixes: ' and 'Cc: <stable@xxxxxxxxxxxxxxx>' tags?
> ---
> mm/damon/sysfs.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 7488e27c87c3..28ed07d26d55 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -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?
> 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
> damon_add_target(ctx, t);
> err = damon_sysfs_set_regions(t, sys_target->regions);
> --
> 2.35.1
>
>
