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

SeongJae Park
2026-04-04T20:52:40Z

sashiko found no real issue for this patch.

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/20260404163943.89278-3-sj@kernel.org
> # start of sashiko.dev inline review
> commit ee565644ac37e8bfceab5e62746d85906fb91b43
> 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 two new sysfs files under the DAMON sysfs
> interface for DAMOS quotas, fail_charge_num and fail_charge_denom, which
> 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 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);
> > +
> > + if (err)
> > + return -EINVAL;
>
> Could this lack of upper bounds checking lead to an integer overflow when
> the scheme is applied in the DAMON core?
>
> If a user provides a large value for fail_charge_num up to UINT_MAX, could the
> charge calculation in damos_apply_scheme():
>
> (sz - sz_applied) * quota->fail_charge_num / quota->fail_charge_denom
>
> overflow the unsigned long value?
>
> For example, on a 32-bit system, an unapplied region of just 5 MB multiplied
> by a user-supplied numerator of 1,000 yields over 5 billion, which exceeds
> the 32-bit maximum limit. On a 64-bit system, a 10 GB region multiplied by
> 4,000,000,000 would similarly overflow.
>
> When this overflow occurs, does the charged size wrap around to a very small
> value, causing the DAMOS action quota to be drastically undercharged and
> bypassing the user-defined limits?

That can happen, but shouldn't cause a real problem. Meanwhile, adding such
overflow check would make code unnecessarily complicated. I will keep this as
is. If it turns out this causes a real problem, we can consider making this
better documented or updating the code later, after the report is really made.

Thanks,
SJ

>
> > + return count;
> > +}
>
> [ ... ]
>
>
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260404163943.89278-3-sj@kernel.org

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