[PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[Dan Carpenter]

We recently added some build time asserts to detect incorrect calls to
clamp and it detected this bug which breaks the build.  The variable
in this clamp is "max_avail" and it should be the first argument.  The
code currently is the equivalent to max = max(max_avail, max).

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Closes: https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/
Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
I've been trying to add stable CC's to my commits but I'm not sure the
netdev policy on this.  Do you prefer to add them yourself?

 net/netfilter/ipvs/ip_vs_conn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 98d7dbe3d787..9f75ac801301 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -1495,7 +1495,7 @@ 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);
+	max = clamp(max_avail, min, max);
 	ip_vs_conn_tab_bits = clamp_val(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.45.2

Re: [PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[Bartosz Golaszewski]

On Wed, Dec 11, 2024 at 2:16 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> We recently added some build time asserts to detect incorrect calls to
> clamp and it detected this bug which breaks the build.  The variable
> in this clamp is "max_avail" and it should be the first argument.  The
> code currently is the equivalent to max = max(max_avail, max).
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Closes: https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/
> Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I've been trying to add stable CC's to my commits but I'm not sure the
> netdev policy on this.  Do you prefer to add them yourself?
>
>  net/netfilter/ipvs/ip_vs_conn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 98d7dbe3d787..9f75ac801301 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1495,7 +1495,7 @@ 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);
> +       max = clamp(max_avail, min, max);
>         ip_vs_conn_tab_bits = clamp_val(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.45.2
>

Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

RE: [PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[David Laight]

From: Dan Carpenter
> Sent: 11 December 2024 13:17
> 
> We recently added some build time asserts to detect incorrect calls to
> clamp and it detected this bug which breaks the build.  The variable
> in this clamp is "max_avail" and it should be the first argument.  The
> code currently is the equivalent to max = max(max_avail, max).

The fix is correct but the description above is wrong.
When run max_avail is always larger than min so the result is correct.
But the compiler does some constant propagation (for something that
can't happen) and wants to calculate the constant 'clamp(max, min, 0)'
Both max and min are known values so the build assert trips.

I posted the same patch (with a different message) last week.

	David

> 
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Closes:
> https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/
> Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> I've been trying to add stable CC's to my commits but I'm not sure the
> netdev policy on this.  Do you prefer to add them yourself?
> 
>  net/netfilter/ipvs/ip_vs_conn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 98d7dbe3d787..9f75ac801301 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1495,7 +1495,7 @@ 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);
> +	max = clamp(max_avail, min, max);
>  	ip_vs_conn_tab_bits = clamp_val(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.45.2

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

Re: [PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[Dan Carpenter]

On Wed, Dec 11, 2024 at 02:27:06PM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 11 December 2024 13:17
> > 
> > We recently added some build time asserts to detect incorrect calls to
> > clamp and it detected this bug which breaks the build.  The variable
> > in this clamp is "max_avail" and it should be the first argument.  The
> > code currently is the equivalent to max = max(max_avail, max).
> 
> The fix is correct but the description above is wrong.

Aw yes, it's max = min(max_avail, max);  I'll resend.

regards,
dan carpenter

RE: [PATCH net] ipvs: Fix clamp() order in ip_vs_conn_init()

[Julian Anastasov]

	Hello,

On Wed, 11 Dec 2024, David Laight wrote:

> From: Dan Carpenter
> > Sent: 11 December 2024 13:17
> > 
> > We recently added some build time asserts to detect incorrect calls to
> > clamp and it detected this bug which breaks the build.  The variable
> > in this clamp is "max_avail" and it should be the first argument.  The
> > code currently is the equivalent to max = max(max_avail, max).
> 
> The fix is correct but the description above is wrong.
> When run max_avail is always larger than min so the result is correct.
> But the compiler does some constant propagation (for something that
> can't happen) and wants to calculate the constant 'clamp(max, min, 0)'
> Both max and min are known values so the build assert trips.
> 
> I posted the same patch (with a different message) last week.

	I was still waiting for v2 from David Laight as
he can put more specific explanation for the bad 3rd arg
to clamp() and to add the Fixes header.

	David, let me know what should we do, I prefer
to see v2 from you but if you prefer we can go with the
latest version from Dan...

> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Closes:
> > https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/
> > Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > I've been trying to add stable CC's to my commits but I'm not sure the
> > netdev policy on this.  Do you prefer to add them yourself?
> > 
> >  net/netfilter/ipvs/ip_vs_conn.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> > index 98d7dbe3d787..9f75ac801301 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> > @@ -1495,7 +1495,7 @@ 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);
> > +	max = clamp(max_avail, min, max);
> >  	ip_vs_conn_tab_bits = clamp_val(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.45.2

Regards

--
Julian Anastasov <ja@ssi.bg>