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

[SeongJae Park <sj@kernel.org>]

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