Daniel Borkmann writes:
> On 05/03/2019 12:42 PM, Jiong Wang wrote:
>> BPF helper call transfers execution from eBPF insns to native functions
>> while verifier insn walker only walks eBPF insns. So, verifier can only
>> knows argument and return value types from explicit helper function
>> prototype descriptions.
>>
>> For 32-bit optimization, it is important to know whether argument (register
>> use from eBPF insn) and return value (register define from external
>> function) is 32-bit or 64-bit, so corresponding registers could be
>> zero-extended correctly.
>>
>> For arguments, they are register uses, we conservatively treat all of them
>> as 64-bit at default, while the following new bpf_arg_type are added so we
>> could start to mark those frequently used helper functions with more
>> accurate argument type.
>>
>> ARG_CONST_SIZE32
>> ARG_CONST_SIZE32_OR_ZERO
>
> For the above two, I was wondering is there a case where the passed size is
> not used as 32 bit aka couldn't we generally assume 32 bit here w/o adding
> these two extra arg types?
Will give a detailed reply tomorrow. IIRC there was. I was benchmarking
bpf_lxc and found it contains quite a few helper calls which generates a
fairly percentage of unnecessary zext on parameters.
> For ARG_ANYTHING32 and RET_INTEGER64 definitely
> makes sense (btw, opt-in value like RET_INTEGER32 might have been easier for
> reviewing converted helpers).
>
>> ARG_ANYTHING32
>>
>> A few helper functions shown up frequently inside Cilium bpf program are
>> updated using these new types.
>>
>> For return values, they are register defs, we need to know accurate width
>> for correct zero extensions. Given most of the helper functions returning
>> integers return 32-bit value, a new RET_INTEGER64 is added to make those
>> functions return 64-bit value. All related helper functions are updated.
>>
>> Signed-off-by: Jiong Wang <jiong.wang@xxxxxxxxxxxxx>
> [...]
>
>> @@ -2003,9 +2003,9 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
>> .pkt_access = true,
>> .ret_type = RET_INTEGER,
>> .arg1_type = ARG_PTR_TO_MEM_OR_NULL,
>> - .arg2_type = ARG_CONST_SIZE_OR_ZERO,
>> + .arg2_type = ARG_CONST_SIZE32_OR_ZERO,
>> .arg3_type = ARG_PTR_TO_MEM_OR_NULL,
>> - .arg4_type = ARG_CONST_SIZE_OR_ZERO,
>> + .arg4_type = ARG_CONST_SIZE32_OR_ZERO,
>> .arg5_type = ARG_ANYTHING,
>> };
>
> I noticed that the above and also bpf_csum_update() would need to be converted
> to RET_INTEGER64 as they would break otherwise: it's returning error but also
> u32 csum value, so use for error checking would be s64 ret =
> bpf_csum_xyz(...).
Ack.
(I did searched ^u64 inside upai header, should also search ^s64, will
double-check all changes)
>
> Thanks,
> Daniel
