Re: [RFC PATCH 6.12.y] mm/damon/core: error damos_commit_quota_goal() for zero target_value

[SJ Park <sj@kernel.org>]

On Sun, 25 Oct 2026 18:33:53 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] mm/damon/sysfs: ctx memory leak in damon_sysfs_turn_damon_on()
> - [Low] mm/damon/sysfs: unknown member targe_value in damon_sysfs_valid_start_ctx()
> --
> 
> commit 1635d9f393d64d22688141e9540fde6f98f5d6bb
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: error damos_commit_quota_goal() for zero target_value
> 
> This patch rejects zero target_value in DAMOS quota goals during parameter
> commit in damos_commit_quota_goal() and damos_commit_quota_goals() to avoid
> division-by-zero errors. For this stable backport, it also adds a validation
> helper before damon_start() in damon_sysfs_turn_damon_on().
> 
> Link: https://lore.kernel.org/20260903010722.94244-1-sj@kernel.org
> Link: https://lore.kernel.org/20260903010722.94244-2-sj@kernel.org
> Link: https://lore.kernel.org/20260803134034.15217-1-sj@kernel.org [1]
> 
> > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> > index 5aa23c6545cb8..7895b11555d06 100644
> > --- a/mm/damon/sysfs.c
> > +++ b/mm/damon/sysfs.c
> > @@ -1497,6 +1497,20 @@ static struct damon_ctx *damon_sysfs_build_ctx(
> >  	return ctx;
> >  }
> >  
> > +static bool damon_sysfs_valid_start_ctx(struct damon_ctx *ctx)
> > +{
> > +	struct damos *s;
> > +	struct damos_quota_goal *g;
> > +
> > +	damon_for_each_scheme(s, ctx) {
> > +		damos_for_each_quota_goal(g, &s->quota) {
> > +			if (!g->targe_value)
> > +				return false;
> > +		}
> > +	}
> > +	return true;
> > +}
> 
> [Severity: Low]
> Will mm/damon/sysfs.c fail to build in damon_sysfs_valid_start_ctx() due to
> referencing g->targe_value instead of g->target_value?

Nice catch.  My build test was somehow configured to disable DAMON_SYSFS.  I
will fix this in the next spin.

Also, my build test found an issue in macro expansion after fixing the config.
I will add below change for the two build issues.

'''
@@ -1503,8 +1503,8 @@ static bool damon_sysfs_valid_start_ctx(struct damon_ctx *ctx)
        struct damos_quota_goal *g;

        damon_for_each_scheme(s, ctx) {
-               damos_for_each_quota_goal(g, &s->quota) {
-                       if (!g->targe_value)
+               damos_for_each_quota_goal(g, (&s->quota)) {
+                       if (!g->target_value)
                                return false;
                }
        }
'''

> 
> In include/linux/damon.h, struct damos_quota_goal defines this field as:
> 
> struct damos_quota_goal {
> 	enum damos_quota_goal_metric metric;
> 	unsigned long target_value;
> 	unsigned long current_value;
> 
> > +
> >  static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
> >  {
> >  	struct damon_ctx *ctx;
> > @@ -1517,6 +1531,8 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
> >  	ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
> >  	if (IS_ERR(ctx))
> >  		return PTR_ERR(ctx);
> > +	if (!damon_sysfs_valid_start_ctx(ctx))
> > +		return -EINVAL;
> 
> [Severity: Medium]
> Does damon_sysfs_turn_damon_on() leak ctx and its associated allocations when
> damon_sysfs_valid_start_ctx(ctx) returns false?

Good catch.  I will destroy the ctx in the error case in the next version, like
below:

'''
@@ -1531,8 +1531,10 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
        ctx = damon_sysfs_build_ctx(kdamond->contexts->contexts_arr[0]);
        if (IS_ERR(ctx))
                return PTR_ERR(ctx);
-       if (!damon_sysfs_valid_start_ctx(ctx))
+       if (!damon_sysfs_valid_start_ctx(ctx)) {
+               damon_destroy_ctx(ctx);
                return -EINVAL;
+       }
        err = damon_start(&ctx, 1, false);
        if (err) {
                damon_destroy_ctx(ctx);
'''

> 
> When userspace writes "on" to the kdamond state sysfs file with a scheme
> quota goal whose target_value is zero, damon_sysfs_build_ctx() allocates ctx
> along with its targets, regions, schemes, quota goals, filters, and any
> target struct pid references. Returning -EINVAL directly here skips
> damon_destroy_ctx(ctx) (and target pid cleanup via
> damon_sysfs_destroy_targets(ctx)), whereas the damon_start() error path right
> below destroys ctx before returning:
> 
> 	err = damon_start(&ctx, 1, false);
> 	if (err) {
> 		damon_destroy_ctx(ctx);
> 		return err;
> 	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261025182326.178326-1-sj@kernel.org?part=1


Thanks,
SJ