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

[Julian Anastasov <ja@xxxxxx>]

        Hello,

On Sat, 14 Dec 2024, David Laight wrote:

> The 'max_avail' value is calculated from the system memory
> size using order_base_2().
> order_base_2(x) is defined as '(x) ? fn(x) : 0'.
> The compiler generates two copies of the code that follows
> and then expands clamp(max, min, PAGE_SHIFT - 12) (11 on 32bit).
> This triggers a compile-time assert since min is 5.

        8 ?

> 
> In reality a system would have to have less than 512MB memory
> for the bounds passed to clamp to be reversed.
> 
> Swap the order of the arguments to clamp() to avoid the warning.
> 
> Replace the clamp_val() on the line below with clamp().
> clamp_val() is just 'an accident waiting to happen' and not needed here.
> 
> Detected by compile time checks added to clamp(), specifically:
> minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> 
> Reported-by: Linux Kernel Functional Testing <lkft@xxxxxxxxxx>
> Closes: 
> https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@xxxxxxxxxxxxxx/
> Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table")
> Tested-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> Signed-off-by: David Laight <david.laight@xxxxxxxxxx>

        Looks good to me, thanks to everyone!

Acked-by: Julian Anastasov <ja@xxxxxx>

        Pablo, Simon, probably, this should be applied
to the 'nf' tree as it fixes a build failure...

> ---
> 
> Julian seems to be waiting for a 'v2' from me.
> Changed target tree to 'net-next'.
> I've re-written the commit message.
> Copied Andrew Morton - he might want to take the change through the 'mm' tree.
> Plausibly the 'fixes' tag should refer to the minmax.h change?
> This will need back-porting if the minmax set get back-ported.
> 
> I'm not sure whether there ought to be an attribution to Dan Carpenter 
> <dan.carpenter@xxxxxxxxxx>
> 
>  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));
> -     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@xxxxxx>