From: Stanislav Fomichev <sdf@fomichev.me>
To: Song Liu <liu.song.a23@gmail.com>
Cc: Stanislav Fomichev <sdf@google.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
"David S . Miller" <davem@davemloft.net>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Willem de Bruijn <willemb@google.com>,
Petar Penkov <ppenkov@google.com>
Subject: Re: [PATCH bpf-next 5/7] sefltests/bpf: support FLOW_DISSECTOR_F_PARSE_1ST_FRAG
Date: Wed, 24 Jul 2019 16:52:54 -0700 [thread overview]
Message-ID: <20190724235254.GB3500@mini-arch> (raw)
In-Reply-To: <CAPhsuW6Z2Bx66ZDOV-9jW+hsxKbZJxY-YFgP0rL_4QipAuptQA@mail.gmail.com>
On 07/24, Song Liu wrote:
> On Wed, Jul 24, 2019 at 10:11 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > bpf_flow.c: exit early unless FLOW_DISSECTOR_F_PARSE_1ST_FRAG is passed
> > in flags. Also, set ip_proto earlier, this makes sure we have correct
> > value with fragmented packets.
> >
> > Add selftest cases to test ipv4/ipv6 fragments and skip eth_get_headlen
> > tests that don't have FLOW_DISSECTOR_F_PARSE_1ST_FRAG flag.
> >
> > eth_get_headlen calls flow dissector with
> > FLOW_DISSECTOR_F_PARSE_1ST_FRAG flag so we can't run tests that
> > have different set of input flags against it.
> >
> > Cc: Willem de Bruijn <willemb@google.com>
> > Cc: Petar Penkov <ppenkov@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../selftests/bpf/prog_tests/flow_dissector.c | 129 ++++++++++++++++++
> > tools/testing/selftests/bpf/progs/bpf_flow.c | 28 +++-
> > 2 files changed, 151 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > index c938283ac232..966cb3b06870 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
> > @@ -5,6 +5,10 @@
> > #include <linux/if_tun.h>
> > #include <sys/uio.h>
> >
> > +#ifndef IP_MF
> > +#define IP_MF 0x2000
> > +#endif
> > +
> > #define CHECK_FLOW_KEYS(desc, got, expected) \
> > CHECK_ATTR(memcmp(&got, &expected, sizeof(got)) != 0, \
> > desc, \
> > @@ -49,6 +53,18 @@ struct ipv6_pkt {
> > struct tcphdr tcp;
> > } __packed;
> >
> > +struct ipv6_frag_pkt {
> > + struct ethhdr eth;
> > + struct ipv6hdr iph;
> > + struct frag_hdr {
> > + __u8 nexthdr;
> > + __u8 reserved;
> > + __be16 frag_off;
> > + __be32 identification;
> > + } ipf;
> > + struct tcphdr tcp;
> > +} __packed;
> > +
> > struct dvlan_ipv6_pkt {
> > struct ethhdr eth;
> > __u16 vlan_tci;
> > @@ -65,9 +81,11 @@ struct test {
> > struct ipv4_pkt ipv4;
> > struct svlan_ipv4_pkt svlan_ipv4;
> > struct ipv6_pkt ipv6;
> > + struct ipv6_frag_pkt ipv6_frag;
> > struct dvlan_ipv6_pkt dvlan_ipv6;
> > } pkt;
> > struct bpf_flow_keys keys;
> > + __u32 flags;
> > };
> >
> > #define VLAN_HLEN 4
> > @@ -143,6 +161,102 @@ struct test tests[] = {
> > .n_proto = __bpf_constant_htons(ETH_P_IPV6),
> > },
> > },
> > + {
> > + .name = "ipv4-frag",
> > + .pkt.ipv4 = {
> > + .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> > + .iph.ihl = 5,
> > + .iph.protocol = IPPROTO_TCP,
> > + .iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
> > + .iph.frag_off = __bpf_constant_htons(IP_MF),
> > + .tcp.doff = 5,
> > + .tcp.source = 80,
> > + .tcp.dest = 8080,
> > + },
> > + .keys = {
> > + .flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG,
> > + .nhoff = ETH_HLEN,
> > + .thoff = ETH_HLEN + sizeof(struct iphdr),
> > + .addr_proto = ETH_P_IP,
> > + .ip_proto = IPPROTO_TCP,
> > + .n_proto = __bpf_constant_htons(ETH_P_IP),
> > + .is_frag = true,
> > + .is_first_frag = true,
> > + .sport = 80,
> > + .dport = 8080,
> > + },
> > + .flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG,
> > + },
> > + {
> > + .name = "ipv4-no-frag",
> > + .pkt.ipv4 = {
> > + .eth.h_proto = __bpf_constant_htons(ETH_P_IP),
> > + .iph.ihl = 5,
> > + .iph.protocol = IPPROTO_TCP,
> > + .iph.tot_len = __bpf_constant_htons(MAGIC_BYTES),
> > + .iph.frag_off = __bpf_constant_htons(IP_MF),
> > + .tcp.doff = 5,
> > + .tcp.source = 80,
> > + .tcp.dest = 8080,
> > + },
> > + .keys = {
> > + .nhoff = ETH_HLEN,
> > + .thoff = ETH_HLEN + sizeof(struct iphdr),
> > + .addr_proto = ETH_P_IP,
> > + .ip_proto = IPPROTO_TCP,
> > + .n_proto = __bpf_constant_htons(ETH_P_IP),
> > + .is_frag = true,
> > + .is_first_frag = true,
> > + },
> > + },
> > + {
> > + .name = "ipv6-frag",
> > + .pkt.ipv6_frag = {
> > + .eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
> > + .iph.nexthdr = IPPROTO_FRAGMENT,
> > + .iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
> > + .ipf.nexthdr = IPPROTO_TCP,
> > + .tcp.doff = 5,
> > + .tcp.source = 80,
> > + .tcp.dest = 8080,
> > + },
> > + .keys = {
> > + .flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG,
> > + .nhoff = ETH_HLEN,
> > + .thoff = ETH_HLEN + sizeof(struct ipv6hdr) +
> > + sizeof(struct frag_hdr),
> > + .addr_proto = ETH_P_IPV6,
> > + .ip_proto = IPPROTO_TCP,
> > + .n_proto = __bpf_constant_htons(ETH_P_IPV6),
> > + .is_frag = true,
> > + .is_first_frag = true,
> > + .sport = 80,
> > + .dport = 8080,
> > + },
> > + .flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG,
> > + },
> > + {
> > + .name = "ipv6-no-frag",
> > + .pkt.ipv6_frag = {
> > + .eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
> > + .iph.nexthdr = IPPROTO_FRAGMENT,
> > + .iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
> > + .ipf.nexthdr = IPPROTO_TCP,
> > + .tcp.doff = 5,
> > + .tcp.source = 80,
> > + .tcp.dest = 8080,
> > + },
> > + .keys = {
> > + .nhoff = ETH_HLEN,
> > + .thoff = ETH_HLEN + sizeof(struct ipv6hdr) +
> > + sizeof(struct frag_hdr),
> > + .addr_proto = ETH_P_IPV6,
> > + .ip_proto = IPPROTO_TCP,
> > + .n_proto = __bpf_constant_htons(ETH_P_IPV6),
> > + .is_frag = true,
> > + .is_first_frag = true,
> > + },
> > + },
> > };
> >
> > static int create_tap(const char *ifname)
> > @@ -225,6 +339,13 @@ void test_flow_dissector(void)
> > .data_size_in = sizeof(tests[i].pkt),
> > .data_out = &flow_keys,
> > };
> > + static struct bpf_flow_keys ctx = {};
> > +
> > + if (tests[i].flags) {
> > + tattr.ctx_in = &ctx;
> > + tattr.ctx_size_in = sizeof(ctx);
> > + ctx.flags = tests[i].flags;
> > + }
> >
> > err = bpf_prog_test_run_xattr(&tattr);
> > CHECK_ATTR(tattr.data_size_out != sizeof(flow_keys) ||
> > @@ -255,6 +376,14 @@ void test_flow_dissector(void)
> > struct bpf_prog_test_run_attr tattr = {};
> > __u32 key = 0;
> >
> > + /* Don't run tests that are not marked as
> > + * FLOW_DISSECTOR_F_PARSE_1ST_FRAG; eth_get_headlen
> > + * sets this flag.
> > + */
> > +
> > + if (tests[i].flags != FLOW_DISSECTOR_F_PARSE_1ST_FRAG)
> > + continue;
>
> Maybe test flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG == 0 instead?
> It is not necessary now, but might be useful in the future.
I'm not sure about this one. We want flags here to match flags
from eth_get_headlen:
const unsigned int flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
...
if (!skb_flow_dissect_flow_keys_basic(..., flags))
Otherwise the test might break unexpectedly. So I'd rather manually
adjust a test here if eth_get_headlen flags change.
Maybe I should clarify the comment to signify that dependency? Because
currently it might be read as if we only care about
FLOW_DISSECTOR_F_PARSE_1ST_FRAG, but we really care about all flags
in eth_get_headlen; it just happens that it only has one right now.
next prev parent reply other threads:[~2019-07-24 23:52 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-24 17:00 [PATCH bpf-next 0/7] bpf/flow_dissector: support input flags Stanislav Fomichev
2019-07-24 17:00 ` [PATCH bpf-next 1/7] bpf/flow_dissector: pass input flags to BPF flow dissector program Stanislav Fomichev
2019-07-24 22:21 ` Song Liu
2019-07-24 22:32 ` Stanislav Fomichev
2019-07-24 22:45 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 2/7] bpf/flow_dissector: document flags Stanislav Fomichev
2019-07-24 22:24 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 3/7] bpf/flow_dissector: support flags in BPF_PROG_TEST_RUN Stanislav Fomichev
2019-07-24 22:33 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 4/7] tools/bpf: sync bpf_flow_keys flags Stanislav Fomichev
2019-07-24 23:17 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 5/7] sefltests/bpf: support FLOW_DISSECTOR_F_PARSE_1ST_FRAG Stanislav Fomichev
2019-07-24 23:21 ` Song Liu
2019-07-24 23:52 ` Stanislav Fomichev [this message]
2019-07-25 5:36 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 6/7] bpf/flow_dissector: support ipv6 flow_label and FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL Stanislav Fomichev
2019-07-24 23:28 ` Song Liu
2019-07-24 17:00 ` [PATCH bpf-next 7/7] selftests/bpf: support FLOW_DISSECTOR_F_STOP_AT_ENCAP Stanislav Fomichev
2019-07-24 23:29 ` Song Liu
2019-07-24 22:10 ` [PATCH bpf-next 0/7] bpf/flow_dissector: support input flags Willem de Bruijn
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=20190724235254.GB3500@mini-arch \
--to=sdf@fomichev.me \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=liu.song.a23@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=ppenkov@google.com \
--cc=sdf@google.com \
--cc=willemb@google.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.