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? 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(...).
Thanks,
Daniel
