Re: [PATCH v2 1/5] mm/damon: add target_order field for DAMOS_COLLAPSE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The newly added `target_order` field is not initialized in `damon_sysfs_scheme_alloc()` or `damon_new_scheme()`, leading to kernel heap memory leaks to userspace via sysfs.
- [High] The `target_order` field is not copied when committing scheme updates, causing configuration changes to be ignored.
- [Medium] TOCTOU race in `damon_sysfs_mk_scheme` allows configuring invalid `target_order` for DAMOS_COLLAPSE and DAMOS_SPLIT schemes.
- [Low] Unconditional use of `HPAGE_PMD_ORDER` causes a build failure (`BUILD_BUG()`) on architectures or configurations without huge page support.
--

commit 5b7feca9accb6a6f2fee1a76e45a6ee03f08c17e
Author: Wang Lian <lianux.mm@gmail.com>

mm/damon: add target_order field for DAMOS_COLLAPSE

DAMOS_COLLAPSE currently collapses into PMD-size THP only.  Add a
target_order field to express per-order mTHP collapse intent.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index cfbbf8ba28f63..35b90b47e35a0 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -570,6 +570,11 @@ struct damos_migrate_dests {
>  struct damos {
>  	struct damos_access_pattern pattern;
>  	enum damos_action action;
> +	/*
> +	 * @target_order: target order for mTHP actions (DAMOS_COLLAPSE).
> +	 * 0 means system default (PMD order).  Valid: 0, 2..HPAGE_PMD_ORDER.
> +	 */
> +	unsigned int target_order;

[Severity: High]
Does adding this field require updating damos_commit() in mm/damon/core.c to
copy the new value?

When a user updates the target_order via sysfs and commits the changes,
damos_commit() copies parameters from the pending sysfs scheme to the running
scheme. Since target_order is not copied there, it looks like dynamic
configuration changes to this field will be silently ignored.

[Severity: High]
Is this field properly initialized when allocating a new scheme?

Looking at damon_new_scheme() in mm/damon/core.c, the core struct damos is
allocated using kmalloc_obj() which does not zero-initialize memory. Since
target_order is not explicitly initialized there, it will contain
uninitialized heap data.

[ ... ]
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 41f93a1823bf1..085b8042d1250 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
[ ... ]
> @@ -2644,6 +2647,34 @@ static ssize_t target_nid_store(struct kobject *kobj,
>  	return err ? err : count;
>  }
>  
> +static ssize_t target_order_show(struct kobject *kobj,
> +		struct kobj_attribute *attr, char *buf)
> +{
> +	struct damon_sysfs_scheme *scheme = container_of(kobj,
> +			struct damon_sysfs_scheme, kobj);
> +
> +	return sysfs_emit(buf, "%u\n", scheme->target_order);
> +}

[Severity: High]
Can this leak kernel heap memory to userspace?

The struct damon_sysfs_scheme is allocated in damon_sysfs_scheme_alloc()
using kmalloc_obj(), which does not zero the memory. The new target_order
field is not initialized during allocation.

If a user reads the sysfs file via target_order_show() before writing to it,
uninitialized kernel heap memory will be exposed.

> +
> +static ssize_t target_order_store(struct kobject *kobj,
> +		struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> +	struct damon_sysfs_scheme *scheme = container_of(kobj,
> +			struct damon_sysfs_scheme, kobj);
> +	unsigned int val;
> +	int err;
> +
> +	err = kstrtouint(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +	if (val != 0 && (val < 2 || val > HPAGE_PMD_ORDER))

[Severity: Low]
Will this cause a build failure on configurations without huge page support?

When CONFIG_PGTABLE_HAS_HUGE_LEAVES is not set (e.g. both THP and Hugetlb
are disabled), HPAGE_PMD_SHIFT is defined as a statement expression
triggering a BUILD_BUG().

Since CONFIG_DAMON_SYSFS does not depend on huge page support, evaluating
HPAGE_PMD_ORDER unconditionally here will trigger that build error.

[ ... ]
> @@ -3010,6 +3045,16 @@ static struct damos *damon_sysfs_mk_scheme(
>  	if (!scheme)
>  		return NULL;
>  
> +	if (sysfs_scheme->action == DAMOS_COLLAPSE &&
> +	    sysfs_scheme->target_order != 0 &&
> +	    sysfs_scheme->target_order != HPAGE_PMD_ORDER) {

[Severity: Medium]
Is there a Time-of-Check to Time-of-Use race when reading target_order?

damon_sysfs_mk_scheme() builds the kernel internal struct damos object from
the sysfs state while holding damon_sysfs_lock. However, sysfs property
writers like target_order_store() do not acquire damon_sysfs_lock when
modifying sysfs_scheme->target_order.

Because sysfs_scheme->target_order is read multiple times here:

1. To check if it is valid for DAMOS_COLLAPSE.
2. To log a warning if invalid.
3. For final assignment to scheme->target_order below.

A concurrent write could change the value between the checks and the final
assignment, completely bypassing the validation rules.

> +		pr_warn("DAMON collapse: target_order %u not supported, only PMD order (%u) is available. Use 0 or %u.\n",
> +			sysfs_scheme->target_order,
> +			HPAGE_PMD_ORDER, HPAGE_PMD_ORDER);
> +		sysfs_scheme->target_order = 0;
> +	}
> +	scheme->target_order = sysfs_scheme->target_order;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701114716.56503-1-lianux.mm@gmail.com?part=1