Re: [PATCH v3 3/3] mm/damon/sysfs: check contexts->nr in repeat_call_fn

SeongJae Park
2026-03-21T20:10:37Z

TL; DR: no blocker for this patch is found.

On Sat, 21 Mar 2026 13:05:22 -0700 SeongJae Park <sj@kernel.org> wrote:

> Subject: (Sashiko) Re: [PATCH v3 3/3] mm/damon/sysfs: check contexts->nr in repeat_call_fn
>
> Forwarding Sashiko review for doing discussions via mails.
>
> # review url: https://sashiko.dev/#/patchset/20260321175427.86000-4-sj@kernel.org
> # start of sashiko.dev inline review
> commit 349107c02ee8a122119ac77a51451bb3d9311a34
> Author: Josh Law <objecting@objecting.org>
>
> mm/damon/sysfs: check contexts->nr in repeat_call_fn
>
> damon_sysfs_repeat_call_fn() calls damon_sysfs_upd_tuned_intervals(),
> damon_sysfs_upd_schemes_stats(), and
> damon_sysfs_upd_schemes_effective_quotas() without checking
> contexts->nr. If nr_contexts is set to 0 via sysfs while DAMON is
> running, these functions dereference contexts_arr[0] and cause a NULL
> pointer dereference. Add the missing check.
>
> Link: https://patch.msgid.link/20260320163559.178101-3-objecting@objecting.org
>
> > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> > index ddc30586c0e6..6a44a2f3d8fc 100644
> > --- a/mm/damon/sysfs.c
> > +++ b/mm/damon/sysfs.c
> > @@ -1620,9 +1620,12 @@ static int damon_sysfs_repeat_call_fn(void *data)
> >
> > if (!mutex_trylock(&damon_sysfs_lock))
> > return 0;
> > + if (sysfs_kdamond->contexts->nr != 1)
> > + goto out;
> > damon_sysfs_upd_tuned_intervals(sysfs_kdamond);
>
> Does this code cause starvation or incorrect statistics when multiple
> kdamonds are active?
>
> It appears that damon_sysfs_next_update_jiffies is declared as a static
> global variable and used by this function to throttle updates:
>
> static unsigned long damon_sysfs_next_update_jiffies;
>
> static int damon_sysfs_repeat_call_fn(void *data)
> {
> ...
> if (time_before(jiffies, damon_sysfs_next_update_jiffies))
> return 0;
> damon_sysfs_next_update_jiffies = jiffies +
> msecs_to_jiffies(sysfs_kdamond->refresh_ms);
> ...
> }
>
> If multiple kdamonds run concurrently, wouldn't they all share and
> overwrite this single global timestamp?
>
> If one kdamond sets the timestamp to a future value, another kdamond's
> thread might incorrectly evaluate time_before() to true and skip its
> update, potentially leading to starvation and incorrect sysfs
> statistics for all but the winning kdamond.

I answered this in the previous version of this patch. In short, good finding
but not a blocker of this patch, I will work on it.

>
> > damon_sysfs_upd_schemes_stats(sysfs_kdamond);
> > damon_sysfs_upd_schemes_effective_quotas(sysfs_kdamond);
> > +out:
> > mutex_unlock(&damon_sysfs_lock);
> > return 0;
> > }
>
> Could there also be a memory leak of repeat_call_control in
> damon_sysfs_turn_damon_on() if the target kdamond exits early?
>
> In damon_sysfs_turn_damon_on(), repeat_call_control is allocated:
>
> repeat_call_control = kmalloc_obj(*repeat_call_control);
>
> If the kdamond thread encounters an error or has no work, it can exit
> and set ctx->kdamond = NULL before damon_call() is fully executed.
>
> In this race window, if damon_call() detects that the context is
> inactive it calls damon_call_handle_inactive_ctx(). This removes the
> control from the list and returns -EINVAL, but it does not free the
> memory.
>
> Because damon_sysfs_turn_damon_on() ignores the return value of
> damon_call(), does this result in a permanent leak of the memory
> allocated for repeat_call_control?

I answered this in the previous version of this patch. In short, good finding
but not a blocker of this patch, I will work on it.

>
>
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260321175427.86000-4-sj@kernel.org

Thanks,
SJ …