[PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[Ridong Chen]

From: Ridong Chen <chenridong@xiaomi.com>

effective_protection() scales a parent's protection by a ratio of page
counts, e.g. for recursive protection:

	(parent_effective - siblings_protected) * (usage - protected)
		/ (parent_usage - siblings_protected)

The multiply is done at unsigned long width before dividing. On systems
with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
protection value and silently breaking memory.min/low enforcement.

Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
usage and parent_usage are not read atomically (a child is charged
before its parent), usage - protected can briefly exceed the divisor,
making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
the ratio stays <= 1.

Reported by the sashiko review tool [1].

[1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1

Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Reviewed-by: Barry Song <baohua@kernel.org>
Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
---
 mm/page_counter.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/mm/page_counter.c b/mm/page_counter.c
index 661e0f2a5127..e8bd512069c5 100644
--- a/mm/page_counter.c
+++ b/mm/page_counter.c
@@ -8,6 +8,7 @@
 #include <linux/page_counter.h>
 #include <linux/atomic.h>
 #include <linux/kernel.h>
+#include <linux/math64.h>
 #include <linux/string.h>
 #include <linux/sched.h>
 #include <linux/bug.h>
@@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
 	 * otherwise get a smaller chunk than what they claimed.
 	 */
 	if (siblings_protected > parent_effective)
-		return protected * parent_effective / siblings_protected;
+		return mul_u64_u64_div_u64(protected, parent_effective,
+					   siblings_protected);
 
 	/*
 	 * Ok, utilized protection of all children is within what the
@@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
 	if (parent_effective > siblings_protected &&
 	    parent_usage > siblings_protected &&
 	    usage > protected) {
-		unsigned long unclaimed;
+		unsigned long unclaimed = parent_effective - siblings_protected;
+		unsigned long unprotected = usage - protected;
+		unsigned long parent_unprotected = parent_usage - siblings_protected;
 
-		unclaimed = parent_effective - siblings_protected;
-		unclaimed *= usage - protected;
-		unclaimed /= parent_usage - siblings_protected;
+		/*
+		 * The usages aren't read atomically, so a child can transiently
+		 * appear to use more than its parent, making the ratio exceed 1
+		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
+		 */
+		unprotected = min(unprotected, parent_unprotected);
 
-		ep += unclaimed;
+		ep += mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected);
 	}
 
 	return ep;
-- 
2.34.1

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[Johannes Weiner]

