|
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' <horms@xxxxxxxxxx>
> >
> > 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.
|
