Hello,

On Fri, 6 Dec 2024, David Laight wrote:

From: Julian Anastasov

quoted

Sent: 06 December 2024 12:19

On Fri, 6 Dec 2024, David Laight wrote:

quoted

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.

quoted

quoted

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.

quoted

quoted

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 [off-list ref]