| Message ID | 33893212b1cc4a418cec09aeeed0a9fc@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] Fix clamp() of ip_vs_conn_tab on small memory systems. | expand |
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@aculab.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@ssi.bg>
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@aculab.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@ssi.bg> - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hello, On Fri, 6 Dec 2024, David Laight wrote: > 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.) I'm not sure how much memory we can see in small system, IMHO, problem should not be possible in practice: - nobody expects 0 from totalram_pages() in the code - order_base_2(sizeof(struct ip_vs_conn)) is probably 8 on 32-bit - PAGE_SHIFT: 12 (for 4KB) or more? So, if totalram_pages() returns below 128 pages (4KB each) max_avail will be below 19 (7 + 12), then 19 is reduced with 2 + 1 and becomes 16, finally with 8 (from the 2nd order_base_2) to reach 16-8=8. You need a system with less than 512KB (19 bits) to trigger problem in clamp() that will lead to max below 8. Further, without checks, for ip_vs_conn_tab_bits=1 we need totalram_pages() to return 0 pages. > > > 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'. I'm confused with these compiler issues, if you think we should go with the patch just decide if it is a net or net-next material. Your change is safer for bad max_avail values but I don't expect to see problem while running without the change, except the building bugs. Also, please use nf/nf-next tag to avoid any confusion with upstreaming... Regards -- Julian Anastasov <ja@ssi.bg>
From: Julian Anastasov > Sent: 06 December 2024 16:23 ... > I'm not sure how much memory we can see in small system, > IMHO, problem should not be possible in practice: > > - nobody expects 0 from totalram_pages() in the code > > - order_base_2(sizeof(struct ip_vs_conn)) is probably 8 on 32-bit It is 0x120 bytes on 64bit, so 8 could well be right. > - PAGE_SHIFT: 12 (for 4KB) or more? > > So, if totalram_pages() returns below 128 pages (4KB each) > max_avail will be below 19 (7 + 12), then 19 is reduced with 2 + 1 > and becomes 16, finally with 8 (from the 2nd order_base_2) to reach > 16-8=8. You need a system with less than 512KB (19 bits) to trigger > problem in clamp() that will lead to max below 8. Which pretty much won't happen, I think my (dead) sun3 has more than that. > Further, without > checks, for ip_vs_conn_tab_bits=1 we need totalram_pages() to return 0 > pages. > > > > > 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'. > > I'm confused with these compiler issues, The compiler is just doing its job. Consider this expression: (x >= 1 ? 2 * x : 1) - 1 It is likely to get converted to: (x >= 1 ? 2 * x - 1 : 0) to avoid the subtract when x < 1. The same thing is happening here. order_base_2() has a (condition ? fn() : 0) in it. All the +/- constants get moved inside, on 64bit that is +12 -2 -1 -9 = 0. Then the clamp() with constants gets moved inside: (condition ? clamp(27, 8, fn() + 0) : clamp(27, 8, 0 + 0)) Now, at runtime, we know that 'condition' is true and (fn() >= 8) so the first clamp() is valid and the second one never used. But this isn't known by the compiler and clamp() detects the invalid call and generates a warning. > if you > think we should go with the patch just decide if it is a > net or net-next material. Your change is safer for bad > max_avail values but I don't expect to see problem while > running without the change, except the building bugs. > > Also, please use nf/nf-next tag to avoid any > confusion with upstreaming... I've copied Andrew M - he's taken the minmax.h change into his mm tree. This is one of the build breakages. It probably only needs to go into next for now (via some route). But I can image the minmax.h changes getting backported a bit. David > > Regards > > -- > Julian Anastasov <ja@ssi.bg> - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hello, On Fri, 6 Dec 2024, David Laight wrote: > From: Julian Anastasov > > Sent: 06 December 2024 16:23 > ... > > I'm not sure how much memory we can see in small system, > > IMHO, problem should not be possible in practice: > > > > - nobody expects 0 from totalram_pages() in the code > > > > - order_base_2(sizeof(struct ip_vs_conn)) is probably 8 on 32-bit > > It is 0x120 bytes on 64bit, so 8 could well be right. That is already for 9 :) > > > 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'. > > > > I'm confused with these compiler issues, > > The compiler is just doing its job. > Consider this expression: > (x >= 1 ? 2 * x : 1) - 1 > It is likely to get converted to: > (x >= 1 ? 2 * x - 1 : 0) > to avoid the subtract when x < 1. > > The same thing is happening here. > order_base_2() has a (condition ? fn() : 0) in it. > All the +/- constants get moved inside, on 64bit that is +12 -2 -1 -9 = 0. > Then the clamp() with constants gets moved inside: > (condition ? clamp(27, 8, fn() + 0) : clamp(27, 8, 0 + 0)) > Now, at runtime, we know that 'condition' is true and (fn() >= 8) > so the first clamp() is valid and the second one never used. > But this isn't known by the compiler and clamp() detects the invalid > call and generates a warning. I see, such optimizations are beyond my expectations, I used max_avail var to separate the operations between different macro calls but in the end they are mixed together... > > if you > > think we should go with the patch just decide if it is a > > net or net-next material. Your change is safer for bad > > max_avail values but I don't expect to see problem while > > running without the change, except the building bugs. > > > > Also, please use nf/nf-next tag to avoid any > > confusion with upstreaming... > > I've copied Andrew M - he's taken the minmax.h change into his mm tree. > This is one of the build breakages. > > It probably only needs to go into next for now (via some route). > But I can image the minmax.h changes getting backported a bit. OK, then can you send v2 with Fixes header, precised comments and nf tag, it fixes a recent commit, so it can be backported easily... 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 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 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. Change to clamp(max_avail, 5, 20) which has the expected behaviour. Replace the clamp_val() on the line below with clamp(). clamp_val() is just 'an accident waiting to happen' and not needed here. 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() Signed-off-by: David Laight <david.laight@aculab.com> --- net/netfilter/ipvs/ip_vs_conn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)