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

[SeongJae Park <sj@kernel.org>]

TL; DR: sashiko argues two issues that I think ok to ignore, so I will do so.

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/20260407010536.83603-4-sj@kernel.org
> # start of sashiko.dev inline review
> commit 233e1cffd501d91418c4d1f0badd7f5b509664c0
> 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_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);
> 
> Is it possible for large values of fail_charge_num to cause integer
> overflows in the quota calculation?
> 
> Because kstrtouint allows values up to UINT_MAX without bounds checking,
> the multiplication in mult_frac(sz_region - sz_applied,
> quota->fail_charge_num, quota->fail_charge_denom) during
> damos_charge_quota() might silently overflow the unsigned long capacity.
> 
> Upon overflow, the calculated charge wraps around to a significantly
> smaller value.
> 
> Could this artificially suppress the accumulated quota->charged_sz,
> causing damos_quota_is_full() to incorrectly return false and completely
> bypass the user-configured quota limits?

As I mentioned on the second patch thread, I will just let this overflows if
the user really wants to set such large number, since it will make DAMON
behavior bit weird but not breaking something serious, unless other humans have
different opinions.

> 
> > +
> > +	if (err)
> > +		return -EINVAL;
> > +	return count;
> > +}
> > +
> > +static ssize_t fail_charge_denom_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_denom);
> > +}
> > +
> > +static ssize_t fail_charge_denom_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_denom);
> 
> Can the lack of bounds checking on fail_charge_denom also contribute to
> unexpected behaviors or overflows in the same mult_frac() calculation?

Ditto.

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