Re: [PATCH v2 bpf] selftests/bpf: Test bpf_skb_adjust_room on CHECKSUM_PARTIAL

[Martin KaFai Lau <martin.lau@linux.dev>]

On 12/20/22 9:05 PM, Andrii Nakryiko wrote:
> On Tue, Dec 20, 2022 at 5:49 PM Martin KaFai Lau <martin.lau@linux.dev> 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>
>> ---
>> v2: Add test to DENYLIST.s390x due to kfunc usage
>>
>>   tools/testing/selftests/bpf/DENYLIST.s390x    |  1 +
>>   .../selftests/bpf/prog_tests/decap_sanity.c   | 83 +++++++++++++++++++
>>   .../selftests/bpf/progs/bpf_tracing_net.h     |  6 ++
>>   .../selftests/bpf/progs/decap_sanity.c        | 68 +++++++++++++++
>>   4 files changed, 158 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..dba6c027920d 100644
>> --- a/tools/testing/selftests/bpf/DENYLIST.s390x
>> +++ b/tools/testing/selftests/bpf/DENYLIST.s390x
>> @@ -86,3 +86,4 @@ xdp_bpf2bpf                              # failed to auto-attach program 'trace_
>>   xdp_do_redirect                          # prog_run_max_size unexpected error: -22 (errno 22)
>>   xdp_synproxy                             # JIT does not support calling kernel function                                (kfunc)
>>   xfrm_info                                # JIT does not support calling kernel function                                (kfunc)
>> +decap_sanity                             # JIT does not support calling kernel function                                (kfunc)
> 
> let's keep this list sorted?

Ack.

> 
>> \ No newline at end of file
>> 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..2fbb3017b740
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/decap_sanity.c
>> @@ -0,0 +1,83 @@
>> +// 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;
> 
> you seem to be missing bpf_tc_hook_destroy() for clean up

It will go away with the 'ip netns del' at the end of the test, so not needed.
I will add a comment at the end of this function to make it clear.

> 
>> +
>> +       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_EQ(skel->bss->init_csum_partial, true, "init_csum_partial");
>> +       ASSERT_EQ(skel->bss->final_csum_none, true, "final_csum_none");
>> +       ASSERT_EQ(skel->bss->broken_csum_start, false, "broken_csum_start");
> 
> ASSERT_TRUE and ASSERT_FALSE ?

Ack.

Thanks for the review.

> 
>> +
>> +fail:
>> +       if (nstoken)
>> +               close_netns(nstoken);
>> +       system("ip netns del " NS_TEST " >& /dev/null");
>> +       decap_sanity__destroy(skel);
>> +}
> 
> [...]