RE: [PATCH net] Fix clamp() of ip_vs_conn_tab on small memory systems.

[David Laight <David.Laight@ACULAB.COM>]

From: Julian Anastasov
> Sent: 06 December 2024 12:19
> 
> On Fri, 6 Dec 2024, David Laight wrote:
> 
> > The intention of the code seems to be that the minimum table
> > size should be 256 (1 << min).
> > However the code uses max = clamp(20, 5, max_avail) which implies
> 
> 	Actually, it tries to reduce max=20 (max possible) below
> max_avail: [8 .. max_avail]. Not sure what 5 is here...

Me mistyping values between two windows :-)

Well min(max, max_avail) would be the reduced upper limit.
But you'd still fall foul of the compiler propagating the 'n > 1'
check in order_base_2() further down the function.

> > the author thought max_avail could be less than 5.
> > But clamp(val, min, max) is only well defined for max >= min.
> > If max < min whether is returns min or max depends on the order of
> > the comparisons.
> 
> 	Looks like max_avail goes below 8 ? What value you see
> for such small system?

I'm not, but clearly you thought the value could be small otherwise
the code would only have a 'max' limit.
(Apart from a 'sanity' min of maybe 2 to stop the code breaking.)

> 
> > Change to clamp(max_avail, 5, 20) which has the expected behaviour.
> 
> 	It should be clamp(max_avail, 8, 20)
> 
> >
> > Replace the clamp_val() on the line below with clamp().
> > clamp_val() is just 'an accident waiting to happen' and not needed here.
> 
> 	OK
> 
> > Fixes: 4f325e26277b6
> > (Although I actually doubt the code is used on small memory systems.)
> >
> > Detected by compile time checks added to clamp(), specifically:
> > minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> 
> 	Existing or new check? Does it happen that max_avail
> is a constant, so that a compile check triggers?

Is all stems from order_base_2(totalram_pages()).
order_base_2(n) is 'n > 1 ? ilog2(n - 1) + 1 : 0'.
And the compiler generates two copies of the code that follows
for the 'constant zero' and ilog2() values.
And the 'zero' case compiles clamp(20, 8, 0) which is errored.
Note that it is only executed if totalram_pages() is zero,
but it is always compiled 'just in case'.

> 
> >
> > Signed-off-by: David Laight <david.laight@aculab.com>
> 
> 	The code below looks ok to me but can you change the
> comments above to more correctly specify the values and if the
> problem is that max_avail goes below 8 (min).
> 
> > ---
> >  net/netfilter/ipvs/ip_vs_conn.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> > index 98d7dbe3d787..c0289f83f96d 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> > @@ -1495,8 +1495,8 @@ int __init ip_vs_conn_init(void)
> >  	max_avail -= 2;		/* ~4 in hash row */
> >  	max_avail -= 1;		/* IPVS up to 1/2 of mem */
> >  	max_avail -= order_base_2(sizeof(struct ip_vs_conn));
> 
> 	More likely we can additionally clamp max_avail here:
> 
> 	max_avail = max(min, max_avail);
> 
> 	But your solution solves the problem with less lines.

And less code in the path that is actually executed.

	David

> 
> > -	max = clamp(max, min, max_avail);
> > -	ip_vs_conn_tab_bits = clamp_val(ip_vs_conn_tab_bits, min, max);
> > +	max = clamp(max_avail, min, max);
> > +	ip_vs_conn_tab_bits = clamp(ip_vs_conn_tab_bits, min, max);
> >  	ip_vs_conn_tab_size = 1 << ip_vs_conn_tab_bits;
> >  	ip_vs_conn_tab_mask = ip_vs_conn_tab_size - 1;
> >
> > --
> > 2.17.1
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)