Skip to contentSkip to conversation

mm/damon/stat: deallocate damon_call() failure leaking damon_ctx

PATCH Applied SeongJae Park 1 participant 2 messages Apr 2

Subsystems (3)
Stable branch Memory management DAMON
lore
Share & tools
lore

SeongJae Park

author maintainer
#0 lore raw

PATCH v2 1/1 mm/damon/stat: deallocate damon_call() failure leaking damon_ctx

damon_stat_start() always allocates the module's damon_ctx object (damon_stat_context). Meanwhile, if damon_call() in the function fails, the damon_ctx object is not deallocated. Hence, if the damon_call() is failed, and the user writes Y to “enabled” again, the previously allocated damon_ctx object is leaked.

This cannot simply be fixed by deallocating the damon_ctx object when damon_call() fails. That's because damon_call() failure doesn't guarantee the kdamond main function, which accesses the damon_ctx object, is completely finished. In other words, if damon_stat_start() deallocates the damon_ctx object after damon_call() failure, the not-yet-terminated kdamond could access the freed memory (use-after-free).

Fix the leak while avoiding the use-after-free by keeping returning damon_stat_start() without deallocating the damon_ctx object after damon_call() failure, but deallocating it when the function is invoked again and the kdamond is completely terminated. If the kdamond is not yet terminated, simply return -EAGAIN, as the kdamond will soon be terminated.

The issue was discovered [1] by sashiko.

[1] https://lore.kernel.org/[email protected]

Fixes 405f61996d9d ("mm/damon/stat: use damon_call() repeat mode instead of damon_callback")
Cc <[email protected]> # 6.17.x
Signed-off-by SeongJae Park <[email protected]>

---
Changes from RFC
(https://lore.kernel.org/[email protected])
- Drop RFC tag again.
- sashiko didn't find any real issue on this version. Changes from v1
(https://lore.kernel.org/[email protected])
- Avoid sashiko-discovered use-after-free.
- Add RFC tag.

 mm/damon/stat.c | 7 +++++++
 1 file changed, 7 insertions(+)
mm/damon/stat.c +7 -0
index 5a742fc157e4..99ba346f9e32 100644
@@ -245,6 +245,12 @@ static int damon_stat_start(void)
 {
 	int err;
 
+	if (damon_stat_context) {
+		if (damon_is_running(damon_stat_context))
+			return -EAGAIN;
+		damon_destroy_ctx(damon_stat_context);
+	}
+
 	damon_stat_context = damon_stat_build_ctx();
 	if (!damon_stat_context)
 		return -ENOMEM;

@@ -264,6 +270,7 @@ static void damon_stat_stop(void)
 {
 	damon_stop(&damon_stat_context, 1);
 	damon_destroy_ctx(damon_stat_context);
+	damon_stat_context = NULL;
 }
 
 static int damon_stat_enabled_store(

base-commit: 4fd04f750d79667937931314ed64c9d79b0d82ef

Signature · 1 line
2.47.3
author maintainer
#1 lore raw

Re: (sashiko review) [PATCH v2] mm/damon/stat: deallocate damon_call() failure leaking damon_ctx

- stable@, as it seems this is not really need to be shared there.

TL; DR: sashiko didn't find any real issue.

Forwarding Sashiko review in a reply form with my inline comments, for doing detailed discussions via mails if needed.

Quoted 45 lines
# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit f956e734cbf12d26893e0ce6fe09667b1ba0ca51
Author: SeongJae Park <[email protected]>

mm/damon/stat: deallocate damon_call() failure leaking damon_ctx

This commit attempts to fix a memory leak and avoid a use-after-free by deferring the deallocation of the damon_ctx object when damon_call() fails, cleaning it up on the next invocation instead.

Link https://lore.kernel.org/[email protected] open here
Quoted 5 lines
mm/damon/stat.c +0 -0
index 30aeb2c207ec3..b1011a7a2f291 100644

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

Quoted 10 lines
mm/damon/stat.c +6 -0
@@ -245,6 +245,12 @@ static int damon_stat_start(void)
 {
 	int err;
 
+	if (damon_stat_context) {
+		if (damon_is_running(damon_stat_context))
+			return -EAGAIN;
+		damon_destroy_ctx(damon_stat_context);
+	}
+
 	damon_stat_context = damon_stat_build_ctx();

[ ... ]

The commit message notes that deallocating damon_stat_context immediately after a damon_call() failure leads to a use-after-free if kdamond is still running.

However, it appears the immediate deallocation in the error path was left intact at the end of damon_stat_start():

	err = damon_call(damon_stat_context, &call_control);
	if (err) {
		damon_destroy_ctx(damon_stat_context);
		damon_stat_context = NULL;
	}
	return err;
Quoted 8 lines

Since damon_stat_context is still immediately destroyed and set to NULL when damon_call() fails, does this leave the use-after-free issue unresolved?

Additionally, because it is set to NULL, the new deferred cleanup logic added at the top of the function will evaluate to false on the next invocation. Should the immediate deallocation block be removed to allow the new cleanup logic to handle it?

Seems sashiko is saying so because sashiko applied the patch on one of mm.git trees that having the v1 of this patch. I asked Andrew to drop the v1, so sashiko's claim shouldn't be a real issue.

Quoted 4 lines

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Thanks,
SJ

[...]