Re: (sashiko review) [RFC PATCH v5 04/11] mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

SeongJae Park
2026-04-10T23:58:22Z

TL; DR: sashiko asking a question that was raised on the previous version and I
decided to ignore. My opinion is still same.

Forwarding sashiko review in a reply format with my inline
comments below, for details of my view and doing discussions
via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20260410142034.83798-5-sj@kernel.org
> # start of sashiko.dev inline review
> commit efc09e724c58ae44645ba20d1865e976eae67242
> 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 fail_charge_num and fail_charge_denom
> sysfs files for 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 5186966dafb35..d5bfba79f105f 100644
> > --- a/mm/damon/sysfs-schemes.c
> > +++ b/mm/damon/sysfs-schemes.c
>
> [ ... ]
>
> > @@ -1663,6 +1665,48 @@ static ssize_t goal_tuner_store(struct kobject *kobj,
> > return -EINVAL;
> > }
> >
> > +static ssize_t fail_charge_num_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_num);
> > +}
> > +
> > +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);
>
> Could unbounded values set here cause an integer overflow in
> damos_charge_quota() on 32-bit architectures?
>
> Because kstrtouint() allows values up to UINT_MAX, fail_charge_num and
> fail_charge_denom can be quite large. When these are used in mult_frac()
> for calculating the fractional charge:
>
> mult_frac((sz_region - sz_applied), quota->fail_charge_num,
> quota->fail_charge_denom)
>
> The mult_frac(x, n, d) macro computes (x / d) * n + (x % d) * n / d, where
> the types are determined by typeof(x). Here x is an unsigned long.
>
> If userspace provides large values like 1,000,000 for both the numerator
> and denominator, the remainder (x % d) can be up to 999,999. The
> intermediate multiplication (x % d) * n could be around 10^11, which would
> silently overflow ULONG_MAX on 32-bit systems.
>
> Is this potential integer overflow a regression that results in an incorrectly
> small fractional charge, causing DAMON to under-charge the quota limit and
> bypass the configured scheme quotas?
>
>
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260410142034.83798-5-sj@kernel.org

Thanks,
SJ

# hkml [1] generated a draft of this mail. You can regenerate
# this using below command:
#
# hkml patch sashiko_dev --for_forwarding \
# 20260410142034.83798-5-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail