* Re: (sashiko review) [PATCH] mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start()
2026-03-31 10:15 [PATCH] mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start() Jackie Liu
@ 2026-04-01 1:24 ` SeongJae Park
2026-04-01 1:32 ` SeongJae Park
1 sibling, 0 replies; 3+ messages in thread
From: SeongJae Park @ 2026-04-01 1:24 UTC (permalink / raw)
To: Jackie Liu; +Cc: SeongJae Park, akpm, linux-mm, damon
Forwarding Sashiko review for doing discussions via mails, in form of reply,
and adding my comments inline.
TL; DR: No blocker for this patch.
The first one is nice finding but orthogonal to this patch (I start feeling
sashiko might be useful at finding potential bugs in existing code, like static
code analysis tools, more than reviewing patches).
The second one is just wrong.
> # review url: https://sashiko.dev/#/patchset/20260331101553.88422-1-liu.yun@linux.dev
> # start of sashiko.dev inline review
> commit 386810f9b0a1ae341b87a984b16b2a2e1bafab71
> Author: Jackie Liu <liuyun01@kylinos.cn>
>
> mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start()
>
> This commit fixes a memory leak by destroying the DAMON context and resetting
> the global pointer when damon_start() fails. This ensures the previously
> allocated context is not permanently lost on subsequent enable attempts.
>
> > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > --- a/mm/damon/stat.c
> > +++ b/mm/damon/stat.c
> > @@ -249,8 +249,11 @@ static int damon_stat_start(void)
> > if (!damon_stat_context)
> > return -ENOMEM;
> > err = damon_start(&damon_stat_context, 1, true);
> > - if (err)
> > + if (err) {
> > + damon_destroy_ctx(damon_stat_context);
> > + damon_stat_context = NULL;
> > return err;
> > + }
> >
> > damon_stat_last_refresh_jiffies = jiffies;
> > call_control.data = damon_stat_context;
>
> Does this code still leak the context if the subsequent call to damon_call()
> fails?
>
> If the newly spawned kdamond thread exits before the call gets processed (such
> as due to an internal allocation failure for regions_score_histogram in
> kdamond_fn()), damon_call() can return an error like -EINVAL or -ECANCELED.
>
> When damon_stat_start() returns this error to its caller, the caller resets
> enabled = false and propagates the error, skipping damon_stat_stop().
>
> Consequently, damon_stat_context is never destroyed, nor is the global
> pointer reset to NULL. The next time the module is enabled,
> damon_stat_start() overwrites damon_stat_context with a new context,
> permanently leaking the previously allocated struct damon_ctx.
>
> To completely fix the memory leak, should the return value of damon_call() be
> checked and cleaned up similarly to the damon_start() error path?
This is correct, but orthogonal to this patch. I will work on this.
>
> This isn't a bug introduced by this patch, but is there a race condition
> between damon_stat_init() and the sysfs module parameter setter
> damon_stat_enabled_store()?
>
> For loadable modules, sysfs parameter files are exposed to userspace before
> the module's init function completes. If the module is loaded with the
> parameter enabled=1, damon_stat_init() will begin executing
> damon_stat_start(), which allocates damon_stat_context.
This is not correct. damon_initialized() check in the middle is there for
avoiding this.
Thanks,
SJ
>
> Concurrently, if a userspace process writes 0 to the sysfs enabled
> parameter, this triggers damon_stat_enabled_store(), which sets
> enabled = false and immediately calls damon_stat_stop().
>
> Because damon_stat_init() does not hold the kernel_param_lock, there is
> no mutual exclusion between these execution paths. damon_stat_stop()
> calls damon_destroy_ctx(), which frees the newly allocated
> damon_stat_context.
>
> The damon_stat_start() function (still executing in the init thread)
> then proceeds to call damon_start(&damon_stat_context, 1, true),
> dereferencing the freed context pointer and causing a use-after-free.
>
>
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260331101553.88422-1-liu.yun@linux.dev
> #
> # hkml [1] generated a draft of this mail. It can be regenerated
> # using below command:
> #
> # hkml patch sashiko_dev --for_forwarding \
> # 20260331101553.88422-1-liu.yun@linux.dev
> #
> # [1] https://github.com/sjp38/hackermail
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start()
2026-03-31 10:15 [PATCH] mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start() Jackie Liu
2026-04-01 1:24 ` (sashiko review) " SeongJae Park
@ 2026-04-01 1:32 ` SeongJae Park
1 sibling, 0 replies; 3+ messages in thread
From: SeongJae Park @ 2026-04-01 1:32 UTC (permalink / raw)
To: Jackie Liu; +Cc: SeongJae Park, akpm, linux-mm, damon, stable
+ damon@lists.linux.dev. Please Cc damon@ for all DAMON patches.
Hello Jackie,
On Tue, 31 Mar 2026 18:15:53 +0800 Jackie Liu <liu.yun@linux.dev> wrote:
> From: Jackie Liu <liuyun01@kylinos.cn>
>
> Destroy the DAMON context and reset the global pointer when
> damon_start() fails. Otherwise, the context allocated by
> damon_stat_build_ctx() is leaked, and the stale damon_stat_context
> pointer will be overwritten on the next enable attempt, making the
> old allocation permanently unreachable.
>
> Fixes: 369c415e6073 ("mm/damon: introduce DAMON_STAT module")
As this is a memory leak, let's add Cc: stable@ too.
Cc: <stable@vger.kernel.org> # 6.17.x
> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
Reviewed-by: SeongJae Park <sj@kernel.org>
Thanks,
SJ
[...]
^ permalink raw reply [flat|nested] 3+ messages in thread