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

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

[SeongJae Park]

On Thu, 19 Mar 2026 15:57:42 +0000 Josh Law <objecting@objecting.org> wrote:

> damon_sysfs_repeat_call_fn() accesses contexts_arr[0] in
> upd_tuned_intervals, upd_schemes_stats, and upd_schemes_effective_quotas
> without checking nr_contexts. A user can set nr_contexts to 0 via sysfs
> while DAMON is running, causing a NULL pointer dereference in the
> repeat callback. Add a guard under the lock.

Good catch!

Priveleged users can trigger this.

    # damo start --refresh_stat 1s
    # echo 0 > /sys/kernel/mm/damon/admin/kdamonds/0/contexts/nr_contexts
    # dmesg
    [...]
    [  277.616182] BUG: kernel NULL pointer dereference, address: 0000000000000000
    [...]

So, I think this deserves Fixes: and Cc: stable.

Fixes: d809a7c64ba8 ("mm/damon/sysfs: implement refresh_ms file internal work")
Cc: <stable@vger.kernel.org> # 6.17.x

> 
> Signed-off-by: Josh Law <objecting@objecting.org>

Reviewed-by: SeongJae Park <sj@kernel.org>
> ---
>  mm/damon/sysfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index ddcdc4e35b27..d982f2dc7a2b 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);
>  	damon_sysfs_upd_schemes_stats(sysfs_kdamond);
>  	damon_sysfs_upd_schemes_effective_quotas(sysfs_kdamond);
> +out:
>  	mutex_unlock(&damon_sysfs_lock);
>  	return 0;
>  }
> -- 
> 2.34.1

Sashiko comment.  TL; DR: Good points, but orthogonal to this patch.  I will
work for those.

: Does this check fully prevent the NULL pointer dereference? A user can still
: manually trigger these callbacks by writing commands like
: update_tuned_intervals or update_schemes_stats to the state sysfs file.
: 
: Those commands execute through damon_sysfs_handle_cmd() and
: damon_sysfs_damon_call(), which add the callback to the running DAMON thread
: without checking if contexts->nr != 1. When the kdamond thread executes the
: manual command, it seems it would unconditionally dereference
: contexts_arr[0], which could be NULL.
: 
: Would it be safer to place the contexts->nr != 1 check directly inside the
: callback functions (damon_sysfs_upd_tuned_intervals, etc.) to cover all
: entry points?

Good point, but this is orthogonal to this patch.

And the second and the third patches of this series is actually making fixes
for parts of the problems Sashiko mentioned.  I also have an opinion to fix
similar issues in one place.  Let's discuss that on the second patch.

: 
: >  	damon_sysfs_upd_tuned_intervals(sysfs_kdamond);
: >  	damon_sysfs_upd_schemes_stats(sysfs_kdamond);
: >  	damon_sysfs_upd_schemes_effective_quotas(sysfs_kdamond);
: > +out:
: >  	mutex_unlock(&damon_sysfs_lock);
: >  	return 0;
: >  }
: 
: Looking at the context just above this diff in damon_sysfs_repeat_call_fn(),
: is it intentional that damon_sysfs_next_update_jiffies is a static global
: variable?
: 
: static int damon_sysfs_repeat_call_fn(void *data)
: {
:         struct damon_sysfs_kdamond *sysfs_kdamond = data;
: 
:         if (!sysfs_kdamond->refresh_ms)
:                 return 0;
:         if (time_before(jiffies, damon_sysfs_next_update_jiffies))
:                 return 0;
:         damon_sysfs_next_update_jiffies = jiffies +
:                 msecs_to_jiffies(sysfs_kdamond->refresh_ms);
: 
: Since users can create multiple independent kdamond instances via sysfs,
: each running its own thread with a unique refresh_ms interval, multiple
: threads running this function could concurrently read and overwrite the exact
: same timer.
: 
: This seems like it would cause independent kdamond instances to clobber each
: other's refresh cycles, resulting in delayed or skipped interval updates.
: Could this be moved to a per-kdamond field?

Makes sense, but orthogonal to this patch.  I will work on this.

: 
: Also, looking at damon_sysfs_turn_damon_on() which sets up this repeat
: call function:
: 
: static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
: {
:         ...
:         repeat_call_control = kmalloc_obj(*repeat_call_control);
:         ...
:         err = damon_start(&ctx, 1, false);
:         ...
:         damon_call(ctx, repeat_call_control);
:         return err;
: }
: 
: Could this leak the repeat_call_control allocation?
: 
: If the kdamond thread exits prematurely right after damon_start(), the
: context becomes inactive. When damon_call() executes, it checks
: damon_is_running(). Since it is false, damon_call_handle_inactive_ctx()
: unlinks the control block and returns -EINVAL without freeing the control
: block or evaluating dealloc_on_cancel.
: 
: Because damon_sysfs_turn_damon_on() ignores the return value of damon_call(),
: returning the success code from damon_start() instead, it appears the memory
: allocated for repeat_call_control is lost.

Makes sense, but orthogonal to this patch.  I will work on this.

# review url: https://sashiko.dev/#/patchset/20260319155742.186627-5-objecting@objecting.org


Thanks,
SJ