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: <5ec10e7c-d050-dab8-1f1b-d0ca2d922eef@ssi.bg>
Date: Fri, 6 Dec 2024 14:19:21 +0200 (EET)
From: Julian Anastasov <ja@....bg>
To: David Laight <David.Laight@...LAB.COM>
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.


	Hello,

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...

> 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?

> 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?

> 
> 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.

> -	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>