On Thu, Sep 03, 2026 at 11:19:51AM +0800, Ridong Chen wrote:
> From: Ridong Chen <chenridong@xiaomi.com>
> 
> effective_protection() scales a parent's protection by a ratio of page
> counts, e.g. for recursive protection:
> 
> 	(parent_effective - siblings_protected) * (usage - protected)
> 		/ (parent_usage - siblings_protected)
> 
> The multiply is done at unsigned long width before dividing. On systems
> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
> protection value and silently breaking memory.min/low enforcement.
> 
> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
> usage and parent_usage are not read atomically (a child is charged
> before its parent), usage - protected can briefly exceed the divisor,
> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
> the ratio stays <= 1.
> 
> Reported by the sashiko review tool [1].
> 
> [1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1
> 
> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Reviewed-by: Barry Song <baohua@kernel.org>
> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
> ---
>  mm/page_counter.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 661e0f2a5127..e8bd512069c5 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -8,6 +8,7 @@
>  #include <linux/page_counter.h>
>  #include <linux/atomic.h>
>  #include <linux/kernel.h>
> +#include <linux/math64.h>
>  #include <linux/string.h>
>  #include <linux/sched.h>
>  #include <linux/bug.h>
> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
>  	 * otherwise get a smaller chunk than what they claimed.
>  	 */
>  	if (siblings_protected > parent_effective)
> -		return protected * parent_effective / siblings_protected;
> +		return mul_u64_u64_div_u64(protected, parent_effective,
> +					   siblings_protected);
>  
>  	/*
>  	 * Ok, utilized protection of all children is within what the
> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
>  	if (parent_effective > siblings_protected &&
>  	    parent_usage > siblings_protected &&
>  	    usage > protected) {
> -		unsigned long unclaimed;
> +		unsigned long unclaimed = parent_effective - siblings_protected;
> +		unsigned long unprotected = usage - protected;
> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
>  
> -		unclaimed = parent_effective - siblings_protected;
> -		unclaimed *= usage - protected;
> -		unclaimed /= parent_usage - siblings_protected;
> +		/*
> +		 * The usages aren't read atomically, so a child can transiently
> +		 * appear to use more than its parent, making the ratio exceed 1
> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
> +		 */
> +		unprotected = min(unprotected, parent_unprotected);

Looks correct to me. But a few nits on readability, since this code
already is quite painfully complicated.

Please don't do math in the declaration block.

`unclaimed` made a bit more sense when it held *this group's* final
share of the unclaimed protection. As an intermediate, it's *the
parent's* unclaimed protection.

Put together, it should look something like this:

		unsigned long parent_unclaimed, parent_unprotected, unprotected;

		parent_unclaimed = parent_effective - siblings_protected;
		parent_unprotected = parent_usage - siblings_protected;
		unprotected = usage - protected;

		/* overflow comment */
		unprotected = min(usage - protected, parent_unprotected);
		ep += mul_u64_u64_div_u64(parent_unclaimed, unprotected, parent_unprotected);

With that,

Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[Ridong Chen]

On 9/3/2026 10:00 PM, Johannes Weiner wrote:
> On Thu, Sep 03, 2026 at 11:19:51AM +0800, Ridong Chen wrote:
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> effective_protection() scales a parent's protection by a ratio of page
>> counts, e.g. for recursive protection:
>>
>> 	(parent_effective - siblings_protected) * (usage - protected)
>> 		/ (parent_usage - siblings_protected)
>>
>> The multiply is done at unsigned long width before dividing. On systems
>> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
>> protection value and silently breaking memory.min/low enforcement.
>>
>> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
>> usage and parent_usage are not read atomically (a child is charged
>> before its parent), usage - protected can briefly exceed the divisor,
>> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
>> the ratio stays <= 1.
>>
>> Reported by the sashiko review tool [1].
>>
>> [1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1
>>
>> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
>> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-4-8
>> Reviewed-by: Barry Song <baohua@kernel.org>
>> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
>> ---
>>   mm/page_counter.c | 19 +++++++++++++------
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/page_counter.c b/mm/page_counter.c
>> index 661e0f2a5127..e8bd512069c5 100644
>> --- a/mm/page_counter.c
>> +++ b/mm/page_counter.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/page_counter.h>
>>   #include <linux/atomic.h>
>>   #include <linux/kernel.h>
>> +#include <linux/math64.h>
>>   #include <linux/string.h>
>>   #include <linux/sched.h>
>>   #include <linux/bug.h>
>> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
>>   	 * otherwise get a smaller chunk than what they claimed.
>>   	 */
>>   	if (siblings_protected > parent_effective)
>> -		return protected * parent_effective / siblings_protected;
>> +		return mul_u64_u64_div_u64(protected, parent_effective,
>> +					   siblings_protected);
>>   
>>   	/*
>>   	 * Ok, utilized protection of all children is within what the
>> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
>>   	if (parent_effective > siblings_protected &&
>>   	    parent_usage > siblings_protected &&
>>   	    usage > protected) {
>> -		unsigned long unclaimed;
>> +		unsigned long unclaimed = parent_effective - siblings_protected;
>> +		unsigned long unprotected = usage - protected;
>> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
>>   
>> -		unclaimed = parent_effective - siblings_protected;
>> -		unclaimed *= usage - protected;
>> -		unclaimed /= parent_usage - siblings_protected;
>> +		/*
>> +		 * The usages aren't read atomically, so a child can transiently
>> +		 * appear to use more than its parent, making the ratio exceed 1
>> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
>> +		 */
>> +		unprotected = min(unprotected, parent_unprotected);
> 
> Looks correct to me. But a few nits on readability, since this code
> already is quite painfully complicated.
> 
> Please don't do math in the declaration block.
> 
> `unclaimed` made a bit more sense when it held *this group's* final
> share of the unclaimed protection. As an intermediate, it's *the
> parent's* unclaimed protection.
> 
> Put together, it should look something like this:
> 
> 		unsigned long parent_unclaimed, parent_unprotected, unprotected;
> 
> 		parent_unclaimed = parent_effective - siblings_protected;
> 		parent_unprotected = parent_usage - siblings_protected;
> 		unprotected = usage - protected;
> 
> 		/* overflow comment */
> 		unprotected = min(usage - protected, parent_unprotected);
> 		ep += mul_u64_u64_div_u64(parent_unclaimed, unprotected, parent_unprotected);
> 
> With that,
> 
> Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>

Thank you for your suggestion. Will update.

-- 
Best regards
Ridong

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[David Laight]

On Thu,  3 Sep 2026 11:19:51 +0800
Ridong Chen <ridong.chen@linux.dev> wrote:

> From: Ridong Chen <chenridong@xiaomi.com>
> 
> effective_protection() scales a parent's protection by a ratio of page
> counts, e.g. for recursive protection:
> 
> 	(parent_effective - siblings_protected) * (usage - protected)
> 		/ (parent_usage - siblings_protected)
> 
> The multiply is done at unsigned long width before dividing. On systems
> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
> protection value and silently breaking memory.min/low enforcement.
> 
> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
> usage and parent_usage are not read atomically (a child is charged
> before its parent), usage - protected can briefly exceed the divisor,
> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
> the ratio stays <= 1.
> 
> Reported by the sashiko review tool [1].
> 
> [1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1
> 
> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Reviewed-by: Barry Song <baohua@kernel.org>
> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
> ---
>  mm/page_counter.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index 661e0f2a5127..e8bd512069c5 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -8,6 +8,7 @@
>  #include <linux/page_counter.h>
>  #include <linux/atomic.h>
>  #include <linux/kernel.h>
> +#include <linux/math64.h>
>  #include <linux/string.h>
>  #include <linux/sched.h>
>  #include <linux/bug.h>
> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
>  	 * otherwise get a smaller chunk than what they claimed.
>  	 */
>  	if (siblings_protected > parent_effective)
> -		return protected * parent_effective / siblings_protected;
> +		return mul_u64_u64_div_u64(protected, parent_effective,
> +					   siblings_protected);

On 32bit it is only necessary to use a 64bit intermediary.
mul_u64_u64_div_u64() will drop back to the (probably faster) 64 by 64
divide (and then maybe to a 64 by 32 one).
But there is a lot of extra code before that happens.

>  
>  	/*
>  	 * Ok, utilized protection of all children is within what the
> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
>  	if (parent_effective > siblings_protected &&
>  	    parent_usage > siblings_protected &&
>  	    usage > protected) {
> -		unsigned long unclaimed;
> +		unsigned long unclaimed = parent_effective - siblings_protected;
> +		unsigned long unprotected = usage - protected;
> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
>  
> -		unclaimed = parent_effective - siblings_protected;
> -		unclaimed *= usage - protected;
> -		unclaimed /= parent_usage - siblings_protected;
> +		/*
> +		 * The usages aren't read atomically, so a child can transiently
> +		 * appear to use more than its parent, making the ratio exceed 1
> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
> +		 */
> +		unprotected = min(unprotected, parent_unprotected);
>  
> -		ep += unclaimed;
> +		ep += mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected);

If the ratio is forced to 1 there is no point doing the scaling.
So maybe:
		if (likely(parent_unprotected > unprotected))
			unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected,
							parent_unprotected);
		ep += unclaimed;
OTOH if the min() generates a cmov rather than a conditional branch
then you don't get a statically mispredicted branch in the normal case
(which is very likely with the empty 'else' branch).

David

>  	}
>  
>  	return ep;

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[Ridong Chen]

