On 12/10/24 13:42, Eelco Chaudron wrote:
> While offloading header modifications to TC, OVS is using {TCA_PEDIT} +
> {TCA_CSUM} combination as that it the only way to represent header
> rewrite. However, {TCA_CSUM} is unable to calculate L4 checksums for
> IP fragments.
>
> Since TC already applies fragmentation bit masking, this patch simply
> needs to prevent these packets from being processed through TC.
>
> Reported-at: https://issues.redhat.com/browse/FDP-545
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
> v4: - When no mask is present for IP fragment bits do not offload.
> v3: - Fixed some comment style issue.
> - Add the nw_frag mask if not set.
> v2: - Fixed and added some comments.
> - Use ovs-pcap to compare packets.
> ---
> lib/netdev-offload-tc.c | 39 +++++++++++++++++
> lib/tc.c | 5 ++-
> tests/system-traffic.at | 92 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 135 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 3be1c08d2..3d917e7d9 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1490,6 +1490,31 @@ parse_put_flow_ct_action(struct tc_flower *flower,
> return 0;
> }
>
> +/* This function returns true if the tc layer will add a l4 checksum action
> + * for this set action. Refer to the csum_update_flag() function for
> + * detailed logic. Note that even the kernel only supports updating TCP,
> + * UDP and ICMPv6.
> + */
> +static bool
> +tc_will_add_l4_checksum(struct tc_flower *flower, int type)
> +{
> + switch (type) {
> + case OVS_KEY_ATTR_IPV4:
> + case OVS_KEY_ATTR_IPV6:
> + case OVS_KEY_ATTR_TCP:
> + case OVS_KEY_ATTR_UDP:
> + switch (flower->key.ip_proto) {
> + case IPPROTO_TCP:
> + case IPPROTO_UDP:
> + case IPPROTO_ICMPV6:
> + case IPPROTO_UDPLITE:
> + return true;
> + }
> + break;
> + }
> + return false;
> +}
> +
> static int
> parse_put_flow_set_masked_action(struct tc_flower *flower,
> struct tc_action *action,
> @@ -1522,6 +1547,14 @@ parse_put_flow_set_masked_action(struct tc_flower
> *flower,
> return EOPNOTSUPP;
> }
>
> + if (flower->key.flags & TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT
> + && tc_will_add_l4_checksum(flower, type)) {
> + VLOG_DBG_RL(&rl, "set action type %d not supported on fragments "
> + "due to checksum limitation", type);
> + ofpbuf_uninit(&set_buf);
> + return EOPNOTSUPP;
> + }
> +
> for (i = 0; i < ARRAY_SIZE(set_flower_map[type]); i++) {
> struct netlink_field *f = &set_flower_map[type][i];
>
> @@ -2447,6 +2480,12 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
>
> mask->nw_frag = 0;
> + } else {
> + /* This scenario should not occur. Currently, all installed IP DP
> + * flows perform a fully masked match on the fragmentation bits.
> + * However, since TC depends on this behavior, we return ENOTSUPP
> + * for now in case this behavior changes in the future. */
> + return EOPNOTSUPP;
Nit: Double spaces between sentences.
Otherwise:
Acked-by: Ilya Maximets <[email protected]>
Thanks!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
