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@xxxxxx>
