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.
>
> 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.
>
> 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_field: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_field:fc00::3-\>ipv6_src,ovs-p0],
> [], [stdout], [stderr])
The ', [], [stdout], [stderr]' part in above commands should
not be needed.
> +
> +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
