Re: [PATCH] damon/sysfs: Fix possible memleak on damon_sysfs_add_target.

[SeongJae Park <sj@kernel.org>]

On Sun, 25 Sep 2022 23:02:57 +0900 Levi Yun <ppbuk5246@gmail.com> 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@gmail.com>

Could we add relevant 'Fixes: ' and 'Cc: <stable@vger.kernel.org>' 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
> 
>