Date:   Tue, 17 Apr 2018 16:04:14 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     "Nikita V. Shirokov" <tehnerd@...nerd.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 02/10] [bpf]: adding tests for
 bpf_xdp_adjust_tail

On Mon, Apr 16, 2018 at 11:51:23PM -0700, Nikita V. Shirokov wrote:
> adding selftests for bpf_xdp_adjust_tail helper. in this syntetic test
> we are testing that 1) if data_end < data helper will return EINVAL
> 2) for normal use case packet's length would be reduced.
> 
> aside from adding new tests i'm changing behaviour of bpf_prog_test_run
> so it would recalculate packet's length if only data_end pointer was
> changed
> 
> Signed-off-by: Nikita V. Shirokov <tehnerd@...nerd.com>
> ---
>  net/bpf/test_run.c                             |  3 ++-
>  tools/include/uapi/linux/bpf.h                 | 11 ++++++++-
>  tools/testing/selftests/bpf/Makefile           |  2 +-
>  tools/testing/selftests/bpf/bpf_helpers.h      |  3 +++
>  tools/testing/selftests/bpf/test_adjust_tail.c | 29 +++++++++++++++++++++++
>  tools/testing/selftests/bpf/test_progs.c       | 32 ++++++++++++++++++++++++++
>  6 files changed, 77 insertions(+), 3 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/test_adjust_tail.c
> 
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 2ced48662c1f..68c3578343b4 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -170,7 +170,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
>  	xdp.rxq = &rxqueue->xdp_rxq;
>  
>  	retval = bpf_test_run(prog, &xdp, repeat, &duration);
> -	if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN)
> +	if (xdp.data != data + XDP_PACKET_HEADROOM + NET_IP_ALIGN ||
> +	    xdp.data_end != xdp.data + size)

please split fixing prog_test_run for adjust_tail into separate patch
and selftests into another one.

>  		size = xdp.data_end - xdp.data;
>  	ret = bpf_test_finish(kattr, uattr, xdp.data, size, retval, duration);
>  	kfree(data);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 9d07465023a2..9a2d1a04eb24 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -755,6 +755,13 @@ union bpf_attr {
>   *     @addr: pointer to struct sockaddr to bind socket to
>   *     @addr_len: length of sockaddr structure
>   *     Return: 0 on success or negative error code
> + *
> + * int bpf_xdp_adjust_tail(xdp_md, delta)
> + *     Adjust the xdp_md.data_end by delta. Only shrinking of packet's
> + *     size is supported.
> + *     @xdp_md: pointer to xdp_md
> + *     @delta: A negative integer to be added to xdp_md.data_end
> + *     Return: 0 on success or negative on error
>   */
>  #define __BPF_FUNC_MAPPER(FN)		\
>  	FN(unspec),			\
> @@ -821,7 +828,8 @@ union bpf_attr {
>  	FN(msg_apply_bytes),		\
>  	FN(msg_cork_bytes),		\
>  	FN(msg_pull_data),		\
> -	FN(bind),
> +	FN(bind),			\
> +	FN(xdp_adjust_tail),
>  
>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>   * function eBPF program intends to call
> @@ -864,6 +872,7 @@ enum bpf_func_id {
>  /* BPF_FUNC_skb_set_tunnel_key flags. */
>  #define BPF_F_ZERO_CSUM_TX		(1ULL << 1)
>  #define BPF_F_DONT_FRAGMENT		(1ULL << 2)
> +#define BPF_F_SEQ_NUMBER		(1ULL << 3)

William Tu missed adding it to tools/include/uapi/bpf.h when it was added
to main uapi/bpf.h
but don't add it as part of this patch.
I saw a separate patch for this passing by in tip tree from Arnaldo.
I'm not sure how quickly it will get into Linus tree,
let's not create extra merge conflicts.

>  
>  /* BPF_FUNC_perf_event_output, BPF_FUNC_perf_event_read and
>   * BPF_FUNC_perf_event_read_value flags.
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 0a315ddabbf4..3e819dc70bee 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -31,7 +31,7 @@ TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test
>  	sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \
>  	test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
>  	sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \
> -	sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o
> +	sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o
>  
>  # Order correspond to 'make run_tests' order
>  TEST_PROGS := test_kmod.sh \
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index d8223d99f96d..50c607014b22 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -96,6 +96,9 @@ static int (*bpf_msg_pull_data)(void *ctx, int start, int end, int flags) =
>  	(void *) BPF_FUNC_msg_pull_data;
>  static int (*bpf_bind)(void *ctx, void *addr, int addr_len) =
>  	(void *) BPF_FUNC_bind;
> +static int (*bpf_xdp_adjust_tail)(void *ctx, int offset) =
> +	(void *) BPF_FUNC_xdp_adjust_tail;
> +
>  
>  /* llvm builtin functions that eBPF C program may use to
>   * emit BPF_LD_ABS and BPF_LD_IND instructions
> diff --git a/tools/testing/selftests/bpf/test_adjust_tail.c b/tools/testing/selftests/bpf/test_adjust_tail.c
> new file mode 100644
> index 000000000000..86239e792d6d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/test_adjust_tail.c
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2016,2017 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */

Please use SPDX header here and in other patches.

> +#include <linux/bpf.h>
> +#include <linux/if_ether.h>
> +#include "bpf_helpers.h"
> +
> +int _version SEC("version") = 1;

we really should fix libbpf to avoid requiring that for all program types.
It's annoying to see this in every networking test.

Powered by blists - more mailing lists