On 12 Dec 2024, at 23:28, Ilya Maximets wrote:
> 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.
Thanks Ilya, this is something I’ll never learn :)
> Otherwise:
>
> Acked-by: Ilya Maximets <[email protected]>
>
> Thanks!
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
