From: sdf@google.com
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
kernel-team@meta.com, Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH v4 bpf] selftests/bpf: Test bpf_skb_adjust_room on CHECKSUM_PARTIAL
Date: Wed, 21 Dec 2022 11:19:02 -0800 [thread overview]
Message-ID: <Y6NcJjQDUE+Y/edw@google.com> (raw)
In-Reply-To: <20221221185653.1589961-1-martin.lau@linux.dev>
On 12/21, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> When the bpf_skb_adjust_room() shrinks the skb such that
> its csum_start is invalid, the skb->ip_summed should
> be reset from CHECKSUM_PARTIAL to CHECKSUM_NONE.
> The commit 54c3f1a81421 ("bpf: pull before calling skb_postpull_rcsum()")
> fixed it.
> This patch adds a test to ensure the skb->ip_summed changed
> from CHECKSUM_PARTIAL to CHECKSUM_NONE after bpf_skb_adjust_room().
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
I'm bit late to the party:
Acked-by: Stanislav Fomichev <sdf@google.com>
> ---
> v4: Remove one 'inline' from progs/decap_sanity.c
> v3:
> * Use ASSERT_TRUE and ASSERT_FALSE
> * On top of 'ip netns del', also call bpf_tc_hook_destroy()
> in case bpf_tc_hook_create() may allocate memory in
> the future.
> * Keep alphabet order in DENYLIST.s390x
> v2: Add test to DENYLIST.s390x due to kfunc usage
> tools/testing/selftests/bpf/DENYLIST.s390x | 1 +
> .../selftests/bpf/prog_tests/decap_sanity.c | 85 +++++++++++++++++++
> .../selftests/bpf/progs/bpf_tracing_net.h | 6 ++
> .../selftests/bpf/progs/decap_sanity.c | 68 +++++++++++++++
> 4 files changed, 160 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/decap_sanity.c
> create mode 100644 tools/testing/selftests/bpf/progs/decap_sanity.c
> diff --git a/tools/testing/selftests/bpf/DENYLIST.s390x
> b/tools/testing/selftests/bpf/DENYLIST.s390x
> index 585fcf73c731..3fc3e54b19aa 100644
> --- a/tools/testing/selftests/bpf/DENYLIST.s390x
> +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
> @@ -14,6 +14,7 @@ cgrp_kfunc # JIT does not
> support calling kernel f
> cgrp_local_storage # prog_attach unexpected error:
> -524 (trampoline)
> core_read_macros # unknown func
> bpf_probe_read#4
> (overlapping)
> d_path # failed to auto-attach
> program 'prog_stat': -524 (trampoline)
> +decap_sanity # JIT does not support calling
> kernel function (kfunc)
> deny_namespace # failed to attach: ERROR:
> strerror_r(-524)=22 (trampoline)
> dummy_st_ops # test_run unexpected error:
> -524 (errno 524) (trampoline)
> fentry_fexit # fentry attach failed:
> -524 (trampoline)
> diff --git a/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
> b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
> new file mode 100644
> index 000000000000..0b2f73b88c53
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <net/if.h>
> +#include <linux/in6.h>
> +
> +#include "test_progs.h"
> +#include "network_helpers.h"
> +#include "decap_sanity.skel.h"
> +
> +#define SYS(fmt, ...) \
> + ({ \
> + char cmd[1024]; \
> + snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__); \
> + if (!ASSERT_OK(system(cmd), cmd)) \
> + goto fail; \
> + })
> +
> +#define NS_TEST "decap_sanity_ns"
> +#define IPV6_IFACE_ADDR "face::1"
> +#define UDP_TEST_PORT 7777
> +
> +void test_decap_sanity(void)
> +{
> + LIBBPF_OPTS(bpf_tc_hook, qdisc_hook, .attach_point = BPF_TC_EGRESS);
> + LIBBPF_OPTS(bpf_tc_opts, tc_attach);
> + struct nstoken *nstoken = NULL;
> + struct decap_sanity *skel;
> + struct sockaddr_in6 addr;
> + socklen_t addrlen;
> + char buf[128] = {};
> + int sockfd, err;
> +
> + skel = decap_sanity__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "skel open_and_load"))
> + return;
> +
> + SYS("ip netns add %s", NS_TEST);
> + SYS("ip -net %s -6 addr add %s/128 dev lo nodad", NS_TEST,
> IPV6_IFACE_ADDR);
> + SYS("ip -net %s link set dev lo up", NS_TEST);
> +
> + nstoken = open_netns(NS_TEST);
> + if (!ASSERT_OK_PTR(nstoken, "open_netns"))
> + goto fail;
> +
> + qdisc_hook.ifindex = if_nametoindex("lo");
> + if (!ASSERT_GT(qdisc_hook.ifindex, 0, "if_nametoindex lo"))
> + goto fail;
> +
> + err = bpf_tc_hook_create(&qdisc_hook);
> + if (!ASSERT_OK(err, "create qdisc hook"))
> + goto fail;
> +
> + tc_attach.prog_fd = bpf_program__fd(skel->progs.decap_sanity);
> + err = bpf_tc_attach(&qdisc_hook, &tc_attach);
> + if (!ASSERT_OK(err, "attach filter"))
> + goto fail;
> +
> + addrlen = sizeof(addr);
> + err = make_sockaddr(AF_INET6, IPV6_IFACE_ADDR, UDP_TEST_PORT,
> + (void *)&addr, &addrlen);
> + if (!ASSERT_OK(err, "make_sockaddr"))
> + goto fail;
> + sockfd = socket(AF_INET6, SOCK_DGRAM, 0);
> + if (!ASSERT_NEQ(sockfd, -1, "socket"))
> + goto fail;
> + err = sendto(sockfd, buf, sizeof(buf), 0, (void *)&addr, addrlen);
> + close(sockfd);
> + if (!ASSERT_EQ(err, sizeof(buf), "send"))
> + goto fail;
> +
> + ASSERT_TRUE(skel->bss->init_csum_partial, "init_csum_partial");
> + ASSERT_TRUE(skel->bss->final_csum_none, "final_csum_none");
> + ASSERT_FALSE(skel->bss->broken_csum_start, "broken_csum_start");
> +
> +fail:
> + if (nstoken) {
> + bpf_tc_hook_destroy(&qdisc_hook);
> + close_netns(nstoken);
> + }
> + system("ip netns del " NS_TEST " >& /dev/null");
> + decap_sanity__destroy(skel);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> index b394817126cf..cfed4df490f3 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
> @@ -50,6 +50,12 @@
> #define ICSK_TIME_LOSS_PROBE 5
> #define ICSK_TIME_REO_TIMEOUT 6
> +#define ETH_HLEN 14
> +#define ETH_P_IPV6 0x86DD
> +
> +#define CHECKSUM_NONE 0
> +#define CHECKSUM_PARTIAL 3
> +
> #define IFNAMSIZ 16
> #define RTF_GATEWAY 0x0002
> diff --git a/tools/testing/selftests/bpf/progs/decap_sanity.c
> b/tools/testing/selftests/bpf/progs/decap_sanity.c
> new file mode 100644
> index 000000000000..bd3c657c58a7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/decap_sanity.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
> +
> +#include "vmlinux.h"
> +#include "bpf_tracing_net.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_endian.h>
> +
> +#define UDP_TEST_PORT 7777
> +
> +void *bpf_cast_to_kern_ctx(void *) __ksym;
> +bool init_csum_partial = false;
> +bool final_csum_none = false;
> +bool broken_csum_start = false;
> +
> +static unsigned int skb_headlen(const struct sk_buff *skb)
> +{
> + return skb->len - skb->data_len;
> +}
> +
> +static unsigned int skb_headroom(const struct sk_buff *skb)
> +{
> + return skb->data - skb->head;
> +}
> +
> +static int skb_checksum_start_offset(const struct sk_buff *skb)
> +{
> + return skb->csum_start - skb_headroom(skb);
> +}
> +
> +SEC("tc")
> +int decap_sanity(struct __sk_buff *skb)
> +{
> + struct sk_buff *kskb;
> + struct ipv6hdr ip6h;
> + struct udphdr udph;
> + int err;
> +
> + if (skb->protocol != __bpf_constant_htons(ETH_P_IPV6))
> + return TC_ACT_SHOT;
> +
> + if (bpf_skb_load_bytes(skb, ETH_HLEN, &ip6h, sizeof(ip6h)))
> + return TC_ACT_SHOT;
> +
> + if (ip6h.nexthdr != IPPROTO_UDP)
> + return TC_ACT_SHOT;
> +
> + if (bpf_skb_load_bytes(skb, ETH_HLEN + sizeof(ip6h), &udph,
> sizeof(udph)))
> + return TC_ACT_SHOT;
> +
> + if (udph.dest != __bpf_constant_htons(UDP_TEST_PORT))
> + return TC_ACT_SHOT;
> +
> + kskb = bpf_cast_to_kern_ctx(skb);
> + init_csum_partial = (kskb->ip_summed == CHECKSUM_PARTIAL);
> + err = bpf_skb_adjust_room(skb, -(s32)(ETH_HLEN + sizeof(ip6h) +
> sizeof(udph)),
> + 1, BPF_F_ADJ_ROOM_FIXED_GSO);
> + if (err)
> + return TC_ACT_SHOT;
> + final_csum_none = (kskb->ip_summed == CHECKSUM_NONE);
> + if (kskb->ip_summed == CHECKSUM_PARTIAL &&
> + (unsigned int)skb_checksum_start_offset(kskb) >= skb_headlen(kskb))
> + broken_csum_start = true;
> +
> + return TC_ACT_SHOT;
> +}
> +
> +char __license[] SEC("license") = "GPL";
> --
> 2.30.2
next prev parent reply other threads:[~2022-12-21 19:19 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-21 18:56 [PATCH v4 bpf] selftests/bpf: Test bpf_skb_adjust_room on CHECKSUM_PARTIAL Martin KaFai Lau
2022-12-21 19:19 ` sdf [this message]
2022-12-22 0:00 ` patchwork-bot+netdevbpf
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=Y6NcJjQDUE+Y/edw@google.com \
--to=sdf@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@meta.com \
--cc=kuba@kernel.org \
--cc=martin.lau@linux.dev \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox