linux-kernel - RE: [PATCH net] Fix clamp() of ip_vs_conn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <184af1fd-2bc4-11c1-9319-0ca879e53d99@ssi.bg>
Date: Fri, 6 Dec 2024 22:58:13 +0200 (EET)
From: Julian Anastasov <ja@....bg>
To: David Laight <David.Laight@...LAB.COM>
cc: "'Andrew Morton'" <akpm@...ux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "'Naresh Kamboju'" <naresh.kamboju@...aro.org>,
        "'Dan Carpenter'" <dan.carpenter@...aro.org>,
        "'pablo@...filter.org'" <pablo@...filter.org>,
        "'open list'" <linux-kernel@...r.kernel.org>,
        "'lkft-triage@...ts.linaro.org'" <lkft-triage@...ts.linaro.org>,
        "'Linux Regressions'" <regressions@...ts.linux.dev>,
        "'Linux ARM'" <linux-arm-kernel@...ts.infradead.org>,
        "'netfilter-devel@...r.kernel.org'" <netfilter-devel@...r.kernel.org>,
        "'Arnd Bergmann'" <arnd@...db.de>,
        "'Anders Roxell'" <anders.roxell@...aro.org>,
        "'Johannes Berg'" <johannes.berg@...el.com>,
        "'toke@...nel.org'" <toke@...nel.org>,
        "'Al Viro'" <viro@...iv.linux.org.uk>,
        "'kernel@...rr.cc'" <kernel@...rr.cc>,
        "'kees@...nel.org'" <kees@...nel.org>
Subject: RE: [PATCH net] Fix clamp() of ip_vs_conn_tab on small memory
 systems.


	Hello,

On Fri, 6 Dec 2024, David Laight wrote:

> 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.

	That is already for 9 :)

> > > 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.

	I see, such optimizations are beyond my expectations,
I used max_avail var to separate the operations between
different macro calls but in the end they are mixed together...

> > 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.

	OK, then can you send v2 with Fixes header, precised comments
and nf tag, it fixes a recent commit, so it can be backported easily...

Regards

--
Julian Anastasov <ja@....bg>