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

SeongJae Park
2026-04-05T19:31:00Z

TL; DR: sashiko found no issue in this patch.

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

> # review url: https://sashiko.dev/#/patchset/20260405151232.102690-3-sj@kernel.org
> # start of sashiko.dev inline review
> commit f076dcb3d9e8a7170daf7c4fcb085e3a2c869183
> Author: SeongJae Park <sj@kernel.org>
>
> mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files
>
> Implement the user-space ABI for the DAMOS action failed region
> quota-charge ratio setup. For this, add two new sysfs files under the
> DAMON sysfs interface for DAMOS quotas.
>
> > 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_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;
>
> Could this introduce a regression by allowing an integer overflow when
> calculating the charge ratio?
>
> By using kstrtouint(), fail_charge_num can be set to any value up to
> UINT_MAX.
>
> Later, in mm/damon/core.c:damos_charge_quota(), this numerator is passed to
> mult_frac(sz_region - sz_applied, fail_charge_num, fail_charge_denom).
>
> The mult_frac() macro performs multiplications with the numerator. If a very
> large fail_charge_num is provided, could this overflow the unsigned long
> result, especially on 32-bit architectures?
>
> When the calculation wraps around, quota->charged_sz could be incremented by
> a small or negative-equivalent value, potentially bypassing the DAMOS scheme
> quota limit.

Same question was raised to the previous version of this patch, and I decided
to ignore this. Read the previous discussion for details.

>
> > + return count;
> > +}
>
>
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260405151232.102690-3-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 \
# 20260405151232.102690-3-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail