On 16 Dec 2024, at 12:30, Eelco Chaudron wrote:
> 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.
FYI, backported down to 3.3.
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
