RE: [PATCH net] Fix clamp() of ip_vs_conn_tab on small memory systems.

[David Laight <David.Laight@ACULAB.COM>]

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)