On 8/11/21 9:54 AM, Paul Chaignon wrote:
On Mon, Aug 09, 2021 at 11:31:48PM -0700, Yonghong Song wrote:
On 8/9/21 3:53 PM, Yonghong Song wrote:
On 8/9/21 8:12 AM, Paul Chaignon wrote:
[...]
LLVM 12.0.1 and latest LLVM sources (e.g., commit 2b4a1d4b from today)
have the same issue. We've bisected it to LLVM commit 552c6c23
("PR44406: Follow behavior of array bound constant folding in more
recent versions of GCC."), but that could just be the commit where
the regression was exposed in Cilium's case.
The above commit is indeed responsible. With the above commit,
the variable length array compile time evaluation becomes conservative.
For cilium function dsr_reply_icmp4 in nodeport.h
const __u32 l3_max = MAX_IPOPTLEN + sizeof(*ip4) + orig_dgram;
__u8 tmp[l3_max];
I didn't try to compile with/without the above commit, the following
is the thesis.
Before the above commit, llvm evaluates the expression
MAX_IPOPTLEN + sizeof(*ip4) + orig_dgram
and concludes that l3_max is a constant so tmp can be allocated
on stack with fixed size.
With the above commit, llvm becomes conservative to evaluate
the expression at compile time. So above l3_max becomes a
non-constant variable and tmp becomes a variable length
array. Since it becomes a variable length array, the
hidden stack pointer "r11" is used and this caused a problem
in verifier.
To workaround the issue, simply define "tmp" with
__u8 tmp[68];
Thanks Yonghong! I've tested this workaround on the Cilium codebase
with all of our tested configurations. I'm not seeing this R11 issue in
this BPF program or anywhere else anymore.
That is great!
But that is not for from user experience. I guess we can do:
1. for BPF target, let us still do aggressive constant folding
in compile time in clang, basically restores to pre-commit-552c6c23
level for BPF programs.
2. provide an error message if r11 is generated in final code.
Will come back to this thread once I got concrete patches. Thanks!
I'm happy to run any patch you have through the Cilium CI if that helps.
Thanks for testing. Could you help try whether the patch
https://reviews.llvm.org/D107882 can fix the problem or not?
You might need to add -Wno-gnu-folding-constant to silence
array size constant folding warnings.
If this indeed fix the cilium, could you add a comment
to the above llvm patch?
--
Paul
