mm/damon/stat: fix memory leak on damon_start() failure in damon_stat_start()

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 386810f9b0a1ae341b87a984b16b2a2e1bafab71
Author: Jackie Liu <[email protected]>

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.

Quoted 6 lines

mm/damon/stat.c +0 -0

@@ -249,8 +249,11 @@ static int damon_stat_start(void)
 	if (!damon_stat_context)
 		return -ENOMEM;

mm/damon/stat.c static int damon_stat_start(void) · line 259

Quoted 10 lines

mm/damon/stat.c +4 -1

 	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 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.

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/[email protected]
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail

From: Jackie Liu <[email protected]>

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")

Signed-off-by Jackie Liu <[email protected]>