Re: R11 is invalid with LLVM 12 and later

[Yonghong Song <yhs@fb.com>]

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