Commit 2bdeb3ed authored by Alexei Starovoitov's avatar Alexei Starovoitov
Browse files

Merge branch 'fix-sockmap'



John Fastabend says:

====================
Fix a splat introduced by recent changes to avoid skipping ingress policy
when kTLS is enabled. The RCU splat was introduced because in the non-TLS
case the caller is wrapped in an rcu_read_lock/unlock. But, in the TLS
case we have a reference to the psock and the caller did not wrap its
call in rcu_read_lock/unlock.

To fix extend the RCU section to include the redirect case which was
missed. From v1->v2 I changed the location a bit to simplify the code
some. See patch 1.

But, then Martin asked why it was not needed in the non-TLS case. The
answer for patch 1 was, as stated above, because the caller has the
rcu read lock. However, there was still a missing case where a BPF
user could in-theory line up a set of parameters to hit a case
where the code was entered from strparser side from a different context
then the initial caller. To hit this user would need a parser program
to return value greater than skb->len then an ENOMEM error could happen
in the strparser codepath triggering strparser to retry from a workqueue
and without rcu_read_lock original caller used. See patch 2 for details.

Finally, we don't actually have any selftests for parser returning a
value geater than skb->len so add one in patch 3. This is especially
needed because at least I don't have any code that uses the parser
to return value greater than skb->len. So I wouldn't have caught any
errors here in my own testing.

Thanks, John

v1->v2: simplify code in patch 1 some and add patches 2 and 3.
====================

Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
parents 7a64135f 53792fa4
Loading
Loading
Loading
Loading
+15 −8
Original line number Diff line number Diff line
@@ -683,7 +683,7 @@ static struct sk_psock *sk_psock_from_strp(struct strparser *strp)
	return container_of(parser, struct sk_psock, parser);
}

static void sk_psock_skb_redirect(struct sk_psock *psock, struct sk_buff *skb)
static void sk_psock_skb_redirect(struct sk_buff *skb)
{
	struct sk_psock *psock_other;
	struct sock *sk_other;
@@ -715,12 +715,11 @@ static void sk_psock_skb_redirect(struct sk_psock *psock, struct sk_buff *skb)
	}
}

static void sk_psock_tls_verdict_apply(struct sk_psock *psock,
				       struct sk_buff *skb, int verdict)
static void sk_psock_tls_verdict_apply(struct sk_buff *skb, int verdict)
{
	switch (verdict) {
	case __SK_REDIRECT:
		sk_psock_skb_redirect(psock, skb);
		sk_psock_skb_redirect(skb);
		break;
	case __SK_PASS:
	case __SK_DROP:
@@ -741,8 +740,8 @@ int sk_psock_tls_strp_read(struct sk_psock *psock, struct sk_buff *skb)
		ret = sk_psock_bpf_run(psock, prog, skb);
		ret = sk_psock_map_verd(ret, tcp_skb_bpf_redirect_fetch(skb));
	}
	sk_psock_tls_verdict_apply(skb, ret);
	rcu_read_unlock();
	sk_psock_tls_verdict_apply(psock, skb, ret);
	return ret;
}
EXPORT_SYMBOL_GPL(sk_psock_tls_strp_read);
@@ -770,7 +769,7 @@ static void sk_psock_verdict_apply(struct sk_psock *psock,
		}
		goto out_free;
	case __SK_REDIRECT:
		sk_psock_skb_redirect(psock, skb);
		sk_psock_skb_redirect(skb);
		break;
	case __SK_DROP:
		/* fall-through */
@@ -782,11 +781,18 @@ out_free:

static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
{
	struct sk_psock *psock = sk_psock_from_strp(strp);
	struct sk_psock *psock;
	struct bpf_prog *prog;
	int ret = __SK_DROP;
	struct sock *sk;

	rcu_read_lock();
	sk = strp->sk;
	psock = sk_psock(sk);
	if (unlikely(!psock)) {
		kfree_skb(skb);
		goto out;
	}
	prog = READ_ONCE(psock->progs.skb_verdict);
	if (likely(prog)) {
		skb_orphan(skb);
@@ -794,8 +800,9 @@ static void sk_psock_strp_read(struct strparser *strp, struct sk_buff *skb)
		ret = sk_psock_bpf_run(psock, prog, skb);
		ret = sk_psock_map_verd(ret, tcp_skb_bpf_redirect_fetch(skb));
	}
	rcu_read_unlock();
	sk_psock_verdict_apply(psock, skb, ret);
out:
	rcu_read_unlock();
}

static int sk_psock_strp_read_done(struct strparser *strp, int err)
+7 −1
Original line number Diff line number Diff line
@@ -79,7 +79,7 @@ struct {

struct {
	__uint(type, BPF_MAP_TYPE_ARRAY);
	__uint(max_entries, 2);
	__uint(max_entries, 3);
	__type(key, int);
	__type(value, int);
} sock_skb_opts SEC(".maps");
@@ -94,6 +94,12 @@ struct {
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
{
	int *f, two = 2;

	f = bpf_map_lookup_elem(&sock_skb_opts, &two);
	if (f && *f) {
		return *f;
	}
	return skb->len;
}

+18 −0
Original line number Diff line number Diff line
@@ -85,6 +85,7 @@ int txmsg_ktls_skb_drop;
int txmsg_ktls_skb_redir;
int ktls;
int peek_flag;
int skb_use_parser;

static const struct option long_options[] = {
	{"help",	no_argument,		NULL, 'h' },
@@ -174,6 +175,7 @@ static void test_reset(void)
	txmsg_apply = txmsg_cork = 0;
	txmsg_ingress = txmsg_redir_skb = 0;
	txmsg_ktls_skb = txmsg_ktls_skb_drop = txmsg_ktls_skb_redir = 0;
	skb_use_parser = 0;
}

static int test_start_subtest(const struct _test *t, struct sockmap_options *o)
@@ -1211,6 +1213,11 @@ run:
		}
	}

	if (skb_use_parser) {
		i = 2;
		err = bpf_map_update_elem(map_fd[7], &i, &skb_use_parser, BPF_ANY);
	}

	if (txmsg_drop)
		options->drop_expected = true;

@@ -1650,6 +1657,16 @@ static void test_txmsg_cork(int cgrp, struct sockmap_options *opt)
	test_send(opt, cgrp);
}

static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
{
	txmsg_pass = 1;
	skb_use_parser = 512;
	opt->iov_length = 256;
	opt->iov_count = 1;
	opt->rate = 2;
	test_exec(cgrp, opt);
}

char *map_names[] = {
	"sock_map",
	"sock_map_txmsg",
@@ -1748,6 +1765,7 @@ struct _test test[] = {
	{"txmsg test pull-data", test_txmsg_pull},
	{"txmsg test pop-data", test_txmsg_pop},
	{"txmsg test push/pop data", test_txmsg_push_pop},
	{"txmsg text ingress parser", test_txmsg_ingress_parser},
};

static int check_whitelist(struct _test *t, struct sockmap_options *opt)