Re: bpf: allow bpf programs to tail-call other bpf programs

[Alexei Starovoitov <ast@plumgrid.com>]

On 5/26/15 9:16 AM, Daniel Borkmann wrote:
> On 05/26/2015 06:01 PM, Alexei Starovoitov wrote:
>> On 5/23/15 10:33 AM, Dan Carpenter wrote:
>>> Hello Alexei Starovoitov,
>>>
>>> This is a semi-automatic email about new static checker warnings.
>>>
>>> The patch 04fd61ab36ec: "bpf: allow bpf programs to tail-call other
>>> bpf programs" from May 19, 2015, leads to the following Smatch
>>> complaint:
>>>
>>> kernel/bpf/verifier.c:921 check_call()
>>>      error: we previously assumed 'map' could be null (see line 911)
>>>
>>> kernel/bpf/verifier.c
>>>     910
>>>     911        if (map && map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
>>>                      ^^^
>>> Patch introduces a check for NULL.
>>>
>>>     912            func_id != BPF_FUNC_tail_call)
>>>     913            /* prog_array map type needs extra care:
>>>     914             * only allow to pass it into bpf_tail_call() for
>>> now.
>>>     915             * bpf_map_delete_elem() can be allowed in the
>>> future,
>>>     916             * while bpf_map_update_elem() must only be done
>>> via syscall
>>>     917             */
>>>     918            return -EINVAL;
>>>     919
>>>     920        if (func_id = BPF_FUNC_tail_call &&
>>>     921            map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
>>>                      ^^^^^^^^^^^^^
>>> New unchecked dereference.
>>
>> that is false positive.
>> See that 'if (func_id = BPF_FUNC_tail_call ...)' is done before
>> map->map_type dereference.
>> tail_call_proto has:
>> .arg2_type = ARG_CONST_MAP_PTR
>> so call at line 869:
>> check_func_arg(env, BPF_REG_2, fn->arg2_type, &map);
>> will check that arg2 is map_ptr and will assign valid map pointer
>> into map variable.
>> So if line 920 is true, then line 869 was ok as well and map
>> pointer is valid. No need for extra check at line 921.
>
> A bit hard to parse, perhaps. The control flow might have been easier
> to understand, if it would have looked like:
>
> if (!map)
>      /* Nothing to check further. */
>      return 0;
> if (map->map_type = BPF_MAP_TYPE_PROG_ARRAY &&
>      func_id != BPF_FUNC_tail_call)
>      return -EINVAL;
>
> if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY &&
>      func_id = BPF_FUNC_tail_call)
>      return -EINVAL;

not really. map = NULL is valid case.
Currently the existing two 'if' are logically checking two
different conditions. Splitting them into 3 like above
makes me cringe a little, since the checking logic is not
contained. I agree that the above _looks_ nicer, but it
feels wrong to me, since checks are split up this way.