Re: [PATCH bpf-next 2/3] bpf: Perform necessary sign/zero extension for kfunc return values

[Yonghong Song <yhs@fb.com>]

On 8/8/22 4:25 PM, Andrii Nakryiko wrote:
> On Sun, Aug 7, 2022 at 10:51 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Tejun reported a bpf program kfunc return value mis-handling which
>> may cause incorrect result. The following is an example to show
>> the problem.
>>    $ cat t.c
>>    unsigned char bar();
>>    int foo() {
>>          if (bar() != 10) return 0; else return 1;
>>    }
>>    $ clang -target bpf -O2 -c t.c
>>    $ llvm-objdump -d t.o
>>    ...
>>    0000000000000000 <foo>:
>>         0:       85 10 00 00 ff ff ff ff call -1
>>         1:       bf 01 00 00 00 00 00 00 r1 = r0
>>         2:       b7 00 00 00 01 00 00 00 r0 = 1
>>         3:       15 01 01 00 0a 00 00 00 if r1 == 10 goto +1 <LBB0_2>
>>         4:       b7 00 00 00 00 00 00 00 r0 = 0
>>
>>    0000000000000028 <LBB0_2>:
>>         5:       95 00 00 00 00 00 00 00 exit
>>    $
>>
>> In the above example, the return type for bar() is 'unsigned char'.
>> But in the disassembly code, the whole register 'r1' is used to
>> compare to 10 without truncating upper 56 bits.
>>
>> If function bar() is implemented as a bpf function, everything
>> should be okay since bpf ABI will make sure the caller do
>> proper truncation of upper 56 bits.
>>
>> But if function bar() is implemented as a non-bpf kfunc,
>> there could a mismatch between bar() implementation and bpf program.
>> For example, if the host arch is x86_64, the bar() function
>> may just put the return value in lower 8-bit subregister and all
>> upper 56 bits could contain garbage. This is not a problem
>> if bar() is called in x86_64 context as the caller will use
>> %al to get the value.
>>
>> But this could be a problem if bar() is called in bpf context
>> and there is a mismatch expectation between bpf and native architecture.
>> Currently, bpf programs use the default llvm ABI ([1], function
>> isPromotableIntegerTypeForABI()) such that if an integer type size
>> is less than int type size, it is assumed proper sign or zero
>> extension has been done to the return value. There will be a problem
>> if the kfunc return value type is u8/s8/u16/s16.
> 
> Reading this I was still confused how (and whether) s32/u32 returns
> are going to be handled correctly, especially on non-cpuv3 BPF object
> code. So I played with this a bit and Clang does generate explicit <<
> and >>/>>= shifts as expected. It might be worth it emphasizing that
> for 32-bit returns Clang will generate explicit shifts?

Yes, I can reword the commit message to emphasize 32-bit return
value won't be an issue.

> 
>>
>> This patch intends to address this issue by doing proper sign or zero
>> extension for the kfunc return value before it is used later.
>>
>>   [1] https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp
>>
>> Reported-by: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/bpf.h   |  2 ++
>>   kernel/bpf/btf.c      |  9 +++++++++
>>   kernel/bpf/verifier.c | 35 +++++++++++++++++++++++++++++++++--
>>   3 files changed, 44 insertions(+), 2 deletions(-)
>>
> 
> [...]