Re: [PATCH bpf-next 2/2] selftests/bpf: Offloaded prog after non-offloaded should not cause BUG

[Martin KaFai Lau <martin.lau@linux.dev>]

On 9/11/23 5:55 PM, Eduard Zingerman wrote:
> Check what happens if non-offloaded dev bound BPF
> program is followed by offloaded dev bound program.
> Test case adapated from syzbot report [1].
> 
> [1] https://lore.kernel.org/bpf/000000000000d97f3c060479c4f8@google.com/
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>   .../bpf/prog_tests/xdp_dev_bound_only.c       | 58 +++++++++++++++++++
>   1 file changed, 58 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c b/tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c
> new file mode 100644
> index 000000000000..5ee4c16d2e21
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_dev_bound_only.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <net/if.h>
> +#include <test_progs.h>
> +#include <network_helpers.h>
> +
> +#define LOCAL_NETNS "xdp_dev_bound_only_netns"
> +
> +int load_dummy_prog(char *name, __u32 ifindex, __u32 flags)

I added static.

> +{
> +	struct bpf_insn insns[] = { BPF_MOV64_IMM(BPF_REG_0, 0), BPF_EXIT_INSN() };
> +	LIBBPF_OPTS(bpf_prog_load_opts, opts);
> +
> +	opts.prog_flags = flags;
> +	opts.prog_ifindex = ifindex;
> +	return bpf_prog_load(BPF_PROG_TYPE_XDP, name, "GPL", insns, ARRAY_SIZE(insns), &opts);
> +}
> +
> +/* A test case for bpf_offload_netdev->offload handling bug:
> + * - create a veth device (does not support offload);
> + * - create a device bound XDP program with BPF_F_XDP_DEV_BOUND_ONLY flag
> + *   (such programs are not offloaded);
> + * - create a device bound XDP program without flags (such programs are offloaded).
> + * This might lead to 'BUG: kernel NULL pointer dereference'.
> + */
> +void test_xdp_dev_bound_only_offdev(void)
> +{
> +	struct nstoken *tok = NULL;
> +	__u32 ifindex;
> +	int fd1 = -1;
> +	int fd2 = -1;
> +
> +	SYS(out, "ip netns add " LOCAL_NETNS);
> +	tok = open_netns(LOCAL_NETNS);

Also added NULL check for tok.

> +	SYS(out, "ip link add eth42 type veth");
> +	ifindex = if_nametoindex("eth42");
> +	if (!ASSERT_NEQ(ifindex, 0, "if_nametoindex")) {
> +		perror("if_nametoindex");
> +		goto out;
> +	}
> +	fd1 = load_dummy_prog("dummy1", ifindex, BPF_F_XDP_DEV_BOUND_ONLY);
> +	if (!ASSERT_GE(fd1, 0, "load_dummy_prog #1")) {
> +		perror("load_dummy_prog #1");
> +		goto out;
> +	}
> +	/* Program with ifindex is considered offloaded, however veth
> +	 * does not support offload => error should be reported.
> +	 */
> +	fd2 = load_dummy_prog("dummy2", ifindex, 0);
> +	ASSERT_EQ(fd2, -EINVAL, "load_dummy_prog #2 (offloaded)");
> +
> +out:
> +	close(fd1);
> +	close(fd2);
> +	SYS_NOFAIL("ip link delete eth42");
> +	SYS_NOFAIL("ip netns del " LOCAL_NETNS);
> +	if (tok)

close_netns() can handle NULL, so removed this tok check.

Applied. Thanks for the fix and test!

> +		close_netns(tok);
> +}