mm/damon: validate all parameters in the core

DAMON has a number of parameters. Some of the parameters are validated by DAMON, while some are validated by DAMON API callers. Each caller has their own set of parameters that are exposed to users. Hence each caller has their own ways to do the validation. There is no clear lines for the responsibilities. It is confusing and easy to make mistakes at validations. Actually we found a few bugs in the class.

Define DAMON core as the place to do all the validations and implement those. damon_set_regions(), damon_start() and damon_commit_ctx() are the three main DAMON core API functions for setup of DAMON parameters. Make the three functions to do safe and holistic parameter checks. The first one is already providing the validation, so changes are only for the last two functions.

This might add unnecessary validations for some use cases. The overhead should be negligible as parameters update is expected to only rarely happen. It reduces the number of places to check and fix for bugs of the class from all callers to the single component. The maintenance efficiency gain is obvious.

Further cleanup documents and caller code. Make the lines of validation duties clearer. Remove validations that are no more needed owing to the core validations.

Patches Sequence
================

Patch 1 implements the core holistic parameters validation for damon_commit_ctx(). Patch 2 extends the validation to damon_start(). Patch 3 removes the duplicated holistic parameters validation in DAMON sysfs interface, which is now embedded into the core layer. Patches 4 and 5 remove duplicated min_region_sz validation in DAMON modules. Patches 6 updates kernel doc to clarify damon_set_regions() is doing the range validation. Patches 7 and 8 remove monitoring target range validations that doesn't necessary thanks to the validation in damon_set_regions().

Note that the patch for providing the validation from damon_set_regions() is separately posted as a hotfix [1] but not yet picked into mm.git as of this writing.

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

Changes from RFC v1.1
- RFC v1.1: https://lore.kernel.org/[email protected]
- Drop RFC tag.
- Rebase to latest mm-new.
Changes from RFC v1
- RFC v1: https://lore.kernel.org/[email protected]
- Fix wrong dst testing of damon_commit_ctx().
- Use fresh test_ctx for each ctx in damon_start().

SJ Park (8):
mm/damon/core: safely validate src on damon_commit_ctx() mm/damon/core: do parameter testing commit on damon_start() mm/damon/sysfs: remove duplicated commit input validity check mm/damon/reclaim: remove duplicated min_region_sz power of 2 check mm/damon/lru_sort: remove duplicated min_region_sz power_of_2() check mm/damon: document region size validation in damon_set_regions() mm/damon/core: remove start, end check in
damon_set_region_system_rams()
mm/damon/sysfs: remove region size validation

 include/linux/damon.h |  3 +-
 mm/damon/core.c       | 78 ++++++++++++++++++++++++++++++++-----------
 mm/damon/lru_sort.c   |  5 ---
 mm/damon/reclaim.c    |  5 ---
 mm/damon/sysfs.c      | 35 +------------------
 5 files changed, 62 insertions(+), 64 deletions(-)

base-commit: 372482a3e74023a5c23b427ef5c6e7a057b46e6d

2.47.3

damon_commit_ctx() does its holistic parameter set validation while applying the new parameter in the set one by one. If it finds a parameter is invalid, because some invalid parameters may already be committed (it is called "commit" but not atomic and irreversable), it stops the running DAMON context.

The callers of the function therefore have to validate the parameters before calling it. Because the function already embeds holistic validation, DAMON_SYSFS reuses it in a safe way. It creates a test-purpose context that is not running but mimics the running one, and calls damon_commit_ctx() against the test purpose context. If it succeeds, the parameters are considered valid, and a real damon_commit_ctx() call against the running context is made with those.

Other callers such as DAMON_RECLAIM and DAMON_LRU_SORT do not expose full parameters to users. For efficiency, they validate only the known set of parameters. The efficiency gain is arguably small and doubtful, though. Meanwhile the maintenance overhead of the multiple different validations is clearly high. We actually found and fixed a few bugs in the class.

Update damon_commit_ctx() to embed DAMON_SYSFS' safe and holistic validation approach. Callers can simply call damon_commit_ctx() without worrying if their parameters are invalid.

