netdev - Re: [PATCH bpf-next] selftests: xsk: read current MAX_SKB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ8uoz0BDJ=y-5M3=Wrz7F1LtT8AUVCyNh1G88SaKv+yEYL-Bg@mail.gmail.com>
Date: Tue, 10 Sep 2024 13:48:35 +0200
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, netdev@...r.kernel.org, magnus.karlsson@...el.com, 
	bjorn@...nel.org
Subject: Re: [PATCH bpf-next] selftests: xsk: read current MAX_SKB_FRAGS from
 sysctl knob

On Mon, 9 Sept 2024 at 16:12, Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> Currently, xskxceiver assumes that MAX_SKB_FRAGS value is always 17
> which is not true - since the introduction of BIG TCP this can now take
> any value between 17 to 45 via CONFIG_MAX_SKB_FRAGS.
>
> Adjust the TOO_MANY_FRAGS test case to read the currently configured
> MAX_SKB_FRAGS value by reading it from /proc/sys/net/core/max_skb_frags.
>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 41 +++++++++++++++++++++---
>  tools/testing/selftests/bpf/xskxceiver.h |  1 -
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 92af633faea8..595b6da26897 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -325,6 +325,25 @@ static bool ifobj_zc_avail(struct ifobject *ifobject)
>         return zc_avail;
>  }
>
> +#define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
> +static unsigned int get_max_skb_frags(void)
> +{
> +       unsigned int max_skb_frags = 0;
> +       FILE *file;
> +
> +       file = fopen(MAX_SKB_FRAGS_PATH, "r");
> +       if (!file) {
> +               ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);
> +               return 0;
> +       }
> +
> +       if (fscanf(file, "%u", &max_skb_frags) != 1)
> +               ksft_print_msg("Error reading %s\n", MAX_SKB_FRAGS_PATH);
> +
> +       fclose(file);
> +       return max_skb_frags;
> +}
> +
>  static struct option long_options[] = {
>         {"interface", required_argument, 0, 'i'},
>         {"busy-poll", no_argument, 0, 'b'},
> @@ -2245,13 +2264,22 @@ static int testapp_poll_rxq_tmout(struct test_spec *test)
>
>  static int testapp_too_many_frags(struct test_spec *test)
>  {
> -       struct pkt pkts[2 * XSK_DESC__MAX_SKB_FRAGS + 2] = {};
> +       struct pkt *pkts;
>         u32 max_frags, i;
> +       int ret;
>
> -       if (test->mode == TEST_MODE_ZC)
> +       if (test->mode == TEST_MODE_ZC) {
>                 max_frags = test->ifobj_tx->xdp_zc_max_segs;
> -       else
> -               max_frags = XSK_DESC__MAX_SKB_FRAGS;
> +       } else {
> +               max_frags = get_max_skb_frags();
> +               if (!max_frags)
> +                       return TEST_FAILURE;

Thanks for this fix Maciej. However, I think failing the test here is
a little bit too drastic. How about just returning TEST_SKIP and print
out that the max number of skbs is unknown as the reason for the skip?
Or even more optimistically, print out a warning that we could not
read the max number of skb but we are guessing 17 and then run the
test? If it passes, great we guessed correctly, but if it fails we are
not worse off than the current code. Do not know how often a file
system does not contain /proc/sys/net/core/max_skb_frags though.

> +               max_frags += 1;
> +       }
> +
> +       pkts = calloc(2 * max_frags + 2, sizeof(struct pkt));
> +       if (!pkts)
> +               return TEST_FAILURE;
>
>         test->mtu = MAX_ETH_JUMBO_SIZE;
>
> @@ -2281,7 +2309,10 @@ static int testapp_too_many_frags(struct test_spec *test)
>         pkts[2 * max_frags + 1].valid = true;
>
>         pkt_stream_generate_custom(test, pkts, 2 * max_frags + 2);
> -       return testapp_validate_traffic(test);
> +       ret = testapp_validate_traffic(test);
> +
> +       free(pkts);
> +       return ret;
>  }
>
>  static int xsk_load_xdp_programs(struct ifobject *ifobj)
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 885c948c5d83..e46e823f6a1a 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -55,7 +55,6 @@
>  #define XSK_UMEM__LARGE_FRAME_SIZE (3 * 1024)
>  #define XSK_UMEM__MAX_FRAME_SIZE (4 * 1024)
>  #define XSK_DESC__INVALID_OPTION (0xffff)
> -#define XSK_DESC__MAX_SKB_FRAGS 18
>  #define HUGEPAGE_SIZE (2 * 1024 * 1024)
>  #define PKT_DUMP_NB_TO_PRINT 16
>  #define RUN_ALL_TESTS UINT_MAX
> --
> 2.34.1
>
>