[PATCH 6.12.y] mm/damon/core: always put unsuccessfully committed target pids

[PATCH 6.12.y] mm/damon/core: always put unsuccessfully committed target pids

[SJ Park]

From: SeongJae Park <sj@kernel.org>

damon_commit_target() puts and gets the destination and the source target
pids.  It puts the destination target pid because it will be overwritten
by the source target pid.  It gets the source pid because the caller is
supposed to eventually put the pids.  In more detail, the caller will call
damon_destroy_ctx() after damon_commit_ctx() to destroy the entire source
context.  And in this case, [f]vaddr operation set's cleanup_target()
callback will put the pids.

The commit operation is made at the context level.  The operation can fail
in multiple places including in the middle and after the targets commit
operations.  For any such failures, immediately the error is returned to
the damon_commit_ctx() caller.  If some or all of the source target pids
were committed to the destination during the unsuccessful context commit
attempt, those pids should be put twice.

The source context will do the put operations using the above explained
routine.  However, let's suppose the destination context was not
originally using [f]vaddr operation set and the commit failed before the
ops of the source context is committed.  The destination does not have the
cleanup_target() ops callback, so it cannot put the pids via the
damon_destroy_ctx().

As a result, the pids are leaked.  The issue in the real world would be
not very common.  The commit feature is for changing parameters of running
DAMON context while inheriting internal status like the monitoring
results.  The monitoring results of a physical address range ain't have
things that are beneficial to be inherited to a virtual address ranges
monitoring.  So the problem-causing DAMON control would be not very common
in the real world.  That said, it is a supported feature.  And
damon_commit_target() failure due to memory allocation is relatively
realistic [1] if there are a huge number of target regions.

Fix by putting the pids in the commit operation in case of the failures.

The issue was discovered [2] by Sashiko.

Link: https://lore.kernel.org/20260605013849.83750-1-sj@kernel.org
Link: https://lore.kernel.org/20260603112306.58490-1-akinobu.mita@gmail.com [1]
Link: https://lore.kernel.org/20260320020056.835-1-sj@kernel.org [2]
Fixes: 83dc7bbaecae ("mm/damon/sysfs: use damon_commit_ctx()")
Signed-off-by: SeongJae Park <sj@kernel.org>
Cc: <stable@vger.kernel.org> # 6.11.x
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
(cherry picked from commit 6a66c557a2ab2609575bafd15e093669c05f9711)
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 2a6da49816526..416ef750ad140 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -938,10 +938,36 @@ static int damon_commit_target(
 	return 0;
 }
 
