On Sun, Aug 7, 2022 at 10:51 AM Yonghong Song <yhs@xxxxxx> 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?
>
> 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@xxxxxxxxxx>
> Signed-off-by: Yonghong Song <yhs@xxxxxx>
> ---
> include/linux/bpf.h | 2 ++
> kernel/bpf/btf.c | 9 +++++++++
> kernel/bpf/verifier.c | 35 +++++++++++++++++++++++++++++++++--
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
[...]
