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.

quoted

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,

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

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)