On 9/4/2026 4:37 PM, David Laight wrote:
> On Thu,  3 Sep 2026 11:19:51 +0800
> Ridong Chen <ridong.chen@linux.dev> wrote:
> 
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> effective_protection() scales a parent's protection by a ratio of page
>> counts, e.g. for recursive protection:
>>
>> 	(parent_effective - siblings_protected) * (usage - protected)
>> 		/ (parent_usage - siblings_protected)
>>
>> The multiply is done at unsigned long width before dividing. On systems
>> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
>> protection value and silently breaking memory.min/low enforcement.
>>
>> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
>> usage and parent_usage are not read atomically (a child is charged
>> before its parent), usage - protected can briefly exceed the divisor,
>> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
>> the ratio stays <= 1.
>>
>> Reported by the sashiko review tool [1].
>>
>> [1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1
>>
>> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
>> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-4-8
>> Reviewed-by: Barry Song <baohua@kernel.org>
>> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
>> ---
>>   mm/page_counter.c | 19 +++++++++++++------
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/page_counter.c b/mm/page_counter.c
>> index 661e0f2a5127..e8bd512069c5 100644
>> --- a/mm/page_counter.c
>> +++ b/mm/page_counter.c
>> @@ -8,6 +8,7 @@
>>   #include <linux/page_counter.h>
>>   #include <linux/atomic.h>
>>   #include <linux/kernel.h>
>> +#include <linux/math64.h>
>>   #include <linux/string.h>
>>   #include <linux/sched.h>
>>   #include <linux/bug.h>
>> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
>>   	 * otherwise get a smaller chunk than what they claimed.
>>   	 */
>>   	if (siblings_protected > parent_effective)
>> -		return protected * parent_effective / siblings_protected;
>> +		return mul_u64_u64_div_u64(protected, parent_effective,
>> +					   siblings_protected);
> 
> On 32bit it is only necessary to use a 64bit intermediary.
> mul_u64_u64_div_u64() will drop back to the (probably faster) 64 by 64
> divide (and then maybe to a 64 by 32 one).
> But there is a lot of extra code before that happens.
> 

Hi David,

Thank you for your review.

Do you want me to split this for 32-bit, or is keeping the single
mul_u64_u64_div_u64() call for readability fine with you?

>>   
>>   	/*
>>   	 * Ok, utilized protection of all children is within what the
>> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
>>   	if (parent_effective > siblings_protected &&
>>   	    parent_usage > siblings_protected &&
>>   	    usage > protected) {
>> -		unsigned long unclaimed;
>> +		unsigned long unclaimed = parent_effective - siblings_protected;
>> +		unsigned long unprotected = usage - protected;
>> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
>>   
>> -		unclaimed = parent_effective - siblings_protected;
>> -		unclaimed *= usage - protected;
>> -		unclaimed /= parent_usage - siblings_protected;
>> +		/*
>> +		 * The usages aren't read atomically, so a child can transiently
>> +		 * appear to use more than its parent, making the ratio exceed 1
>> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
>> +		 */
>> +		unprotected = min(unprotected, parent_unprotected);
>>   
>> -		ep += unclaimed;
>> +		ep += mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected);
> 
> If the ratio is forced to 1 there is no point doing the scaling.
> So maybe:
> 		if (likely(parent_unprotected > unprotected))
> 			unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected,
> 							parent_unprotected);
> 		ep += unclaimed;
> OTOH if the min() generates a cmov rather than a conditional branch
> then you don't get a statically mispredicted branch in the normal case
> (which is very likely with the empty 'else' branch).
> 

