mm/damon/core: disallow overlapping input ranges for damon_set_regions()
Subsystems (3)
SJ Park
maintainerPATCH 1/1 mm/damon/core: disallow overlapping input ranges for damon_set_regions()
damon_set_regions() assumes the input ranges are sorted by the address and don't overlap each other. Hence the assumption was initially to be explicitly validated. But commit 97d482f4592f ("mm/damon/sysfs: reuse damon_set_regions() for regions setting") has mistakenly removed the validation.
This can make DAMON behave in unexpected ways. At the best, the monitoring results snapshot will just look weird since there will be overlapping regions. DAMOS will also work weirdly, applying the same action multiple times for overlapping regions, and make DAMOS quota weird. More seriously, depending on the setup and regions updates sequence, negative size regions can be made. It will trigger WARN_ONCE() if the kernel is built with CONFIG_DAMON_DEBUG_SANITY=y. Depending on the monitoring results, the negative size region can further trigger division by zero in damon_merge_two_regions().
Note that some of the consequences including the WARN_ONCE() and the divide by zero depend on commits that were introduced after the root cause commit 97d482f4592f ("mm/damon/sysfs: reuse damon_set_regions() for regions setting").
Fix the problems by checking the assumption and returning an error if the input ranges don't meet the assumption.
The issue was discovered [1] by Sashiko.
[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.keernel.org/[email protected]
- Move consequences divergence note to body of the commit message.
- Rebase to the latest mm-new.
mm/damon/core.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
mm/damon/core.c +9 -2
@@ -418,12 +418,19 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges, {struct damon_region *r, *next;unsigned int i; +unsigned long last_end;int err;for (i =0 ; i < nr_ranges; i++) { -if (ALIGN_DOWN (ranges[i].start, min_region_sz) >= -ALIGN (ranges[i].end, min_region_sz)) +unsigned long start, end; + + start =ALIGN_DOWN (ranges[i].start, min_region_sz); + end =ALIGN (ranges[i].end, min_region_sz); +if (start >= end) +return -EINVAL ; +if (i >0 && last_end > start)return -EINVAL ; + last_end = end; }/* Remove regions which are not in the new ranges */
base-commit: af0487ccfb044dd7d9418c06c6718af630e16311
Signature · 1 line
2.47.3
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Execution of ALIGN() and ALIGN_DOWN() macros with a non-power-of-two min_region_sz, violating alignment subsystem guidelines.
- [High] TOCTOU race due to lockless concurrent reads of sys_ctx->ops_id and sys_ctx->addr_unit.
--
commit b68f48fc29d6aa945e1dd1ef0c694913e40d3854
Author: SJ Park <[email protected]>
mm/damon/core: disallow overlapping input ranges for damon_set_regions()
This commit reinstates validation in damon_set_regions() to ensure that input ranges are sorted and do not overlap. It checks the boundaries and returns an error if they are invalid, preventing unintended DAMON behavior.
Quoted 8 lines
mm/damon/core.c +0
-0@@ -418,12 +418,19 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges, {struct damon_region *r, *next;unsigned int i;
mm/damon/core.c int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges, · line 428
Quoted 10 lines
mm/damon/core.c +5
-2+unsigned long last_end;int err;for (i =0 ; i < nr_ranges; i++) { -if (ALIGN_DOWN (ranges[i].start, min_region_sz) >= -ALIGN (ranges[i].end, min_region_sz)) +unsigned long start, end; + + start =ALIGN_DOWN (ranges[i].start, min_region_sz); + end =ALIGN (ranges[i].end, min_region_sz);
[Severity: High]
This is a pre-existing issue, but does this safely handle a non-power-of-two min_region_sz?
The ALIGN macros rely on bitwise operations that produce incorrect boundaries if the alignment argument isn't a power of two. A user can write a non-power-of-two value to addr_unit via sysfs, which sets min_region_sz to a non-power-of-two in damon_sysfs_apply_inputs():
mm/damon/sysfs.c:damon_sysfs_apply_inputs() {
...
ctx->min_region_sz = max(
DAMON_MIN_REGION_SZ / sys_ctx->addr_unit, 1);
...
}Even though damon_commit_ctx() eventually validates and rejects the configuration, these unaligned boundaries are actively used here to allocate and merge regions during setup. Could this lead to overlapping regions or division by zero before the rejection occurs?
[Severity: High]
This isn't a bug introduced by this patch, but can a time-of-check to time-of-use (TOCTOU) race occur with sys_ctx->ops_id?
damon_sysfs_apply_inputs() locklessly reads sys_ctx->ops_id multiple times:
mm/damon/sysfs.c:damon_sysfs_apply_inputs() {
...
err = damon_select_ops(ctx, sys_ctx->ops_id);
...
if (sys_ctx->ops_id == DAMON_OPS_PADDR) {
...
}Meanwhile, a concurrent sysfs write can update ops_id without acquiring damon_sysfs_lock:
mm/damon/sysfs.c:operations_store() {
...
context->ops_id = ops_name->ops_id;
...
}If ops_id is changed between the damon_select_ops() call and the subsequent check, could it result in a torn configuration where the context operates in one mode (e.g., DAMON_OPS_VADDR) but the region size scaling logic is applied using rules for another mode (DAMON_OPS_PADDR)?
Quoted 8 lines
mm/damon/core.c +4
-0+if (start >= end) +return -EINVAL ; +if (i >0 && last_end > start)return -EINVAL ; + last_end = end; }/* Remove regions which are not in the new ranges */
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
SJ Park
maintainerQuoted 1 line · On Fri, 03 Jul 2026 17:11:41 +0000 [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
No blocker for this patch.
Quoted 3 lines
Pre-existing issues:
- [High] Execution ofALIGN()andALIGN_DOWN()macros with a non-power-of-twomin_region_sz, violating alignment subsystem guidelines.
Wrong finding.
Quoted 1 line
- [High] TOCTOU race due to lockless concurrent reads of
sys_ctx->ops_idandsys_ctx->addr_unit.
Correct finding but not a blocker of this patch.
Quoted 48 lines
--
commit
b68f48fc29d6aa945e1dd1ef0c694913e40d3854
Author: SJ Park <[email protected]>
mm/damon/core: disallow overlapping input ranges fordamon_set_regions()This commit reinstates validation in
damon_set_regions()to ensure that input ranges are sorted and do not overlap. It checks the boundaries and returns an error if they are invalid, preventing unintended DAMON behavior.Quoted 8 lines
mm/damon/core.c +0
-0@@ -418,12 +418,19 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges, {struct damon_region *r, *next;unsigned int i;mm/damon/core.c int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges, · line 428
Quoted 10 lines
mm/damon/core.c +5
-2+unsigned long last_end;int err;for (i =0 ; i < nr_ranges; i++) { -if (ALIGN_DOWN (ranges[i].start, min_region_sz) >= -ALIGN (ranges[i].end, min_region_sz)) +unsigned long start, end; + + start =ALIGN_DOWN (ranges[i].start, min_region_sz); + end =ALIGN (ranges[i].end, min_region_sz);[Severity: High]
This is a pre-existing issue, but does this safely handle a non-power-of-twomin_region_sz?The ALIGN macros rely on bitwise operations that produce incorrect boundaries if the alignment argument isn't a power of two. A user can write a non-power-of-two value to
addr_unitvia sysfs, which setsmin_region_szto a non-power-of-two indamon_sysfs_apply_inputs():mm/damon/sysfs.c:damon_sysfs_apply_inputs() { ... ctx->min_region_sz = max( DAMON_MIN_REGION_SZ / sys_ctx->addr_unit, 1); ... }
Quoted 4 lines
Even though
damon_commit_ctx()eventually validates and rejects the configuration, these unaligned boundaries are actively used here to allocate and merge regions during setup. Could this lead to overlapping regions or division by zero before the rejection occurs?
We do the validation "after" the alignment, so no overlapping region can be created. Hence no division by zero.
Quoted 26 lines
[Severity: High]
This isn't a bug introduced by this patch, but can a time-of-check to time-of-use (TOCTOU) race occur withsys_ctx->ops_id?
damon_sysfs_apply_inputs()locklessly readssys_ctx->ops_idmultiple times:mm/damon/sysfs.c:damon_sysfs_apply_inputs() { ... err = damon_select_ops(ctx, sys_ctx->ops_id); ... if (sys_ctx->ops_id == DAMON_OPS_PADDR) { ... }Meanwhile, a concurrent sysfs write can update
ops_idwithout acquiringdamon_sysfs_lock:mm/damon/sysfs.c:operations_store() { ... context->ops_id = ops_name->ops_id; ... }
Quoted 4 lines
If
ops_idis changed between thedamon_select_ops()call and the subsequent check, could it result in a torn configuration where the context operates in one mode (e.g.,DAMON_OPS_VADDR) but the region size scaling logic is applied using rules for another mode (DAMON_OPS_PADDR)?
Nice catch! Not a blocker of this patch, though, as Sashiko also mentioned. I will separately work on this.
Quoted 12 lines
Quoted 9 lines
mm/damon/core.c +4
-0+if (start >= end) +return -EINVAL ; +if (i >0 && last_end > start)return -EINVAL ; + last_end = end; }/* Remove regions which are not in the new ranges */ Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Thanks,
SJ