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