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

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.

Quoted 28 lines · Dan Carpenter

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)
                     ^^^^^^^^^^^^^

Quoted 1 line

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.

Quoted 34 lines · Alexei Starovoitov

Quoted 25 lines · Dan Carpenter

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

Quoted 7 lines

    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.

Quoted 11 lines

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;