mm/damon/stat: detect and use fresh enabled status

Sashiko AI review found 1 potential issue(s):
- [Medium] Writing 'N' to the enabled parameter after kdamond has stopped unexpectedly bypasses damon_stat_stop(), leaking damon_stat_context.

--

commit 1171ea4d01e310ec9987bf3cb2d19f8bf89b60cb
Author: SeongJae Park <[email protected]>

mm/damon/stat: detect and use fresh enabled status

This commit changes the enabled parameter's get callback to report the actual running status of kdamond instead of the static enabled variable. It introduces damon_stat_enabled() to dynamically check if the context is running.

Quoted 4 lines

mm/damon/stat.c +0 -0

index 99ba346f9e325..3951b762cbddf 100644

[ ... ]

Quoted 14 lines

mm/damon/stat.c +7 -0

@@ -273,17 +276,23 @@ static void damon_stat_stop(void)
 	damon_stat_context = NULL;
 }
 
+static bool damon_stat_enabled(void)
+{
+	if (!damon_stat_context)
+		return false;
+	return damon_is_running(damon_stat_context);
+}
+
 static int damon_stat_enabled_store(
 		const char *val, const struct kernel_param *kp)
 {

mm/damon/stat.c static void damon_stat_stop(void) · line 296

Quoted 10 lines

mm/damon/stat.c +1 -2

-	bool is_enabled = enabled;
 	int err;
 
 	err = kstrtobool(val, &enabled);
 	if (err)
 		return err;
 
-	if (is_enabled = enabled)
+	if (damon_stat_enabled() = enabled)
 		return 0;

If kdamond stops unexpectedly (for example, due to an allocation failure in kdamond_fn), damon_is_running() and consequently damon_stat_enabled() will return false.

If a user then explicitly disables the module by writing 'N' to the enabled parameter, kstrtobool() will update the global enabled variable to false.

Could this cause the check above to evaluate to true (false = false) and return 0 early?

Quoted 7 lines

mm/damon/stat.c +0 -0

 	if (!damon_initialized())

@@ -293,16 +302,17 @@ static int damon_stat_enabled_store(
 		 */
 		return 0;
 

mm/damon/stat.c static int damon_stat_enabled_store( · line 307

Quoted 9 lines

mm/damon/stat.c +2 -6

-	if (enabled) {
-		err = damon_stat_start();
-		if (err)
-			enabled = false;
-		return err;
-	}
+	if (enabled)
+		return damon_stat_start();
 	damon_stat_stop();

If the early return is taken, damon_stat_stop() is bypassed. Does this leak damon_stat_context since damon_destroy_ctx() won't be called?

Quoted 3 lines

mm/damon/stat.c +0 -0

 	return 0;
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

DAMON_STAT assumes the kdamond will keep running once damon_stat_start() succeeds, until it calls damon_stop() to stop it. If the regions_score_histogram allocation in kdamond_fn() is tried after damon_stat_start() returns, however, and if the allocation fails, the kdamond can stop before DAMON_STAT calls damon_stop(). In this case, users will show the 'enabled' parameter value as 'true', while it is not working. This could make users confused.

The user impact should be mild, though. First of all, the issue may happen only quite rarely. The allocation failure is arguably too small to fail (100 unsigned long objects) in common setups. The time window for the race is also quite small. Even if the race and the allocation failure happen, users could find the fact that the kdamond is stopped using 'ps' like commands. By writing 'N' and 'Y' to the 'enabled' parameter sequentially, the user can also easily restart DAMON_STAT.

That said, the bug is a bug that needs to be fixed. Instead of managing the complicated state in the variable, detect and use the real kdamond running status when the user reads the parameter, via the parameter read callback. This will allow users to always read the correct 'enabled' value.

Note that the 'enabled' variable is no longer the argument for the 'enabled' parameter. But it is still used for two use case. For keeping the config/boot time user-set parameter value. And for keeping the user request to compare against the current state, to see if the damon_start() or damon_stop() call are really needed.