Re: [PATCH] mm, vmscan: guarantee drop_slab_node() termination

[Vlastimil Babka <vbabka@suse.cz>]

On 8/24/21 12:02, Matthew Wilcox wrote:
> On Wed, Aug 18, 2021 at 05:22:39PM +0200, Vlastimil Babka wrote:
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 403a175a720f..ef3554314b47 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -936,6 +936,7 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid,
>>  void drop_slab_node(int nid)
>>  {
>>  	unsigned long freed;
>> +	int shift = 0;
>>  
>>  	do {
>>  		struct mem_cgroup *memcg = NULL;
>> @@ -948,7 +949,7 @@ void drop_slab_node(int nid)
>>  		do {
>>  			freed += shrink_slab(GFP_KERNEL, nid, memcg, 0);
>>  		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
>> -	} while (freed > 10);
>> +	} while ((freed >> shift++) > 0);
> 
> This can, if you're really unlucky, produce UB.  If you free 2^63 items
> when shift is 63, then 2^63 >> 63 is 1 and shift becomes 64, producing
> UB on the next iteration.  We could do:
> 
> 	} while (shift < BITS_PER_LONG) && (freed >> shift++) > 0);
> 
> but honestly, that feels silly.  How about:
> 
> 	} while ((freed >> shift++) > 1);
> 
> almost exactly as arbitrary, but guarantees no UB.

Hey, zero is not arbitrary :P
But thanks, here's a fix up.

From 88189bf16406c5910400193422b3f18f859f18d8 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Tue, 24 Aug 2021 14:08:53 +0200
Subject: [PATCH] mm, vmscan: guarantee drop_slab_node() termination-fix

Matthew reports [1] that if we free enough objects, we can eventually
right-shift by BITS_PER_LONG, which is undefined behavior. Raise the
threshold from 0 to 1 which means we will shift only up to BITS_PER_LONG-1.

[1] https://lore.kernel.org/linux-mm/YSTDnqKgQLvziyQI@casper.infradead.org/

Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4ffaa7970904..f08aef08c351 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -952,7 +952,7 @@ void drop_slab_node(int nid)
 		do {
 			freed += shrink_slab(GFP_KERNEL, nid, memcg, 0);
 		} while ((memcg = mem_cgroup_iter(NULL, memcg, NULL)) != NULL);
-	} while ((freed >> shift++) > 0);
+	} while ((freed >> shift++) > 1);
 }
 
 void drop_slab(void)
-- 
2.32.0