+/*
+ * damon_revert_target_commits() - revert unsuccessful target commits.
+ * @dst:	Commit destination context
+ * @failed:	Commit failed destination target
+ * @src:	Commit source context
+ *
+ * Revert target states that changed by damon_commit_target(), and cannot be
+ * cleaned up by the destination context's ops.cleanup_target().
+ */
+static void damon_revert_target_commits(struct damon_ctx *dst,
+		struct damon_target *failed, struct damon_ctx *src)
+{
+	struct damon_target *target;
+
+	if (!damon_target_has_pid(src))
+		return;
+	if (damon_target_has_pid(dst))
+		return;
+	damon_for_each_target(target, dst) {
+		if (target == failed)
+			return;
+		put_pid(target->pid);
+	}
+}
+
 static int damon_commit_targets(
 		struct damon_ctx *dst, struct damon_ctx *src)
 {
 	struct damon_target *dst_target, *next, *src_target, *new_target;
+	struct damon_target *failed;
 	int i = 0, j = 0, err;
 
 	damon_for_each_target_safe(dst_target, next, dst) {
@@ -950,8 +976,10 @@ static int damon_commit_targets(
 			err = damon_commit_target(
 					dst_target, damon_target_has_pid(dst),
 					src_target, damon_target_has_pid(src));
-			if (err)
-				return err;
+			if (err) {
+				failed = dst_target;
+				goto out;
+			}
 		} else {
 			if (damon_target_has_pid(dst))
 				put_pid(dst_target->pid);
@@ -959,21 +987,28 @@ static int damon_commit_targets(
 		}
 	}
 
+	failed = NULL;
 	damon_for_each_target_safe(src_target, next, src) {
 		if (j++ < i)
 			continue;
 		new_target = damon_new_target();
-		if (!new_target)
-			return -ENOMEM;
+		if (!new_target) {
+			err = -ENOMEM;
+			goto out;
+		}
 		err = damon_commit_target(new_target, false,
 				src_target, damon_target_has_pid(src));
 		if (err) {
 			damon_destroy_target(new_target);
-			return err;
+			goto out;
 		}
 		damon_add_target(dst, new_target);
 	}
 	return 0;
+
+out:
+	damon_revert_target_commits(dst, failed, src);
+	return err;
 }
 
 /**
@@ -1007,8 +1042,10 @@ int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
 	 *    committing require putting pids).
 	 */
 	err = damon_set_attrs(dst, &src->attrs);
-	if (err)
+	if (err) {
+		damon_revert_target_commits(dst, NULL, src);
 		return err;
+	}
 	dst->ops = src->ops;
 
 	return 0;
-- 
2.47.3

Re: [PATCH 6.12.y] mm/damon/core: always put unsuccessfully committed target pids

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] PID references are leaked when damon_start() or late stages of context building fail in damon_sysfs_turn_damon_on().
--

--- Patch [1]: [PATCH 6.12.y] mm/damon/core: always put unsuccessfully committed target pids ---
commit d053b994cdbf8e3fe384c68bbdb8f593cb5a45e5
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: always put unsuccessfully committed target pids

This commit fixes a PID leak by explicitly putting the PIDs of targets in
the commit operation when damon_commit_target() fails.

Link: https://lore.kernel.org/20260605013849.83750-1-sj@kernel.org
Link: https://lore.kernel.org/20260603112306.58490-1-akinobu.mita@gmail.com [1]
Link: https://lore.kernel.org/20260320020056.835-1-sj@kernel.org [2]

> The source context will do the put operations using the above explained
> routine.  However, let's suppose the destination context was not
> originally using [f]vaddr operation set and the commit failed before the
> ops of the source context is committed.  The destination does not have the
> cleanup_target() ops callback, so it cannot put the pids via the
> damon_destroy_ctx().
> 
> As a result, the pids are leaked.

[Severity: Medium]
This is a pre-existing issue, but does a similar PID leak happen in
mm/damon/sysfs.c when context building or starting fails?

For example, in damon_sysfs_build_ctx():

mm/damon/sysfs.c:damon_sysfs_build_ctx() {
	...
	err = damon_sysfs_apply_inputs(ctx, sys_ctx);
	if (err) {
		damon_destroy_ctx(ctx);
		return ERR_PTR(err);
	}
	...
}

If damon_sysfs_apply_inputs() fails during scheme addition (after targets
were successfully added and PIDs referenced), damon_destroy_ctx(ctx) is
called and the PIDs are leaked.

Similarly, in damon_sysfs_turn_damon_on():

mm/damon/sysfs.c:damon_sysfs_turn_damon_on() {
	...
	err = damon_start(&ctx, 1, false);
	if (err) {
		damon_destroy_ctx(ctx);
		return err;
	}
	...
}

If damon_start() fails (e.g., due to ENOMEM), the error path directly calls
damon_destroy_ctx(ctx). For vaddr operations, ctx->ops.cleanup is NULL,
and PID references are normally released by damon_sysfs_before_terminate().

However, since the thread is never started or the context is aborted early,
the termination callback is not called and the PIDs are never put.

Is this something that needs a similar fix to ensure these PIDs are
released?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721052238.163760-1-sj@kernel.org?part=1