On 13 Dec 2024, at 17:41, Ilya Maximets wrote:
> 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 ?
I’m fine with backporting this, was just not sure as it is a behavior change
(which should fix the behavior as it should be failing as is).
If no one objects, I’ll backport it later this week.
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
