bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: Martin KaFai Lau <kafai@fb.com>, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Lorenz Bauer <lmb@cloudflare.com>,
	netdev@vger.kernel.org
Subject: RE: [PATCH v4 bpf-next 13/13] bpf: selftest: Add test_btf_skc_cls_ingress
Date: Fri, 25 Sep 2020 09:24:02 -0700	[thread overview]
Message-ID: <5f6e19a2621d8_8ae15208fd@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <20200925000458.3859627-1-kafai@fb.com>

Martin KaFai Lau wrote:
> This patch attaches a classifier prog to the ingress filter.
> It exercises the following helpers with different socket pointer
> types in different logical branches:
> 1. bpf_sk_release()
> 2. bpf_sk_assign()
> 3. bpf_skc_to_tcp_request_sock(), bpf_skc_to_tcp_sock()
> 4. bpf_tcp_gen_syncookie, bpf_tcp_check_syncookie
> 
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
>  tools/testing/selftests/bpf/bpf_tcp_helpers.h |   5 +
>  .../bpf/prog_tests/btf_skc_cls_ingress.c      | 234 ++++++++++++++++++
>  .../bpf/progs/test_btf_skc_cls_ingress.c      | 174 +++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c
> 


Hi Martin,

One piece I'm missing is how does this handle null pointer dereferences
from network side when reading BTF objects? I've not got through all the
code yet so maybe I'm just not there yet.

For example,

> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> index a0e8b3758bd7..2915664c335d 100644
> --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> @@ -16,6 +16,7 @@ BPF_PROG(name, args)
>  
>  struct sock_common {
>  	unsigned char	skc_state;
> +	__u16		skc_num;
>  } __attribute__((preserve_access_index));
>  
>  enum sk_pacing {
> @@ -45,6 +46,10 @@ struct inet_connection_sock {
>  	__u64			  icsk_ca_priv[104 / sizeof(__u64)];
>  } __attribute__((preserve_access_index));
>  
> +struct request_sock {
> +	struct sock_common		__req_common;
> +} __attribute__((preserve_access_index));
> +
>  struct tcp_sock {
>  	struct inet_connection_sock	inet_conn;

add some pointer from tcp_sock which is likely not set so should be NULL,

        struct tcp_fastopen_request *fastopen_req;

[...]

> +	if (bpf_skc->state == BPF_TCP_NEW_SYN_RECV) {
> +		struct request_sock *req_sk;
> +
> +		req_sk = (struct request_sock *)bpf_skc_to_tcp_request_sock(bpf_skc);
> +		if (!req_sk) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		if (bpf_sk_assign(skb, req_sk, 0)) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		req_sk_sport = req_sk->__req_common.skc_num;
> +
> +		bpf_sk_release(req_sk);
> +		return TC_ACT_OK;
> +	} else if (bpf_skc->state == BPF_TCP_LISTEN) {
> +		struct tcp_sock *tp;
> +
> +		tp = bpf_skc_to_tcp_sock(bpf_skc);
> +		if (!tp) {
> +			LOG();
> +			goto release;
> +		}
> +
> +		if (bpf_sk_assign(skb, tp, 0)) {
> +			LOG();
> +			goto release;
> +		}
> +


Then use it here without a null check in the BPF program,

                fastopen = tp->fastopen_req;
		if (fastopen->size > 0x1234)
                      (do something)

> +		listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num;
> +
> +		test_syncookie_helper(ip6h, th, tp, skb);
> +		bpf_sk_release(tp);
> +		return TC_ACT_OK;
> +	}

My quick check shows this didn't actually fault and the xlated
looks like it did the read and dereference. Must be missing
something? We shouldn't have fault_handler set for cls_ingress.

Perhaps a comment in the cover letter about this would be
helpful? Or if I'm just being dense this morning let me know
as well. ;)

> +
> +	if (bpf_sk_assign(skb, bpf_skc, 0))
> +		LOG();
> +
> +release:
> +	bpf_sk_release(bpf_skc);
> +	return TC_ACT_OK;
> +}

  reply	other threads:[~2020-09-25 16:24 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-25  0:03 [PATCH v4 bpf-next 00/13] bpf: Enable bpf_skc_to_* sock casting helper to networking prog type Martin KaFai Lau
2020-09-25  0:03 ` [PATCH v4 bpf-next 01/13] bpf: Move the PTR_TO_BTF_ID check to check_reg_type() Martin KaFai Lau
2020-09-25  8:22   ` Lorenz Bauer
2020-09-25 13:36   ` John Fastabend
2020-09-25  0:03 ` [PATCH v4 bpf-next 02/13] bpf: Enable bpf_skc_to_* sock casting helper to networking prog type Martin KaFai Lau
2020-09-25  8:26   ` Lorenz Bauer
2020-09-25 13:18     ` Martin KaFai Lau
2020-09-25 13:50       ` Lorenz Bauer
2020-09-25 15:47         ` Alexei Starovoitov
2020-09-25 14:21   ` John Fastabend
2020-09-25  0:03 ` [PATCH v4 bpf-next 03/13] bpf: Change bpf_sk_release and bpf_sk_*cgroup_id to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 04/13] bpf: Change bpf_sk_storage_*() " Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 05/13] bpf: Change bpf_tcp_*_syncookie " Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 06/13] bpf: Change bpf_sk_assign " Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 07/13] bpf: selftest: Add ref_tracking verifier test for bpf_skc casting Martin KaFai Lau
2020-09-25  8:30   ` Lorenz Bauer
2020-09-25  0:04 ` [PATCH v4 bpf-next 08/13] bpf: selftest: Move sock_fields test into test_progs Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 09/13] bpf: selftest: Adapt sock_fields test to use skel and global variables Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 10/13] bpf: selftest: Use network_helpers in the sock_fields test Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 11/13] bpf: selftest: Use bpf_skc_to_tcp_sock() " Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 12/13] bpf: selftest: Remove enum tcp_ca_state from bpf_tcp_helpers.h Martin KaFai Lau
2020-09-25  0:04 ` [PATCH v4 bpf-next 13/13] bpf: selftest: Add test_btf_skc_cls_ingress Martin KaFai Lau
2020-09-25 16:24   ` John Fastabend [this message]
2020-09-25 17:58     ` Martin KaFai Lau
2020-09-25  9:40 ` [PATCH v4 bpf-next 00/13] bpf: Enable bpf_skc_to_* sock casting helper to networking prog type Lorenz Bauer
2020-09-25 23:22 ` Alexei Starovoitov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5f6e19a2621d8_8ae15208fd@john-XPS-13-9370.notmuch \
    --to=john.fastabend@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kafai@fb.com \
    --cc=kernel-team@fb.com \
    --cc=lmb@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --subject='RE: [PATCH v4 bpf-next 13/13] bpf: selftest: Add test_btf_skc_cls_ingress' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).