Re: [RFC PATCH] mm/damon/core: avoid overflow in damon_feed_loop_next_input()

[Guenter Roeck <linux@roeck-us.net>]

On 9/5/24 10:24, SeongJae Park wrote:
> damon_feed_loop_next_input() is fragile to overflows.  Rewrite code to
> avoid overflows.  This is not yet well tested on 32bit archs.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/944f3d5b-9177-48e7-8ec9-7f1331a3fea3@roeck-us.net
> Fixes: 9294a037c015 ("mm/damon/core: implement goal-oriented feedback-driven quota auto-tuning")
> Signed-off-by: SeongJae Park <sj@kernel.org>

Passes all my testing except for

Failed unit tests:
	arm:mps2-an385:damon_test_ops_registration
	arm:mps2-an385:damon
	xtensa:de212:damon_test_ops_registration
	xtensa:de212:damon

which unrelated and not surprising since those are nommu systems.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> ---
> As mentioned on the commit message, this is not yet sufficiently tested
> on 32bit machines.  That's why this is RFC.
> 
>   mm/damon/core.c | 33 +++++++++++++++++++++++++++------
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 32677f13f437..1d951c2a1d85 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1494,15 +1494,36 @@ static unsigned long damon_feed_loop_next_input(unsigned long last_input,
>   		unsigned long score)
>   {
>   	const unsigned long goal = 10000;
> -	unsigned long score_goal_diff = max(goal, score) - min(goal, score);
> -	unsigned long score_goal_diff_bp = score_goal_diff * 10000 / goal;
> -	unsigned long compensation = last_input * score_goal_diff_bp / 10000;
>   	/* Set minimum input as 10000 to avoid compensation be zero */
>   	const unsigned long min_input = 10000;
> +	unsigned long score_goal_diff;
> +	unsigned long compensation;
> +
> +	if (score == goal)
> +		return last_input;
> +
> +	/* last_input, score <= ULONG_MAX */
> +	if (score < goal) {
> +		score_goal_diff = goal - score;
> +	} else {
> +		/* if score_goal_diff > goal, will return min_input anyway */
> +		score_goal_diff = min(score - goal, goal);
> +	}
> +
> +	if (last_input < ULONG_MAX / score_goal_diff)
> +		compensation = last_input * score_goal_diff / goal;
> +	else
> +		compensation = last_input / goal * score_goal_diff;
> +
> +	/* compensation <= last_input <= ULONG_MAX */
> +
> +	if (goal > score) {
> +		if (last_input < ULONG_MAX - compensation)
> +			return last_input + compensation;
> +		return ULONG_MAX;
> +	}
>   
> -	if (goal > score)
> -		return last_input + compensation;
> -	if (last_input > compensation + min_input)
> +	if (last_input - compensation > min_input)
>   		return last_input - compensation;
>   	return min_input;
>   }