Re: [PATCH] memcg: fix integer overflow in calculate_high_delay() bypassing memory.high throttle

[Johannes Weiner <hannes@cmpxchg.org>]

On Thu, Oct 29, 2026 at 03:38:56PM +0800, zhangjiao2 wrote:
> From: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> 
> calculate_high_delay() computes the throttling penalty as:
> 
>   penalty_jiffies = max_overage * max_overage * HZ;
> 
> where max_overage is a u64 derived from the usage/high ratio shifted
> left by MEMCG_DELAY_PRECISION_SHIFT (20).  When a cgroup exceeds its
> memory.high by a large factor (>= ~4096x), max_overage reaches 2^32
> and the squared multiplication overflows u64, wrapping to a small or
> zero value.  A zero penalty passes the 'penalty_jiffies <= HZ / 100'
> early-exit in __mem_cgroup_handle_over_high(), so the cgroup is never
> throttled despite being massively over limit.
> 
> This is exploitable for denial of service: set memory.high to a small
> value (e.g. 1 MB) and allocate several gigabytes; the overflow causes
> the throttling sleep to be skipped entirely.
> 
> Additionally, on 32-bit kernels the 'unsigned long penalty_jiffies'
> intermediate truncates the u64 product before the right-shifts, which
> can also zero the result.

And then right-shifts by 34, which means the throttling was completely
inert on 32 bit. Please put this explicitly in the changelog.

> Fix both issues by:
> 
>   1. Capping max_overage at (1 << 20) before squaring.  Any overage
>      this large already produces a penalty far above
>      MEMCG_MAX_HIGH_DELAY_JIFFIES, which the caller clamps anyway, so
>      the cap is lossless.
> 
>   2. Using a u64 intermediate for penalty_jiffies so the right-shifts
>      operate on the full 64-bit value on all architectures.
> 
> The post-fix worst-case intermediate is (1<<20)^2 * HZ = 2^40 * HZ,
> which is ~2^50 for HZ=1000 and well within u64 range.
> 
> Signed-off-by: zhang jiao <zhangjiao2@cmss.chinamobile.com>
> ---
>  mm/memcontrol.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 84f4a8fd4d16..c0ab3940025c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2576,7 +2576,7 @@ static u64 swap_find_max_overage(struct mem_cgroup *memcg)
>  static unsigned long calculate_high_delay(unsigned int nr_pages,
>  					  u64 max_overage)
>  {
> -	unsigned long penalty_jiffies;
> +	u64 penalty_jiffies;
>  
>  	if (!max_overage)
>  		return 0;
> @@ -2588,7 +2588,16 @@ static unsigned long calculate_high_delay(unsigned int nr_pages,
>  	 * memcg in question makes it clear that it has no intention of stopping
>  	 * its crazy behaviour, so we exponentially increase the delay based on
>  	 * overage amount.
> +	 *
> +	 * Cap max_overage before squaring to prevent integer overflow that
> +	 * could otherwise wrap the penalty to zero and bypass throttling.
> +	 * Use a u64 intermediate so the right-shifts are not truncated on
> +	 * 32-bit kernels.  Any overage large enough to hit the cap already
> +	 * produces a penalty well above MEMCG_MAX_HIGH_DELAY_JIFFIES, which
> +	 * the caller enforces regardless, so capping is lossless.
>  	 */
> +	max_overage = min_t(u64, max_overage, 1ULL << 20);

This is a bit of a magic value. The simpler fix is the u64 and then do
the multiplications with check_mul_overflow():

	if (check_mul_overflow(max_overage, max_overage, &penalty_jiffies) ||
	    check_mul_overflow(penalty_jiffies, (u64)HZ, &penalty_jiffies))
		return MEMCG_MAX_HIGH_DELAY_JIFFIES;

>  	penalty_jiffies = max_overage * max_overage * HZ;
>  	penalty_jiffies >>= MEMCG_DELAY_PRECISION_SHIFT;
>  	penalty_jiffies >>= MEMCG_DELAY_SCALING_SHIFT;

I would then also make the function return u64 and make the
penalty_jiffies in __mem_cgroup_handle_over_high() u64 as well, since
that's the place where the final clamping occurs, so this function
does not have to worry about it before the return.