Re: [PATCH net] ipvs: fix shift-out-of-bounds in ip_vs_rht_desired_size

On Thu, Apr 30, 2026 at 04:10:16PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 30 Apr 2026, Simon Horman wrote:
> 
> > From: 'Simon Horman' <[email protected]>
> > 
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> > ---
> > ipvs: fix shift-out-of-bounds in ip_vs_rht_desired_size
> > 
> > This commit addresses a shift-out-of-bounds issue when roundup_pow_of_two()
> > is called with 0 in ip_vs_rht_desired_size. It updates the assignment to fall
> > back to 1 if n is 0.
> > 
> > > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > > index f5b7a2047291..ea450944465f 100644
> > > --- a/net/netfilter/ipvs/ip_vs_core.c
> > > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > > @@ -237,7 +237,7 @@ int ip_vs_rht_desired_size(struct netns_ipvs *ipvs, struct ip_vs_rht *t, int n,
> > >  {
> > >  	if (!t)
> > >  		return 1 << min_bits;
> > > -	n = roundup_pow_of_two(n);
> > > +	n = n ? roundup_pow_of_two(n) : 1;
> 
> 	To overflow it on 64-bit we need 600GB+ memory, the
> conns are ~300 bytes.
> 
> 	In v2 I changed it to n > 0. To solve it completely
> we may need to introduce some kind of "conn_max" sysctl,
> now per-netns. It can help to limit the conns which are
> created GFP_ATOMIC while for other structures we will switch
> to GFP_KERNEL_ACCOUNT.

Thanks, I agree with this approach.

> 
> > The parameter n is a signed int. If n happens to be negative, it will evaluate
> > to true and be passed to roundup_pow_of_two().
> > 
> > Because roundup_pow_of_two() casts the input to unsigned long, a negative
> > value like -1 becomes ULONG_MAX. This results in a shift exponent of 64,
> > triggering a shift-out-of-bounds regression. Would it be safer to check
> > for n <= 0 instead?
> > 
> > Additionally, if n exceeds 1.07 billion (1 << 30) on systems with large
> > amounts of RAM, roundup_pow_of_two(n) evaluates to 1UL << 31. Since n is
> > a 32-bit signed int, assigning 1UL << 31 back to n overflows and results
> > in a negative number.
> > 
> > Since subsequent bounds checks use signed comparisons:
> > 
> > >  	if (lfactor < 0) {
> > >  		int factor = min(-lfactor, max_bits);
> > >  
> > >  		n = min(n, 1 << (max_bits - factor));
> > 
> > These checks will fail to constrain the size correctly.
> > 
> > [ ... ]
> > 
> > >  	/* Shrink but keep it n * 2 to prevent frequent resizing */
> > >  	return clamp(n << 1, 1 << min_bits, 1 << max_bits);
> > >  }
> > 
> > Eventually, the function returns clamp(n << 1, 1 << min_bits, 1 << max_bits).
> > Shifting a negative number causes undefined behavior, and the clamp operation
> > will force the hash table to its absolute minimum size.
> > 
> > Could this cause millions of connections to be placed into a minimally sized
> > hash table, causing severe collisions during RCU hash lookups?
> 
> 	All that is valid without conn/mem limits.

Sorry, my previous email was supposed to come with some commentary
that I think that this is a pre-existing issue that can be treated
separately from this patch.