RE: [PATCHv17 bpf-next 5/6] selftests/bpf: Add verifier tests for bpf arg ARG_CONST_MAP_PTR_OR_NULL

John Fastabend
2021-01-27T22:58:36Z

Hangbin Liu wrote:
> Use helper bpf_redirect_map() and bpf_redirect_map_multi() to test bpf
> arg ARG_CONST_MAP_PTR and ARG_CONST_MAP_PTR_OR_NULL. Make sure the
> map arg could be verified correctly when it is NULL or valid map
> pointer.
>
> Add devmap and devmap_hash in struct bpf_test due to bpf_redirect_{map,
> map_multi} limit.
>
> Test result:
> ]# ./test_verifier 713 716
> #713/p ARG_CONST_MAP_PTR: null pointer OK
> #714/p ARG_CONST_MAP_PTR: valid map pointer OK
> #715/p ARG_CONST_MAP_PTR_OR_NULL: null pointer for ex_map OK
> #716/p ARG_CONST_MAP_PTR_OR_NULL: valid map pointer for ex_map OK
> Summary: 4 PASSED, 0 SKIPPED, 0 FAILED
>
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 22 +++++-
> .../testing/selftests/bpf/verifier/map_ptr.c | 70 +++++++++++++++++++
> 2 files changed, 91 insertions(+), 1 deletion(-)
>

[...]

> +{
> + "ARG_CONST_MAP_PTR_OR_NULL: null pointer for ex_map",
> + .insns = {
> + BPF_MOV64_IMM(BPF_REG_1, 0),
> + /* bpf_redirect_map_multi arg1 (in_map) */
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + /* bpf_redirect_map_multi arg2 (ex_map) */
> + BPF_MOV64_IMM(BPF_REG_2, 0),
> + /* bpf_redirect_map_multi arg3 (flags) */
> + BPF_MOV64_IMM(BPF_REG_3, 0),
> + BPF_EMIT_CALL(BPF_FUNC_redirect_map_multi),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map_devmap = { 1 },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .retval = 4,

Do we need one more case where this is map_or_null? In above
ex_map will be scalar tnum_const=0 and be exactly a null. This
will push verifier here,

meta->map_ptr = register_is_null(reg) ? NULL : reg->map_ptr;

In the below case it is known to be not null.

Is it also interesting to have a case where register_is_null(reg)
check fails and reg->map_ptr is set, but may be null.

> +},
> +{
> + "ARG_CONST_MAP_PTR_OR_NULL: valid map pointer for ex_map",
> + .insns = {
> + BPF_MOV64_IMM(BPF_REG_1, 0),
> + /* bpf_redirect_map_multi arg1 (in_map) */
> + BPF_LD_MAP_FD(BPF_REG_1, 0),
> + /* bpf_redirect_map_multi arg2 (ex_map) */
> + BPF_LD_MAP_FD(BPF_REG_2, 1),
> + /* bpf_redirect_map_multi arg3 (flags) */
> + BPF_MOV64_IMM(BPF_REG_3, 0),
> + BPF_EMIT_CALL(BPF_FUNC_redirect_map_multi),
> + BPF_EXIT_INSN(),
> + },
> + .fixup_map_devmap = { 1 },
> + .fixup_map_devmap_hash = { 3 },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .retval = 4,
> +},
> --
> 2.26.2
>