> -----Original Message-----
> From: Ilya Maximets <i.maxim...@ovn.org>
> Sent: Tuesday, June 7, 2022 1:14 PM
> To: Salem Sol <sal...@nvidia.com>; d...@openvswitch.org
> Cc: Eli Britstein <el...@nvidia.com>; Michael Pattrick <m...@redhat.com>;
> i.maxim...@ovn.org
> Subject: Re: [ovs-dev] [PATCH 1/1] packets: Re-calculate IPv6 checksum only
> for last frags upon modify.
>
> External email: Use caution opening links or attachments
>
>
> On 6/6/22 12:12, Salem Sol via dev wrote:
> > In case of modifying an IPv6 packet src/dst address the checksum
> > should be recalculated only for the last frag. Currently it's done for
> > all frags, leading to incorrect reassembled packet checksum.
> > Fix it by adding a new flag to recalculate the checksum only for last frag.
>
> Hi. Thanks for working on this!
>
> There is definitely a problem with checksum calculation, but I don't
> understand the logic of the fix. We're talking about re-calculation of the L4
> checksum, but the *last* fragment should not contain the L4 header. So,
> we're just corrupting the data of the last fragment. If we want to
> re-calculate
> the checksum, we should do that for the *first* fragment instead, because
> it's the only fragment that will likely contain the L4 header. Or am I
> missing
> something?
>
> BTW, it looks like we have the same issue for ipv4 packets. We should
> update checksum only for the first fragment. This can be a separate fix
> though.
>
> Some more comments inline.
>
> Best regards, Ilya Maximets.
>
Thanks for the catch, you're right, my bad, in my testing I used only one
extension header and the original patchset seemed to resolve the issue.
New implementation can be found in V3.
> >
> > Fixes: bc7a5acdff08 ("datapath: add ipv6 'set' action")
> > Signed-off-by: Salem Sol <sal...@nvidia.com>
> > Acked-by: Mike Pattrick <m...@redhat.com>
> > ---
> > lib/packets.c | 20 ++++++++++++++++++--
> > tests/system-traffic.at | 35 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/packets.c b/lib/packets.c index d0fba8176..b7aef41a5
> > 100644
> > --- a/lib/packets.c
> > +++ b/lib/packets.c
> > @@ -1148,6 +1148,18 @@ packet_set_ipv4_addr(struct dp_packet
> *packet,
> > put_16aligned_be32(addr, new_addr); }
> >
> > +static bool
> > +packet_is_last_ipv6_frag(struct dp_packet *packet) {
> > + const struct ovs_16aligned_ip6_frag *frag_hdr;
> > + uint8_t *data = dp_packet_l3(packet);
> > +
> > + data += sizeof (struct ovs_16aligned_ip6_hdr);
> > + frag_hdr = ALIGNED_CAST(struct ovs_16aligned_ip6_frag *, data);
> > + return (frag_hdr->ip6f_offlg & IP6F_OFF_MASK) &&
> > + !(frag_hdr->ip6f_offlg & IP6F_MORE_FRAG); }
> > +
> > /* Returns true, if packet contains at least one routing header where
> > * segements_left > 0.
> > *
> > @@ -1334,17 +1346,21 @@ packet_set_ipv6(struct dp_packet *packet,
> > const struct in6_addr *src, {
> > struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet);
> > uint8_t proto = 0;
> > + bool recalc_csum;
> > bool rh_present;
> >
> > rh_present = packet_rh_present(packet, &proto);
> > + recalc_csum = nh->ip6_nxt == IPPROTO_FRAGMENT ?
> > + packet_is_last_ipv6_frag(packet) : true;
>
> The fragmentation header is usually the last extension header, not the first.
> This check will work for the simple case with only one extension header, but
> it will not work in the general case. We should search for the fragmentation
> header the same way as we look for the routing header in
> packet_rh_present().
> We should, probably, modify the packet_rh_present() function to also say if
> the packet is fragmented / is it the first fragment.
>
Thanks for the comment, I followed your suggestion and changed
packet_rh_present() to update if it's the first frag in V3.
> >
> > if (memcmp(&nh->ip6_src, src, sizeof(ovs_be32[4]))) {
> > - packet_set_ipv6_addr(packet, proto, nh->ip6_src.be32, src, true);
> > + packet_set_ipv6_addr(packet, proto, nh->ip6_src.be32,
> > + src, recalc_csum);
> > }
> >
> > if (memcmp(&nh->ip6_dst, dst, sizeof(ovs_be32[4]))) {
> > packet_set_ipv6_addr(packet, proto, nh->ip6_dst.be32, dst,
> > - !rh_present);
> > + !rh_present && recalc_csum);
> > }
> >
> > packet_set_ipv6_tc(&nh->ip6_flow, key_tc); diff --git
> > a/tests/system-traffic.at b/tests/system-traffic.at index
> > 239105e89..16ba42d84 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -192,6 +192,41 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i
> > 0.3 -w 2 fc00:1::2 | FORMAT_PI OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> >
> > +AT_SETUP([datapath - ping6 between two ports with header modify])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +
> > +ADD_VETH(p0, at_ns0, br0, "fc00::1/96", e4:11:22:33:44:55)
> > +ADD_VETH(p1, at_ns1, br0, "fc00::2/96", e4:11:22:33:44:54)
> > +NS_CHECK_EXEC([at_ns0], [ip -6 neigh add fc00::3 lladdr
> > +e4:11:22:33:44:54 dev p0]) NS_CHECK_EXEC([at_ns0], [ip -6 neigh add
> > +fc00::2 lladdr e4:11:22:33:44:54 dev p0]) NS_CHECK_EXEC([at_ns0], [ip
> > +-6 neigh add fc00::1 lladdr e4:11:22:33:44:55 dev p0])
> > +
> > +dnl Linux seems to take a little time to get its IPv6 stack in order.
> > +Without dnl waiting, we get occasional failures due to the following error:
> > +dnl "connect: Cannot assign requested address"
> > +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping6 -c 1 fc00::2])
> > +
> > +AT_CHECK([ovs-ofctl del-flows -OOpenFlow15 br0], [], [stdout],
> > +[stderr]) AT_CHECK([ovs-ofctl add-flow -OOpenFlow15 br0
> > +in_port=ovs-p0,ipv6,ipv6_dst=fc00::3,ipv6_src=fc00::1,actions=set_fie
> > +ld:fc00::2-\>ipv6_dst,ovs-p1], [], [stdout], [stderr])
> > +AT_CHECK([ovs-ofctl add-flow -OOpenFlow15 br0
> > +in_port=ovs-p1,ipv6,ipv6_dst=fc00::1,ipv6_src=fc00::2,actions=set_fie
> > +ld:fc00::3-\>ipv6_src,ovs-p0], [], [stdout], [stderr])
>
> The ', [], [stdout], [stderr]' part in above commands should not be needed.
ACK.
>
> > +
> > +NS_CHECK_EXEC([at_ns0], [ping6 -q -c 3 -i 0.3 -w 2 fc00::3 |
> > +FORMAT_PING], [0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +NS_CHECK_EXEC([at_ns0], [ping6 -s 1600 -q -c 3 -i 0.3 -w 2 fc00::3 |
> > +FORMAT_PING], [0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -w 2 fc00::3 |
> > +FORMAT_PING], [0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +
> > +OVS_TRAFFIC_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > AT_SETUP([datapath - ping over bond])
> > OVS_TRAFFIC_VSWITCHD_START()
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
