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@xxxxxx>
> ---
> 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;
> +}
