Re: [PATCH 04/11] mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

[SeongJae Park <sj@kernel.org>]

On Tue, 28 Apr 2026 01:54:35 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 1 potential issue(s):
> - [Medium] The sysfs store functions for `fail_charge_num` and `fail_charge_denom` do not validate or bound the ratio, leading to potential integer overflow in the core quota tracking logic.

No blocker.  Please read below for more details.

> --
> 
> commit 43a135a4abb7954bcb255ae58b1aa323b2447477
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files
> 
> This commit implements the user-space ABI for the DAMOS action failed region
> quota-charge ratio setup. It adds sysfs files for fail_charge_num and
> fail_charge_denom to allow reading and setting the numerator and
> denominator of the failed regions charge ratio.
> 
> > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > index 04746cbb3327..dee1f5788bea 100644
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
> [ ... ]
> > +static ssize_t fail_charge_num_store(struct kobject *kobj,
> > +		struct kobj_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> > +			struct damon_sysfs_quotas, kobj);
> > +	int err = kstrtouint(buf, 0, &quotas->fail_charge_num);
> > +
> > +	if (err)
> > +		return -EINVAL;
> > +	return count;
> > +}
> 
> Can this unbounded input lead to an integer overflow in the core quota
> tracking logic?
> 
> The values read via kstrtouint() are unconstrained up to UINT_MAX. Later, in
> damos_charge_quota(), these values are passed to:
> 
> mult_frac(sz_region - sz_applied, quota->fail_charge_num,
>           quota->fail_charge_denom)
> 
> If the region size in bytes is large (for example, a single DAMON region of
> 10 GB) and the numerator is set near UINT_MAX, their product could exceed
> ULONG_MAX. For instance, 10GB * UINT_MAX is roughly 40 * 10^18, which is
> larger than ULONG_MAX's 18.4 * 10^18.
> 
> Could this cause an overflow that wraps around to an anomalously small value,
> effectively bypassing the administrator's configured quota limits?

Yes, and the real world use case ain't set such extreme input.  Even if the
overflow happens, the impact is minor.

> 
> > +
> > +static ssize_t fail_charge_denom_show(struct kobject *kobj,
> > +		struct kobj_attribute *attr, char *buf)
> > +{
> > +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> > +			struct damon_sysfs_quotas, kobj);
> > +
> > +	return sysfs_emit(buf, "%u\n", quotas->fail_charge_denom);
> > +}
> > +
> > +static ssize_t fail_charge_denom_store(struct kobject *kobj,
> > +		struct kobj_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct damon_sysfs_quotas *quotas = container_of(kobj,
> > +			struct damon_sysfs_quotas, kobj);
> > +	int err = kstrtouint(buf, 0, &quotas->fail_charge_denom);
> > +
> > +	if (err)
> > +		return -EINVAL;
> > +	return count;
> > +}
> 
> Does the fail_charge_denom input also require bounding to prevent the same
> overflow issues when passed into the quota tracking logic?

My opinion is same to the above one.


Thanks,
SJ

[...]