Note that damon_commit_ctx() can still cause an unexpected stop of the running context, if internal memory allocation fails. It is arguably unlikely since those internal allocations are too small to fail, but theoretically possible. It should also be better addressed, but not necessarily a blocker of this small and incremental improvement effort.

---
 mm/damon/core.c | 61 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 14 deletions(-)

index 871c6f5257c9e..018dd5ff80324 100644
@@ -1664,20 +1664,7 @@ static int damon_commit_probes(struct damon_ctx *dst, struct damon_ctx *src)
 	return 0;
 }
 
-/**
- * damon_commit_ctx() - Commit parameters of a DAMON context to another.
- * @dst:	The commit destination DAMON context.
- * @src:	The commit source DAMON context.
- *
- * This function copies user-specified parameters from @src to @dst and update
- * the internal status and results accordingly.  Users should use this function
- * for context-level parameters update of running context, instead of manual
- * in-place updates.
- *
- * This function should be called from parameters-update safe context, like
- * damon_call().
- */
-int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
+static int __damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
 {
 	int err;
 	struct damos *scheme;

@@ -1732,6 +1719,52 @@ int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
 	return 0;
 }
 
+static struct damon_ctx *damon_new_test_ctx(struct damon_ctx *dst)
+{
+	struct damon_ctx *test_ctx;
+	int err;
+
+	test_ctx = damon_new_ctx();
+	if (!test_ctx)
+		return NULL;
+	err = __damon_commit_ctx(test_ctx, dst);
+	if (err) {
+		damon_destroy_ctx(test_ctx);
+		return NULL;
+	}
+	return test_ctx;
+}
+
+/**
+ * damon_commit_ctx() - Commit parameters of a DAMON context to another.
+ * @dst:	The commit destination DAMON context.
+ * @src:	The commit source DAMON context.
+ *
+ * This function copies user-specified parameters from @src to @dst and update
+ * the internal status and results accordingly.  Users should use this function
+ * for context-level parameters update of running context, instead of manual
+ * in-place updates.
+ *
+ * This function should be called from parameters-update safe context, like
+ * damon_call().
+ */
+int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
+{
+	struct damon_ctx *test_ctx;
+	int err;
+
+	test_ctx = damon_new_test_ctx(dst);
+	if (!test_ctx)
+		return -ENOMEM;
+	err = __damon_commit_ctx(test_ctx, src);
+	if (err)
+		goto out;
+	err = __damon_commit_ctx(dst, src);
+out:
+	damon_destroy_ctx(test_ctx);
+	return err;
+}
+
 /**
  * damon_nr_running_ctxs() - Return number of currently running contexts.
  */

2.47.3

damon_start() and damon_commit_ctx() are two main DAMON core API functions for setting whole DAMON parameters. While damon_commit_ctx() does holistic parameters testing, damon_start() just believes the caller validated the whole thing. Embed the holistic parameter check that is already in damon_commit_ctx() into damon_start(). After this change, the callers can safely call damon_start() without validating the parameters.

---
 mm/damon/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 018dd5ff80324..ca301abcb9ec5 100644
@@ -1865,6 +1865,8 @@ static int __damon_start(struct damon_ctx *ctx)
 	return err;
 }
 
