* [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector @ 2020-11-10 17:12 Santucci Pierpaolo 2020-11-11 4:48 ` Andrii Nakryiko 2020-11-16 10:30 ` [PATCH v2] " Santucci Pierpaolo 0 siblings, 2 replies; 10+ messages in thread From: Santucci Pierpaolo @ 2020-11-10 17:12 UTC (permalink / raw) To: shuah, ast, daniel, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf From second fragment on, IPV6FR program must stop the dissection of IPV6 fragmented packet. This is the same approach used for IPV4 fragmentation. Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> --- tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c index 5a65f6b51377..95a5a0778ed7 100644 --- a/tools/testing/selftests/bpf/progs/bpf_flow.c +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) */ if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) return export_flow_keys(keys, BPF_OK); + } else { + return export_flow_keys(keys, BPF_OK); } return parse_ipv6_proto(skb, fragh->nexthdr); -- 2.29.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-10 17:12 [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector Santucci Pierpaolo @ 2020-11-11 4:48 ` Andrii Nakryiko 2020-11-11 11:17 ` Jakub Sitnicki 2020-11-16 10:30 ` [PATCH v2] " Santucci Pierpaolo 1 sibling, 1 reply; 10+ messages in thread From: Andrii Nakryiko @ 2020-11-11 4:48 UTC (permalink / raw) To: Santucci Pierpaolo, Jakub Sitnicki Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo <santucci@epigenesys.com> wrote: > > From second fragment on, IPV6FR program must stop the dissection of IPV6 > fragmented packet. This is the same approach used for IPV4 fragmentation. > Jakub, can you please take a look as well? > Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> > --- > tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c > index 5a65f6b51377..95a5a0778ed7 100644 > --- a/tools/testing/selftests/bpf/progs/bpf_flow.c > +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c > @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) > */ > if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) > return export_flow_keys(keys, BPF_OK); > + } else { > + return export_flow_keys(keys, BPF_OK); > } > > return parse_ipv6_proto(skb, fragh->nexthdr); > -- > 2.29.2 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-11 4:48 ` Andrii Nakryiko @ 2020-11-11 11:17 ` Jakub Sitnicki 2020-11-11 14:12 ` Santucci Pierpaolo 0 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2020-11-11 11:17 UTC (permalink / raw) To: Andrii Nakryiko, Santucci Pierpaolo Cc: Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf On Wed, Nov 11, 2020 at 05:48 AM CET, Andrii Nakryiko wrote: > On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo > <santucci@epigenesys.com> wrote: >> >> From second fragment on, IPV6FR program must stop the dissection of IPV6 >> fragmented packet. This is the same approach used for IPV4 fragmentation. >> > > Jakub, can you please take a look as well? I'm not initimately familiar with this test, but looking at the change I'd consider that Destinations Options and encapsulation headers can follow the Fragment Header. With enough of Dst Opts or levels of encapsulation, transport header could be pushed to the 2nd fragment. So I'm not sure if the assertion from the IPv4 dissector that 2nd fragment and following doesn't contain any parseable header holds. Taking a step back... what problem are we fixing here? > >> Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> >> --- >> tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c >> index 5a65f6b51377..95a5a0778ed7 100644 >> --- a/tools/testing/selftests/bpf/progs/bpf_flow.c >> +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c >> @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) >> */ >> if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) >> return export_flow_keys(keys, BPF_OK); >> + } else { >> + return export_flow_keys(keys, BPF_OK); >> } >> >> return parse_ipv6_proto(skb, fragh->nexthdr); >> -- >> 2.29.2 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-11 11:17 ` Jakub Sitnicki @ 2020-11-11 14:12 ` Santucci Pierpaolo 2020-11-11 23:06 ` Daniel Borkmann 0 siblings, 1 reply; 10+ messages in thread From: Santucci Pierpaolo @ 2020-11-11 14:12 UTC (permalink / raw) To: Jakub Sitnicki Cc: Andrii Nakryiko, Shuah Khan, Alexei Starovoitov, Daniel Borkmann, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf Hi Jakub, thanks for your reply. Let me explain the problem with an example. Please consider the PCAP file: https://github.com/named-data/ndn-tools/blob/master/tests/dissect-wireshark/ipv6-udp-fragmented.pcap Let's assume that the dissector is invoked without the flag: BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL. Without the proposed patch, the flow keys for the second fragment (packet timestamp 0.256997) will contain the value 0x6868 for the source and destination port fields: this is obviously wrong. The same happens for the third fragment (packet timestamp 0.256998) and for the fourth fragment (packet timestamp 0.257001). So it seems that the correct thing to do is to stop the dissector after the IPV6 fragmentation header for all fragments from the second on. Regards, Pierpaolo Santucci On Wed, Nov 11, 2020 at 12:17:06PM +0100, Jakub Sitnicki wrote: > On Wed, Nov 11, 2020 at 05:48 AM CET, Andrii Nakryiko wrote: > > On Tue, Nov 10, 2020 at 9:12 AM Santucci Pierpaolo > > <santucci@epigenesys.com> wrote: > >> > >> From second fragment on, IPV6FR program must stop the dissection of IPV6 > >> fragmented packet. This is the same approach used for IPV4 fragmentation. > >> > > > > Jakub, can you please take a look as well? > > I'm not initimately familiar with this test, but looking at the change > I'd consider that Destinations Options and encapsulation headers can > follow the Fragment Header. > > With enough of Dst Opts or levels of encapsulation, transport header > could be pushed to the 2nd fragment. So I'm not sure if the assertion > from the IPv4 dissector that 2nd fragment and following doesn't contain > any parseable header holds. > > Taking a step back... what problem are we fixing here? > > > > >> Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> > >> --- > >> tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c > >> index 5a65f6b51377..95a5a0778ed7 100644 > >> --- a/tools/testing/selftests/bpf/progs/bpf_flow.c > >> +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c > >> @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) > >> */ > >> if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) > >> return export_flow_keys(keys, BPF_OK); > >> + } else { > >> + return export_flow_keys(keys, BPF_OK); > >> } > >> > >> return parse_ipv6_proto(skb, fragh->nexthdr); > >> -- > >> 2.29.2 > >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-11 14:12 ` Santucci Pierpaolo @ 2020-11-11 23:06 ` Daniel Borkmann 2020-11-12 9:00 ` Jakub Sitnicki 0 siblings, 1 reply; 10+ messages in thread From: Daniel Borkmann @ 2020-11-11 23:06 UTC (permalink / raw) To: Santucci Pierpaolo, Jakub Sitnicki Cc: Andrii Nakryiko, Shuah Khan, Alexei Starovoitov, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf, sdf On 11/11/20 3:12 PM, Santucci Pierpaolo wrote: > Hi Jakub, > > thanks for your reply. (Santucci, please do not top-post but always reply inline which makes it easier for discussions to follow.) > Let me explain the problem with an example. > > Please consider the PCAP file: > https://github.com/named-data/ndn-tools/blob/master/tests/dissect-wireshark/ipv6-udp-fragmented.pcap > Let's assume that the dissector is invoked without the flag: > BPF_FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL. > > Without the proposed patch, the flow keys for the second fragment (packet > timestamp 0.256997) will contain the value 0x6868 for the source and > destination port fields: this is obviously wrong. > The same happens for the third fragment (packet timestamp 0.256998) and for > the fourth fragment (packet timestamp 0.257001). > > So it seems that the correct thing to do is to stop the dissector after the > IPV6 fragmentation header for all fragments from the second on. > [...] >> >> I'm not initimately familiar with this test, but looking at the change >> I'd consider that Destinations Options and encapsulation headers can >> follow the Fragment Header. >> >> With enough of Dst Opts or levels of encapsulation, transport header >> could be pushed to the 2nd fragment. So I'm not sure if the assertion >> from the IPv4 dissector that 2nd fragment and following doesn't contain >> any parseable header holds. Hm, staring at rfc8200, it says that the first fragment packet must include the upper-layer header (e.g. tcp, udp). The patch here should probably add a comment wrt to the rfc. Thanks, Daniel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-11 23:06 ` Daniel Borkmann @ 2020-11-12 9:00 ` Jakub Sitnicki 2020-11-13 0:50 ` John Fastabend 0 siblings, 1 reply; 10+ messages in thread From: Jakub Sitnicki @ 2020-11-12 9:00 UTC (permalink / raw) To: Daniel Borkmann, Santucci Pierpaolo Cc: Andrii Nakryiko, Shuah Khan, Alexei Starovoitov, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf, sdf On Thu, Nov 12, 2020 at 12:06 AM CET, Daniel Borkmann wrote: [...] >>> I'm not initimately familiar with this test, but looking at the change >>> I'd consider that Destinations Options and encapsulation headers can >>> follow the Fragment Header. >>> >>> With enough of Dst Opts or levels of encapsulation, transport header >>> could be pushed to the 2nd fragment. So I'm not sure if the assertion >>> from the IPv4 dissector that 2nd fragment and following doesn't contain >>> any parseable header holds. > > Hm, staring at rfc8200, it says that the first fragment packet must include > the upper-layer header (e.g. tcp, udp). The patch here should probably add a > comment wrt to the rfc. You're right, it clearly says so. Nevermind my worries about malformed packets then. Change LGTM: Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-12 9:00 ` Jakub Sitnicki @ 2020-11-13 0:50 ` John Fastabend 0 siblings, 0 replies; 10+ messages in thread From: John Fastabend @ 2020-11-13 0:50 UTC (permalink / raw) To: Jakub Sitnicki, Daniel Borkmann, Santucci Pierpaolo Cc: Andrii Nakryiko, Shuah Khan, Alexei Starovoitov, Martin Lau, Song Liu, Yonghong Song, Andrii Nakryiko, john fastabend, KP Singh, Networking, bpf, sdf Jakub Sitnicki wrote: > On Thu, Nov 12, 2020 at 12:06 AM CET, Daniel Borkmann wrote: > > [...] > > >>> I'm not initimately familiar with this test, but looking at the change > >>> I'd consider that Destinations Options and encapsulation headers can > >>> follow the Fragment Header. > >>> > >>> With enough of Dst Opts or levels of encapsulation, transport header > >>> could be pushed to the 2nd fragment. So I'm not sure if the assertion > >>> from the IPv4 dissector that 2nd fragment and following doesn't contain > >>> any parseable header holds. > > > > Hm, staring at rfc8200, it says that the first fragment packet must include > > the upper-layer header (e.g. tcp, udp). The patch here should probably add a > > comment wrt to the rfc. > > You're right, it clearly says so. Nevermind my worries about malformed > packets then. Change LGTM: > > Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> Also please add some of the details discussed here to the commit msg so we can remember this next time. Thanks! ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-10 17:12 [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector Santucci Pierpaolo 2020-11-11 4:48 ` Andrii Nakryiko @ 2020-11-16 10:30 ` Santucci Pierpaolo 2020-11-16 12:18 ` Jakub Sitnicki 2020-11-16 16:10 ` patchwork-bot+netdevbpf 1 sibling, 2 replies; 10+ messages in thread From: Santucci Pierpaolo @ 2020-11-16 10:30 UTC (permalink / raw) To: shuah, ast, daniel, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf From second fragment on, IPV6FR program must stop the dissection of IPV6 fragmented packet. This is the same approach used for IPV4 fragmentation. This fixes the flow keys calculation for the upper-layer protocols. Note that according to RFC8200, the first fragment packet must include the upper-layer header. Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> --- v2: extend the commit message, as suggested by John Fastabend <john.fastabend@gmail.com> tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c index 5a65f6b51377..95a5a0778ed7 100644 --- a/tools/testing/selftests/bpf/progs/bpf_flow.c +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) */ if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) return export_flow_keys(keys, BPF_OK); + } else { + return export_flow_keys(keys, BPF_OK); } return parse_ipv6_proto(skb, fragh->nexthdr); -- 2.17.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-16 10:30 ` [PATCH v2] " Santucci Pierpaolo @ 2020-11-16 12:18 ` Jakub Sitnicki 2020-11-16 16:10 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 10+ messages in thread From: Jakub Sitnicki @ 2020-11-16 12:18 UTC (permalink / raw) To: Santucci Pierpaolo Cc: shuah, ast, daniel, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh, netdev, bpf On Mon, Nov 16, 2020 at 11:30 AM CET, Santucci Pierpaolo wrote: > From second fragment on, IPV6FR program must stop the dissection of IPV6 > fragmented packet. This is the same approach used for IPV4 fragmentation. > This fixes the flow keys calculation for the upper-layer protocols. > Note that according to RFC8200, the first fragment packet must include > the upper-layer header. > > Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> > --- > v2: extend the commit message, as suggested by John Fastabend > <john.fastabend@gmail.com> > > tools/testing/selftests/bpf/progs/bpf_flow.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/testing/selftests/bpf/progs/bpf_flow.c b/tools/testing/selftests/bpf/progs/bpf_flow.c > index 5a65f6b51377..95a5a0778ed7 100644 > --- a/tools/testing/selftests/bpf/progs/bpf_flow.c > +++ b/tools/testing/selftests/bpf/progs/bpf_flow.c > @@ -368,6 +368,8 @@ PROG(IPV6FR)(struct __sk_buff *skb) > */ > if (!(keys->flags & BPF_FLOW_DISSECTOR_F_PARSE_1ST_FRAG)) > return export_flow_keys(keys, BPF_OK); > + } else { > + return export_flow_keys(keys, BPF_OK); > } > > return parse_ipv6_proto(skb, fragh->nexthdr); Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] selftest/bpf: fix IPV6FR handling in flow dissector 2020-11-16 10:30 ` [PATCH v2] " Santucci Pierpaolo 2020-11-16 12:18 ` Jakub Sitnicki @ 2020-11-16 16:10 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 10+ messages in thread From: patchwork-bot+netdevbpf @ 2020-11-16 16:10 UTC (permalink / raw) To: Santucci Pierpaolo Cc: shuah, ast, daniel, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh, netdev, bpf Hello: This patch was applied to bpf/bpf-next.git (refs/heads/master): On Mon, 16 Nov 2020 11:30:37 +0100 you wrote: > >From second fragment on, IPV6FR program must stop the dissection of IPV6 > fragmented packet. This is the same approach used for IPV4 fragmentation. > This fixes the flow keys calculation for the upper-layer protocols. > Note that according to RFC8200, the first fragment packet must include > the upper-layer header. > > Signed-off-by: Santucci Pierpaolo <santucci@epigenesys.com> > > [...] Here is the summary with links: - [v2] selftest/bpf: fix IPV6FR handling in flow dissector https://git.kernel.org/bpf/bpf-next/c/024cd2cbd1ca You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-16 16:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-10 17:12 [PATCH] selftest/bpf: fix IPV6FR handling in flow dissector Santucci Pierpaolo 2020-11-11 4:48 ` Andrii Nakryiko 2020-11-11 11:17 ` Jakub Sitnicki 2020-11-11 14:12 ` Santucci Pierpaolo 2020-11-11 23:06 ` Daniel Borkmann 2020-11-12 9:00 ` Jakub Sitnicki 2020-11-13 0:50 ` John Fastabend 2020-11-16 10:30 ` [PATCH v2] " Santucci Pierpaolo 2020-11-16 12:18 ` Jakub Sitnicki 2020-11-16 16:10 ` patchwork-bot+netdevbpf
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.