netdev - Re: [PATCH bpf-next 4/8] bpf: add PROG_TEST_RUN support for sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210223011153.4cvzpvxqn7arbcej@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 22 Feb 2021 17:11:53 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Lorenz Bauer <lmb@...udflare.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        jakub@...udflare.com, kernel-team@...udflare.com,
        bpf@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 4/8] bpf: add PROG_TEST_RUN support for
 sk_lookup programs

On Tue, Feb 16, 2021 at 10:57:09AM +0000, Lorenz Bauer wrote:
> +	user_ctx = bpf_ctx_init(kattr, sizeof(*user_ctx));
> +	if (IS_ERR(user_ctx))
> +		return PTR_ERR(user_ctx);
> +
> +	if (!user_ctx)
> +		return -EINVAL;
> +
> +	if (user_ctx->sk)
> +		goto out;
> +
> +	if (!range_is_zero(user_ctx, offsetofend(typeof(*user_ctx), local_port), sizeof(*user_ctx)))
> +		goto out;
> +
> +	if (user_ctx->local_port > U16_MAX || user_ctx->remote_port > U16_MAX) {
> +		ret = -ERANGE;
> +		goto out;
> +	}
> +
> +	ctx.family = user_ctx->family;
> +	ctx.protocol = user_ctx->protocol;
> +	ctx.dport = user_ctx->local_port;
> +	ctx.sport = user_ctx->remote_port;
> +
> +	switch (ctx.family) {
> +	case AF_INET:
> +		ctx.v4.daddr = user_ctx->local_ip4;
> +		ctx.v4.saddr = user_ctx->remote_ip4;
> +		break;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +		ctx.v6.daddr = (struct in6_addr *)user_ctx->local_ip6;
> +		ctx.v6.saddr = (struct in6_addr *)user_ctx->remote_ip6;
> +		break;
> +#endif
> +
> +	default:
> +		ret = -EAFNOSUPPORT;
> +		goto out;
> +	}
> +
> +	while (t_check(&t, repeat, &ret, &duration)) {
> +		ctx.selected_sk = NULL;
> +		retval = BPF_PROG_SK_LOOKUP_RUN_ARRAY(progs, ctx, BPF_PROG_RUN);
> +	}
> +
> +	if (ret < 0)
> +		goto out;
> +
> +	user_ctx->cookie = 0;
> +	if (ctx.selected_sk) {
> +		if (ctx.selected_sk->sk_reuseport && !ctx.no_reuseport) {
> +			ret = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		user_ctx->cookie = sock_gen_cookie(ctx.selected_sk);
> +	}

I'm struggling to come up with the case where running N sk_lookup progs
like this cannot be done with running them one by one.
It looks to me that this N prog_fds api is not really about running and
testing the progs, but about testing BPF_PROG_SK_LOOKUP_RUN_ARRAY()
SK_PASS vs SK_DROP logic.
So it's more of the kernel infra testing than program testing.
Are you suggesting that the sequence of sk_lookup progs are so delicate
that they are aware of each other and _has_ to be tested together
with gluing logic that the macro provides?
But if it is so then testing the progs one by one would be better,
because test_run will be able to check each individual prog return code
instead of implicit BPF_PROG_SK_LOOKUP_RUN_ARRAY logic.
It feels less of the unit test and more as a full stack test,
but if so then lack of cookie on input is questionable.
The progs can only examine port/ip/family data.
So testing them individually would give more accurate picture on
what progs are doing and potential bugs can be found sooner than
testing the sequence of progs. In a sequence one prog could have
been buggy, but the final cookie came out correct.

Looking at patch 7 it seems the unit test framework will be comparing
the cookies for your production tests, but then nentns argument
in the cover letter is suprising. If the tests are run in the init_netns
then selected sockets will be just as special as in patch 7.
So it's not a full stack kinda test.

In other words I'm struggling with in-between state of the api.
test_run with N fds is not really a full test, but not a unit test either.