linux-kernel - RE: [PATCH net] Fix clamp() of ip_vs_conn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2a91ee407ed64d24b82e5fc665971add@AcuMS.aculab.com>
Date: Fri, 6 Dec 2024 13:07:59 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Julian Anastasov' <ja@....bg>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 'Naresh Kamboju'
	<naresh.kamboju@...aro.org>, 'Dan Carpenter' <dan.carpenter@...aro.org>,
	"'pablo@...filter.org'" <pablo@...filter.org>, 'open list'
	<linux-kernel@...r.kernel.org>, "'lkft-triage@...ts.linaro.org'"
	<lkft-triage@...ts.linaro.org>, 'Linux Regressions'
	<regressions@...ts.linux.dev>, 'Linux ARM'
	<linux-arm-kernel@...ts.infradead.org>, "'netfilter-devel@...r.kernel.org'"
	<netfilter-devel@...r.kernel.org>, 'Arnd Bergmann' <arnd@...db.de>, "'Anders
 Roxell'" <anders.roxell@...aro.org>, 'Johannes Berg'
	<johannes.berg@...el.com>, "'toke@...nel.org'" <toke@...nel.org>, 'Al Viro'
	<viro@...iv.linux.org.uk>, "'kernel@...rr.cc'" <kernel@...rr.cc>,
	"'kees@...nel.org'" <kees@...nel.org>
Subject: RE: [PATCH net] Fix clamp() of ip_vs_conn_tab on small memory
 systems.

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@...lab.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@....bg>

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