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

[SeongJae Park <sj@kernel.org>]

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