| Message ID | 24a6bfd0811b4931b6ef40098b33c9ee@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] Fix clamp() of ip_vs_conn_tab on small memory systems. | expand |
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@linaro.org> > Closes: https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/ > Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table") > Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Signed-off-by: David Laight <david.laight@aculab.com> Looks good to me, thanks to everyone! Acked-by: Julian Anastasov <ja@ssi.bg> 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@linaro.org> > > 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@ssi.bg>
Hello, On Tue, 17 Dec 2024, Julian Anastasov wrote: > 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 Also, note that this is 512KB (practically impossible), not 512MB. So, it can fail only on build. Regards -- Julian Anastasov <ja@ssi.bg>
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;
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. 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@linaro.org> Closes: https://lore.kernel.org/all/CA+G9fYsT34UkGFKxus63H6UVpYi5GRZkezT9MRLfAbM3f6ke0g@mail.gmail.com/ Fixes: 4f325e26277b ("ipvs: dynamically limit the connection hash table") Tested-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Signed-off-by: David Laight <david.laight@aculab.com> --- 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@linaro.org> net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)