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