Re: R11 is invalid with LLVM 12 and later

[Paul Chaignon <paul@cilium.io>]

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