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

[SeongJae Park]

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@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit f076dcb3d9e8a7170daf7c4fcb085e3a2c869183
> Author: SeongJae Park <sj@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx


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@xxxxxxxxxx
#
# [1] https://github.com/sjp38/hackermail