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

[SeongJae Park]

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@xxxxxxxxxx
# start of sashiko.dev inline review
commit 349107c02ee8a122119ac77a51451bb3d9311a34
Author: Josh Law <objecting@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx

> 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.

>  	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?


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260321175427.86000-4-sj@xxxxxxxxxx
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260321175427.86000-4-sj@xxxxxxxxxx
#
# [1] https://github.com/sjp38/hackermail

Sent using hkml (https://github.com/sjp38/hackermail)