On 12/13/24 17:09, Eelco Chaudron wrote:
>
>
> 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.
>>
>> Otherwise:
>>
>> Acked-by: Ilya Maximets <[email protected]>
>
> Applied to main with nit fixed.
Should this also be backported at least to 3.3 ?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