I'd rather keep it as-is and avoid the extra branch. Does that work for you?

> 
>>   	}
>>   
>>   	return ep;
> 

-- 
Best regards
Ridong

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[David Laight]

On Sun, 6 Sep 2026 09:11:24 +0800
Ridong Chen <ridong.chen@linux.dev> wrote:

> On 9/4/2026 4:37 PM, David Laight wrote:
> > On Thu,  3 Sep 2026 11:19:51 +0800
> > Ridong Chen <ridong.chen@linux.dev> wrote:
> >   
> >> From: Ridong Chen <chenridong@xiaomi.com>
> >>
> >> effective_protection() scales a parent's protection by a ratio of page
> >> counts, e.g. for recursive protection:
> >>
> >> 	(parent_effective - siblings_protected) * (usage - protected)
> >> 		/ (parent_usage - siblings_protected)
> >>
> >> The multiply is done at unsigned long width before dividing. On systems
> >> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
> >> protection value and silently breaking memory.min/low enforcement.
> >>
> >> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
> >> usage and parent_usage are not read atomically (a child is charged
> >> before its parent), usage - protected can briefly exceed the divisor,
> >> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
> >> the ratio stays <= 1.
> >>
> >> Reported by the sashiko review tool [1].
> >>
> >> [1] https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev?part=1
> >>
> >> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
> >> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
> >> Cc: stable@vger.kernel.org
> >> Assisted-by: Claude:claude-opus-4-8
> >> Reviewed-by: Barry Song <baohua@kernel.org>
> >> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
> >> ---
> >>   mm/page_counter.c | 19 +++++++++++++------
> >>   1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/mm/page_counter.c b/mm/page_counter.c
> >> index 661e0f2a5127..e8bd512069c5 100644
> >> --- a/mm/page_counter.c
> >> +++ b/mm/page_counter.c
> >> @@ -8,6 +8,7 @@
> >>   #include <linux/page_counter.h>
> >>   #include <linux/atomic.h>
> >>   #include <linux/kernel.h>
> >> +#include <linux/math64.h>
> >>   #include <linux/string.h>
> >>   #include <linux/sched.h>
> >>   #include <linux/bug.h>
> >> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
> >>   	 * otherwise get a smaller chunk than what they claimed.
> >>   	 */
> >>   	if (siblings_protected > parent_effective)
> >> -		return protected * parent_effective / siblings_protected;
> >> +		return mul_u64_u64_div_u64(protected, parent_effective,
> >> +					   siblings_protected);  
> > 
> > On 32bit it is only necessary to use a 64bit intermediary.
> > mul_u64_u64_div_u64() will drop back to the (probably faster) 64 by 64
> > divide (and then maybe to a 64 by 32 one).
> > But there is a lot of extra code before that happens.
> >   
> 
> Hi David,
> 
> Thank you for your review.
> 
> Do you want me to split this for 32-bit, or is keeping the single
> mul_u64_u64_div_u64() call for readability fine with you?

Maybe I'll look at wrapping mul_u64_add_u64_div_u64() with something that
checks statically_true(((a) | (b) | (c) | (d)) < 1ull << 32).

Probably neater than the conditional here.

> 
> >>   
> >>   	/*
> >>   	 * Ok, utilized protection of all children is within what the
> >> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
> >>   	if (parent_effective > siblings_protected &&
> >>   	    parent_usage > siblings_protected &&
> >>   	    usage > protected) {
> >> -		unsigned long unclaimed;
> >> +		unsigned long unclaimed = parent_effective - siblings_protected;
> >> +		unsigned long unprotected = usage - protected;
> >> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
> >>   
> >> -		unclaimed = parent_effective - siblings_protected;
> >> -		unclaimed *= usage - protected;
> >> -		unclaimed /= parent_usage - siblings_protected;
> >> +		/*
> >> +		 * The usages aren't read atomically, so a child can transiently
> >> +		 * appear to use more than its parent, making the ratio exceed 1
> >> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
> >> +		 */
> >> +		unprotected = min(unprotected, parent_unprotected);
> >>   
> >> -		ep += unclaimed;
> >> +		ep += mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected);  
> > 
> > If the ratio is forced to 1 there is no point doing the scaling.
> > So maybe:
> > 		if (likely(parent_unprotected > unprotected))
> > 			unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected,
> > 							parent_unprotected);
> > 		ep += unclaimed;
> > OTOH if the min() generates a cmov rather than a conditional branch
> > then you don't get a statically mispredicted branch in the normal case
> > (which is very likely with the empty 'else' branch).
> >   
> 
> I'd rather keep it as-is and avoid the extra branch. Does that work for you?