+static int __damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src);
+
 /**
  * damon_start() - Starts the monitorings for a given group of contexts.
  * @ctxs:	an array of the pointers for contexts to start monitoring

@@ -1886,8 +1888,16 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)
 	int err = 0;
 
 	for (i = 0; i < nr_ctxs; i++) {
-		if (!is_power_of_2(ctxs[i]->min_region_sz))
-			return -EINVAL;
+		struct damon_ctx *test_ctx;
+
+		test_ctx = damon_new_ctx();
+		if (!test_ctx)
+			return -ENOMEM;
+
+		err = __damon_commit_ctx(test_ctx, ctxs[i]);
+		damon_destroy_ctx(test_ctx);
+		if (err)
+			return err;
 	}
 
 	mutex_lock(&damon_lock);

2.47.3

DAMON sysfs interface does parameters validation-purpose damon_commit_ctx() calls for parameters update. Now the same logic is embedded inside damon_commit_ctx() itself. Hence, the validation in DAMON sysfs interface is just an unnecessary duplicate. Remove it.

---
 mm/damon/sysfs.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

index 6710b6d019bf5..e666dddf1feba 100644
@@ -2098,26 +2098,6 @@ static int damon_sysfs_apply_inputs(struct damon_ctx *ctx,
 static struct damon_ctx *damon_sysfs_build_ctx(
 		struct damon_sysfs_context *sys_ctx);
 
-/*
- * Return a new damon_ctx for testing new parameters to commit.
- */
-static struct damon_ctx *damon_sysfs_new_test_ctx(
-		struct damon_ctx *running_ctx)
-{
-	struct damon_ctx *test_ctx;
-	int err;
-
-	test_ctx = damon_new_ctx();
-	if (!test_ctx)
-		return NULL;
-	err = damon_commit_ctx(test_ctx, running_ctx);
-	if (err) {
-		damon_destroy_ctx(test_ctx);
-		return NULL;
-	}
-	return test_ctx;
-}
-
 /*
  * damon_sysfs_commit_input() - Commit user inputs to a running kdamond.
  * @kdamond:	The kobject wrapper for the associated kdamond.

@@ -2127,7 +2107,7 @@ static struct damon_ctx *damon_sysfs_new_test_ctx(
 static int damon_sysfs_commit_input(void *data)
 {
 	struct damon_sysfs_kdamond *kdamond = data;
-	struct damon_ctx *param_ctx, *test_ctx;
+	struct damon_ctx *param_ctx;
 	int err;
 
 	if (!damon_sysfs_kdamond_running(kdamond))

@@ -2139,17 +2119,7 @@ static int damon_sysfs_commit_input(void *data)
 	param_ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
 	if (IS_ERR(param_ctx))
 		return PTR_ERR(param_ctx);
-	test_ctx = damon_sysfs_new_test_ctx(kdamond->damon_ctx);
-	if (!test_ctx) {
-		damon_destroy_ctx(param_ctx);
-		return -ENOMEM;
-	}
-	err = damon_commit_ctx(test_ctx, param_ctx);
-	if (err)
-		goto out;
 	err = damon_commit_ctx(kdamond->damon_ctx, param_ctx);
-out:
-	damon_destroy_ctx(test_ctx);
 	damon_destroy_ctx(param_ctx);
 	return err;
 }

2.47.3

DAMON_RECLAIM validates the user input for min_region_sz. The same validation is done inside damon_start() and damon_commit_ctx(). Remove the duplicate.

---
 mm/damon/reclaim.c | 5 -----
 1 file changed, 5 deletions(-)

index 11b70d0a9a6f0..6469b25cc34f9 100644
@@ -208,11 +208,6 @@ static int damon_reclaim_apply_parameters(void)
 	param_ctx->addr_unit = addr_unit;
 	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
 
-	if (!is_power_of_2(param_ctx->min_region_sz)) {
-		err = -EINVAL;
-		goto out;
-	}
-
 	if (!damon_reclaim_mon_attrs.aggr_interval) {
 		err = -EINVAL;
 		goto out;

2.47.3

No blockers. Please read below for more details.

Users can show and correct it. And this happens only when the user entered an invalid parameter. No bad consequence like kernel panic happens. So, this is just a minor user experience issue. And I want to make validation mminimum for simplicity. This minor issue doesn't deserve to add the complexity in my opinion. I will keep this as is unless others chime in.

Yes, but anyway it cannot be committed due to the validation in the commit. No bad consequene like kernel ppanic happens. So no issue.

Thanks,
SJ

[...]

DAMON_LRU_SORT validates the user input for min_region_sz. The same validation is done inside damon_start() and damon_commit_ctx(). Remove the unnecessary duplicate.

---
 mm/damon/lru_sort.c | 5 -----
 1 file changed, 5 deletions(-)

index 32f41491b726b..2dd0cd0d26273 100644
@@ -284,11 +284,6 @@ static int damon_lru_sort_apply_parameters(void)
 	param_ctx->addr_unit = addr_unit;
 	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
 
-	if (!is_power_of_2(param_ctx->min_region_sz)) {
-		err = -EINVAL;
-		goto out;
-	}
-
 	if (!damon_lru_sort_mon_attrs.sample_interval) {
 		err = -EINVAL;
 		goto out;

2.47.3

The kernel doc comment of damon_region clearly specifies every region should have positive size. But it is unclear who should verify it. damon_set_regions() is the recommended DAMON core function for setting regions from the callers, and has the verification. Update the comment to clarify the callers should be ok to pass any values for region addresses, as long as they use damon_set_regions().

---
 include/linux/damon.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 616bdf0954b52..2661231c0ae82 100644
@@ -49,7 +49,8 @@ struct damon_size_range {
  * @list:		List head for siblings.
  * @age:		Age of this region.
  *
- * For any use case, @ar should be non-zero positive size.
+ * For any use case, @ar should be non-zero positive size.  damon_set_regions()
+ * does the validation.
  *
  * @nr_accesses is reset to zero for every &damon_attrs->aggr_interval and be
  * increased for every &damon_attrs->sample_interval if an access to the region

2.47.3

damon_set_region_system_rams() validates user inputs to avoid creating a negative size region. But DAMON core avoids zero size, too. The check is incomplete. The complete check is done inside damon_set_regions(), which is eventually called from damon_set_region_system_rams_default(). Drop the incomplete and unnecessary check.

---
 mm/damon/core.c | 3 ---
 1 file changed, 3 deletions(-)

index ca301abcb9ec5..3c5a46e1c079a 100644
@@ -3846,9 +3846,6 @@ int damon_set_region_system_rams_default(struct damon_target *t,
 {
 	struct damon_addr_range addr_range;
 
-	if (*start > *end)
-		return -EINVAL;
-
 	if (!*start && !*end &&
 		!damon_find_system_rams_range(start, end, addr_unit))
 		return -EINVAL;

2.47.3

No blocker. Read below for details, please.

Yes, that's how it works, and I find no problem here. No bad consequences like kernel panic happenns. It it might be confusing and better to improve for user experience. However, I don't think this corner case experience deserves the additional complexity. I want to keep the validation simple and minimum.

So I will keep this as is unless others chime in.

Thankss,
SJ

DAMON_SYSFS validates user inputs for monitoring target regions to disallow negative size regions. DAMON core assumes only positive size regions, though. The validation is incomplete. Fortunately damon_set_regions(), which is eventually used by DAMON_SYSFS, does the complete validation. Remove the incomplete and unnecessary validation.

---
 mm/damon/sysfs.c | 3 ---
 1 file changed, 3 deletions(-)

index e666dddf1feba..b65651498e0d1 100644
@@ -1987,9 +1987,6 @@ static int damon_sysfs_set_regions(struct damon_target *t,
 		struct damon_sysfs_region *sys_region =
 			sysfs_regions->regions_arr[i];
 
-		if (sys_region->ar.start > sys_region->ar.end)
-			goto out;
-
 		ranges[i].start = sys_region->ar.start;
 		ranges[i].end = sys_region->ar.end;
 		if (i == 0)

2.47.3

No issue. Read my reply to Sashiko comment for patch 7 for more details.

Thanks,
SJ

[...]

Sashiko found a few things but those are not real issues in my opinion. Please read my replies to Sashiko comments on DAMON mailing list thread [1].

DAMON mailing list is opted in to Sashiko finding mail delivery to the author and the mailing list. I record my opinions to those by replying to the thread. Hence you can show the detailed opinions from DAMON mailing list thread.

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

Thanks,
SJ

[...]

Actually this was included in mm-hotfixes-unstable on July 1(?)

You're correct. I dropped the RFC tag of this patch series because I found it is merged. But I forgot dropping this note. Sorry for causing confusion.

Thanks,
SJ