I nearly came to that conclusion while writing the comment!
Actually the same might go for the other three tests - generating a 0 or 1
from a conditional move ought to be better than the possibly mispredicted
very unlikely branch.
(I am right in thinking that they should never fail?)

I also suspect the #UD is the least of the problems.
You don't want the scaling to produce a bigger number at all.

David

> 
> >   
> >>   	}
> >>   
> >>   	return ep;  
> >   
> 

Re: [PATCH v3 1/2] mm/page_counter: avoid integer overflow in effective_protection()

[Ridong Chen]

On 9/6/2026 6:16 PM, David Laight wrote:
> On Sun, 6 Sep 2026 09:11:24 +0800
> Ridong Chen<ridong.chen@linux.dev> wrote:
> 
>> On 9/4/2026 4:37 PM, David Laight wrote:
>>> On Thu,  3 Sep 2026 11:19:51 +0800
>>> Ridong Chen<ridong.chen@linux.dev> wrote:
>>>    
>>>> From: Ridong Chen<chenridong@xiaomi.com>
>>>>
>>>> effective_protection() scales a parent's protection by a ratio of page
>>>> counts, e.g. for recursive protection:
>>>>
>>>> 	(parent_effective - siblings_protected) * (usage - protected)
>>>> 		/ (parent_usage - siblings_protected)
>>>>
>>>> The multiply is done at unsigned long width before dividing. On systems
>>>> with >= 16TB RAM the product can exceed 2^64 and wrap, giving a bogus
>>>> protection value and silently breaking memory.min/low enforcement.
>>>>
>>>> Use mul_u64_u64_div_u64() to multiply in a 128-bit intermediate. Because
>>>> usage and parent_usage are not read atomically (a child is charged
>>>> before its parent), usage - protected can briefly exceed the divisor,
>>>> making the quotient overflow 64 bits and trap (#DE on x86). Cap it so
>>>> the ratio stays <= 1.
>>>>
>>>> Reported by the sashiko review tool [1].
>>>>
>>>> [1]https://sashiko.dev/#/patchset/20260826133054.88529-1-ridong.chen@linux.dev? 
>>>> part=1
>>>>
>>>> Fixes: bc50bcc6e00b ("mm: memcontrol: clean up and document effective low/min calculations")
>>>> Fixes: 8a931f801340 ("mm: memcontrol: recursive memory.low protection")
>>>> Cc:stable@vger.kernel.org
>>>> Assisted-by: Claude:claude-opus-4-8
>>>> Reviewed-by: Barry Song<baohua@kernel.org>
>>>> Signed-off-by: Ridong Chen<chenridong@xiaomi.com>
>>>> ---
>>>>    mm/page_counter.c | 19 +++++++++++++------
>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/mm/page_counter.c b/mm/page_counter.c
>>>> index 661e0f2a5127..e8bd512069c5 100644
>>>> --- a/mm/page_counter.c
>>>> +++ b/mm/page_counter.c
>>>> @@ -8,6 +8,7 @@
>>>>    #include <linux/page_counter.h>
>>>>    #include <linux/atomic.h>
>>>>    #include <linux/kernel.h>
>>>> +#include <linux/math64.h>
>>>>    #include <linux/string.h>
>>>>    #include <linux/sched.h>
>>>>    #include <linux/bug.h>
>>>> @@ -356,7 +357,8 @@ static unsigned long effective_protection(unsigned long usage,
>>>>    	 * otherwise get a smaller chunk than what they claimed.
>>>>    	 */
>>>>    	if (siblings_protected > parent_effective)
>>>> -		return protected * parent_effective / siblings_protected;
>>>> +		return mul_u64_u64_div_u64(protected, parent_effective,
>>>> +					   siblings_protected);
>>> On 32bit it is only necessary to use a 64bit intermediary.
>>> mul_u64_u64_div_u64() will drop back to the (probably faster) 64 by 64
>>> divide (and then maybe to a 64 by 32 one).
>>> But there is a lot of extra code before that happens.
>>>    
>> Hi David,
>>
>> Thank you for your review.
>>
>> Do you want me to split this for 32-bit, or is keeping the single
>> mul_u64_u64_div_u64() call for readability fine with you?
> Maybe I'll look at wrapping mul_u64_add_u64_div_u64() with something that
> checks statically_true(((a) | (b) | (c) | (d)) < 1ull << 32).
> 
> Probably neater than the conditional here.
> 

Agreed. Other callers face the same 32-bit concern, so a wrapper that does the 
statically_true(...) check once would be cleaner than open-coding a conditional 
in each site.

>>>>    
>>>>    	/*
>>>>    	 * Ok, utilized protection of all children is within what the
>>>> @@ -397,13 +399,18 @@ static unsigned long effective_protection(unsigned long usage,
>>>>    	if (parent_effective > siblings_protected &&
>>>>    	    parent_usage > siblings_protected &&
>>>>    	    usage > protected) {
>>>> -		unsigned long unclaimed;
>>>> +		unsigned long unclaimed = parent_effective - siblings_protected;
>>>> +		unsigned long unprotected = usage - protected;
>>>> +		unsigned long parent_unprotected = parent_usage - siblings_protected;
>>>>    
>>>> -		unclaimed = parent_effective - siblings_protected;
>>>> -		unclaimed *= usage - protected;
>>>> -		unclaimed /= parent_usage - siblings_protected;
>>>> +		/*
>>>> +		 * The usages aren't read atomically, so a child can transiently
>>>> +		 * appear to use more than its parent, making the ratio exceed 1
>>>> +		 * and the quotient overflow 64 bits (#DE on x86).  Cap it.
>>>> +		 */
>>>> +		unprotected = min(unprotected, parent_unprotected);
>>>>    
>>>> -		ep += unclaimed;
>>>> +		ep += mul_u64_u64_div_u64(unclaimed, unprotected, parent_unprotected);
>>> If the ratio is forced to 1 there is no point doing the scaling.
>>> So maybe:
>>> 		if (likely(parent_unprotected > unprotected))
>>> 			unclaimed = mul_u64_u64_div_u64(unclaimed, unprotected,
>>> 							parent_unprotected);
>>> 		ep += unclaimed;
>>> OTOH if the min() generates a cmov rather than a conditional branch
>>> then you don't get a statically mispredicted branch in the normal case
>>> (which is very likely with the empty 'else' branch).
>>>    
>> I'd rather keep it as-is and avoid the extra branch. Does that work for you?
> I nearly came to that conclusion while writing the comment!
> Actually the same might go for the other three tests - generating a 0 or 1
> from a conditional move ought to be better than the possibly mispredicted
> very unlikely branch.
> (I am right in thinking that they should never fail?)
> 

No — these three checks fail all the time in normal operation, so I'd
keep the branch:

if (parent_effective > siblings_protected &&
	parent_usage > siblings_protected &&
	usage > protected) {

Checks 1 and 3 just ask "is there anything left to distribute?". Often
there isn't, so they fail routinely.

Check 2 is a divide-by-zero guard. parent_usage - siblings_protected is
the divisor. A conditional-move can't replace it: the division would run
anyway and only get multiplied by zero afterwards, so we'd still divide
by zero when the two are equal. The branch is what stops us reaching the
division.

> I also suspect the #UD is the least of the problems.
> You don't want the scaling to produce a bigger number at all.
> 

Agreed, and the min() already ensures that. It caps the value so the
result can never be larger than unclaimed. Avoiding the trap is just a
bonus of that same cap.

So I'll keep the patch as-is. Thanks for the review!

-- 
Best regards
Ridong

[PATCH v3 2/2] mm/mglru: fix ineffective memory protection for non-kswapd reclaim

[Ridong Chen]

From: Ridong Chen <chenridong@xiaomi.com>

For MGLRU, memory.min/low is not honored during global proactive reclaim
(writing to the root memory.reclaim) and global direct reclaim, because
these paths shrink memcgs using stale or effective protection (emin/elow).
It can be reproduced as follows:

  # echo 7 > /sys/kernel/mm/lru_gen/enabled
  # cd /sys/fs/cgroup
  # mkdir -p a/b
  # echo 100M > a/memory.min
  # echo +memory > a/cgroup.subtree_control
  # echo 100M > a/b/memory.min
  # echo $$ > a/b/cgroup.procs
  # dd if=/dev/zero of=/tmp/testfile bs=1M count=200
  # cat a/b/memory.current
  222650368
  # echo 500M > memory.reclaim
  -bash: echo: write error: Resource temporarily unavailable
  # cat a/b/memory.current
  6070272

memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the
protection. The traditional LRU path is not affected because
shrink_node() calls mem_cgroup_calculate_protection() for each memcg it
visits during a top-down tree walk.

Commit 30d77b7eef01 ("mm/mglru: fix ineffective protection calculation")
moved the protection computation into lru_gen_age_node(), which only
runs for kswapd. Non-kswapd global reclaim reaches shrink_one() through
lru_gen_shrink_node() -> shrink_many() without any protection
computation, so emin/elow are whatever a previous kswapd run left behind
- or zero if kswapd never ran on this node. Relying on a prior kswapd
pass is not correct either: a memcg's emin/elow are derived from its
ancestors' memory.min/low settings and from children_min_usage, both of
which change over time, so emin/elow go stale even after kswapd has run
and must be recomputed at the point of reclaim.

Introduce mem_cgroup_calculate_protection_path() which computes emin/elow
along the root-to-target path only, by iterating through the cgroup
ancestors array top-down. This avoids the full tree traversal that would
be needed with mem_cgroup_calculate_protection(), limiting the cost to
O(depth) per memcg - typically 3-5 levels.

Call it from shrink_one() for the non-kswapd path so that each memcg
about to be shrunk has correct protection values.

Fixes: e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
---
 include/linux/memcontrol.h | 10 +++++++++
 mm/memcontrol.c            | 45 ++++++++++++++++++++++++++++++++++++++
 mm/vmscan.c                |  8 ++++++-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index f227348a3f24..a65a516adc66 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1885,6 +1885,16 @@ static inline bool memcg_is_dying(struct mem_cgroup *memcg)
 }
 #endif /* CONFIG_MEMCG */
 
+#if defined(CONFIG_MEMCG) && defined(CONFIG_LRU_GEN)
+void mem_cgroup_calculate_protection_path(struct mem_cgroup *root,
+					  struct mem_cgroup *memcg);
+#else
+static inline void mem_cgroup_calculate_protection_path(struct mem_cgroup *root,
+							struct mem_cgroup *memcg)
+{
+}
+#endif
+
 #if defined(CONFIG_MEMCG) && defined(CONFIG_ZSWAP)
 bool obj_cgroup_may_zswap(struct obj_cgroup *objcg);
 void obj_cgroup_charge_zswap(struct obj_cgroup *objcg, size_t size);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 256b68ffca70..ae568fc68813 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5214,6 +5214,51 @@ void mem_cgroup_calculate_protection(struct mem_cgroup *root,
 	page_counter_calculate_protection(&root->memory, &memcg->memory, recursive_protection);
 }
 
+#ifdef CONFIG_LRU_GEN
+/**
+ * mem_cgroup_calculate_protection_path - compute protection along a path
+ * @root: the top ancestor of the sub-tree being checked (NULL for root_mem_cgroup)
+ * @memcg: the target memory cgroup
+ *
+ * Walk the ancestor path from @root down to @memcg and compute the effective
+ * protection at each level.  This is safe for isolated queries because it
+ * ensures parents are computed before children.
+ */
+void mem_cgroup_calculate_protection_path(struct mem_cgroup *root,
+					  struct mem_cgroup *memcg)
+{
+	bool recursive_protection =
+		cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_RECURSIVE_PROT;
+	struct cgroup *cg;
+	int root_level, i;
+
+	if (mem_cgroup_disabled())
+		return;
+
+	if (!root)
+		root = root_mem_cgroup;
+
+	if (memcg == root)
+		return;
+
+	root_level = root->css.cgroup->level;
+	cg = memcg->css.cgroup;
+
+	rcu_read_lock();
+	for (i = root_level + 1; i <= cg->level; i++) {
+		struct mem_cgroup *cur;
+
+		cur = mem_cgroup_from_css(cgroup_css(cg->ancestors[i],
+						     &memory_cgrp_subsys));
+		if (cur)
+			page_counter_calculate_protection(&root->memory,
+							  &cur->memory,
+							  recursive_protection);
+	}
+	rcu_read_unlock();
+}
+#endif /* CONFIG_LRU_GEN */
+
 static int charge_memcg(struct folio *folio, struct mem_cgroup *memcg,
 			gfp_t gfp)
 {
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b4c9b8f3dfe9..500cc2051d13 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -5111,7 +5111,13 @@ static int shrink_one(struct lruvec *lruvec, struct scan_control *sc)
 	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 
-	/* lru_gen_age_node() called mem_cgroup_calculate_protection() */
+	/*
+	 * For kswapd, lru_gen_age_node() has already called
+	 * mem_cgroup_calculate_protection()
+	 */
+	if (!current_is_kswapd())
+		mem_cgroup_calculate_protection_path(NULL, memcg);
+
 	if (mem_cgroup_below_min(NULL, memcg))
 		return MEMCG_LRU_YOUNG;
 
-- 
2.34.1

Re: [PATCH v3 2/2] mm/mglru: fix ineffective memory protection for non-kswapd reclaim

[Barry Song]

On Thu, Sep 3, 2026 at 11:20 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>
> From: Ridong Chen <chenridong@xiaomi.com>
>
> For MGLRU, memory.min/low is not honored during global proactive reclaim
> (writing to the root memory.reclaim) and global direct reclaim, because
> these paths shrink memcgs using stale or effective protection (emin/elow).

Are you sure it should be “stale or effective”?
My gut feeling is that we don't need the “or” here.

[...]
>
> Fixes: e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
> ---

With a few nits:

Reviewed-by: Barry Song <baohua@kernel.org>

[...]
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index b4c9b8f3dfe9..500cc2051d13 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -5111,7 +5111,13 @@ static int shrink_one(struct lruvec *lruvec, struct scan_control *sc)
>         struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>         struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>
> -       /* lru_gen_age_node() called mem_cgroup_calculate_protection() */
> +       /*
> +        * For kswapd, lru_gen_age_node() has already called
> +        * mem_cgroup_calculate_protection()
> +        */

Could be clearer:

/*
 * For kswapd, mem_cgroup_calculate_protection() has already
 * been called during the top-down cgroup traversal.
 */

> +       if (!current_is_kswapd())
> +               mem_cgroup_calculate_protection_path(NULL, memcg);

Best Regards
Barry

Re: [PATCH v3 2/2] mm/mglru: fix ineffective memory protection for non-kswapd reclaim

[Ridong Chen]

On 9/3/2026 2:30 PM, Barry Song wrote:
> On Thu, Sep 3, 2026 at 11:20 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>>
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> For MGLRU, memory.min/low is not honored during global proactive reclaim
>> (writing to the root memory.reclaim) and global direct reclaim, because
>> these paths shrink memcgs using stale or effective protection (emin/elow).
> 
> Are you sure it should be “stale or effective”?
> My gut feeling is that we don't need the “or” here.
> 

My bad. It was a typo. Will remove "or".

> [...]
>>
>> Fixes: e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")
>> Cc: stable@vger.kernel.org
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>
>> ---
> 
> With a few nits:
> 
> Reviewed-by: Barry Song <baohua@kernel.org>
> 
> [...]
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index b4c9b8f3dfe9..500cc2051d13 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -5111,7 +5111,13 @@ static int shrink_one(struct lruvec *lruvec, struct scan_control *sc)
>>          struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>          struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>
>> -       /* lru_gen_age_node() called mem_cgroup_calculate_protection() */
>> +       /*
>> +        * For kswapd, lru_gen_age_node() has already called
>> +        * mem_cgroup_calculate_protection()
>> +        */
> 
> Could be clearer:
> 
> /*
>   * For kswapd, mem_cgroup_calculate_protection() has already
>   * been called during the top-down cgroup traversal.
>   */
> 

Thank you, will update.

>> +       if (!current_is_kswapd())
>> +               mem_cgroup_calculate_protection_path(NULL, memcg);
> 
> Best Regards
> Barry

-- 
Best regards
Ridong

Re: [PATCH v3 2/2] mm/mglru: fix ineffective memory protection for non-kswapd reclaim

[Johannes Weiner]

On Thu, Sep 03, 2026 at 11:19:52AM +0800, Ridong Chen wrote:
> From: Ridong Chen <chenridong@xiaomi.com>
> 
> For MGLRU, memory.min/low is not honored during global proactive reclaim
> (writing to the root memory.reclaim) and global direct reclaim, because
> these paths shrink memcgs using stale or effective protection (emin/elow).
> It can be reproduced as follows:
> 
>   # echo 7 > /sys/kernel/mm/lru_gen/enabled
>   # cd /sys/fs/cgroup
>   # mkdir -p a/b
>   # echo 100M > a/memory.min
>   # echo +memory > a/cgroup.subtree_control
>   # echo 100M > a/b/memory.min
>   # echo $$ > a/b/cgroup.procs
>   # dd if=/dev/zero of=/tmp/testfile bs=1M count=200
>   # cat a/b/memory.current
>   222650368
>   # echo 500M > memory.reclaim
>   -bash: echo: write error: Resource temporarily unavailable
>   # cat a/b/memory.current
>   6070272
> 
> memory.min is 100M, yet reclaim drops a/b down to 6M, breaking the
> protection. The traditional LRU path is not affected because
> shrink_node() calls mem_cgroup_calculate_protection() for each memcg it
> visits during a top-down tree walk.
> 
> Commit 30d77b7eef01 ("mm/mglru: fix ineffective protection calculation")
> moved the protection computation into lru_gen_age_node(), which only
> runs for kswapd. Non-kswapd global reclaim reaches shrink_one() through
> lru_gen_shrink_node() -> shrink_many() without any protection
> computation, so emin/elow are whatever a previous kswapd run left behind
> - or zero if kswapd never ran on this node. Relying on a prior kswapd
> pass is not correct either: a memcg's emin/elow are derived from its
> ancestors' memory.min/low settings and from children_min_usage, both of
> which change over time, so emin/elow go stale even after kswapd has run
> and must be recomputed at the point of reclaim.
> 
> Introduce mem_cgroup_calculate_protection_path() which computes emin/elow
> along the root-to-target path only, by iterating through the cgroup
> ancestors array top-down. This avoids the full tree traversal that would
> be needed with mem_cgroup_calculate_protection(), limiting the cost to
> O(depth) per memcg - typically 3-5 levels.
> 
> Call it from shrink_one() for the non-kswapd path so that each memcg
> about to be shrunk has correct protection values.
> 
> Fixes: e4dde56cd208 ("mm: multi-gen LRU: per-node lru_gen_folio lists")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <chenridong@xiaomi.com>

With Barry's two points of feedback (changelog and comment),

Reviewed-by: Johannes Weiner <hannes@cmpxchg.org>