On 6/17/26 2:14 PM, Eelco Chaudron wrote:
>
>
> On 17 Jun 2026, at 12:25, Ilya Maximets wrote:
>
>> TCA_ACT_MAX_NUM limits the actions number from the OVS perspective,
>> i.e., the size of the actions array in tc_flower structure. However,
>> these actions can be represented by multiple actual TC actions in the
>> netlink message. For example, output to internal ports requires extra
>> "skbedit" action to change the packet type before sending it to the
>> "mirred" action. We also emit extra "csum" actions and put tunnel
>> attributes, etc.
>>
>> In the end, the number of actual TC actions is frequently larger
>> than the number of actions in tc_flower structure. Meaning that the
>> number of priorities in use can be larger than TCA_ACT_MAX_NUM.
>>
>> If that happens, we fail to parse the full ECHO reply for the netlink
>> transaction and report a mismatch between requested and acknowledged
>> configuration. The traffic should work fine, but we have the annoying
>> warning in the logs and the revalidators will try updating that one
>> datapath flow triggering more warnings.
>>
>> We need to always expect the full range of priorities while parsing
>> kernel replies. And we need to check that the number of used
>> priorities doesn't actually exceed the TCA_ACT_MAX_PRIO before sending
>> requests to the kernel, otherwise they can be "silently" truncated in
>> the absence of the kernel commit:
>> 5f5f92912b43 ("tc: Return an error if filters try to attach too many
>> actions")
>>
>> Also adding some comments around the indexing, as it is confusing
>> that priorities for TC flower actions are counted from 1, but the
>> action table indexes for the police action are starting from zero.
>> Both using the same constant for the upper bound, but it is inclusive
>> in one case and not in the other.
>>
>> Notes:
>>
>> Technically, there should be no need for the artificial limit imposed
>> by TCA_ACT_MAX_NUM, since the issue it was trying to work around was
>> fixed in the kernel commit:
>> 369609fc6272 ("tc: Ensure we have enough buffer space when sending filter
>> netlink notifications")
>> But that fix is not available in 5.x kernels at the moment.
>>
>> TCA_ACT_MAX_NUM by itself is a wrong concept. It supposed to limit
>> the number of actual TC actions, but it is applied in two different
>> places limiting tc_flower actions array instead, while there is
>> not a 1 to 1 mapping between them and the actual TC actions generated.
>> It sort of works for the most part, but fails when the mapping is
>> significantly imbalanced or the number of actions is on the edge of
>> the message size. To be fair, it was not meant as a precise limit.
>> And we may actually want some limit at offload-tc-netdev level for
>> performance reasons. But the concept is flawed.
>>
>> Fixing the logic around TCA_ACT_MAX_NUM or removing it outright is
>> a little orthogonal to this patch though, as we'd still need both the
>> TCA_ACT_MAX_PRIO check to avoid sending more than 32 actions to the
>> kernel, and the full range of parsing for actions in replies.
>>
>> Fixes: d0fbb09f907b ("tc: Limit the max action number to 16")
>> Fixes: 0c70132cd288 ("tc: Make the actions order consistent")
>> Reported-at: https://redhat.atlassian.net/browse/FDP-4005
>> Signed-off-by: Ilya Maximets <[email protected]>
>
> Thanks for the quick fix, the change looks good to me. One small nit
> about not using the ovs-p1 naming convention, see below.
>
> Acked-by: Eelco Chaudron <[email protected]>
Thanks, Eelco!
>
>> ---
>> lib/tc.c | 9 +++++--
>> tests/system-offloads-traffic.at | 43 ++++++++++++++++++++++++++++++++
>> 2 files changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/tc.c b/lib/tc.c
>> index 4a9c6c267..1ae026b0e 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -2133,7 +2133,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct
>> tc_flower *flower,
>> bool terse)
>> {
>> const struct nlattr *actions = attrs[TCA_FLOWER_ACT];
>> - static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {};
>> + /* Priorities of flower actions start at one: [1, TCA_ACT_MAX_PRIO]. */
>> + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO + 1] =
>> {};
>> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)];
>> const int max_size = ARRAY_SIZE(actions_orders_policy);
>> int previous_action_count = 0;
>> @@ -2402,6 +2403,7 @@ tc_dump_tc_action_start(char *name, struct nl_dump
>> *dump)
>> int
>> parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[])
>> {
>> + /* Action tables are indexed from zero: [0, TCA_ACT_MAX_PRIO - 1]. */
>> static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO] = {};
>> struct ofpbuf b = ofpbuf_const_initializer(reply->data, reply->size);
>> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)];
>> @@ -3540,7 +3542,10 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct
>> tc_flower *flower)
>> }
>> nl_msg_end_nested(request, offset);
>>
>> - return 0;
>> + /* Some tc_actions generate more than one actual TC action. Make sure
>> + * we're within the kernel's limit. 'act_index - 1' is the last used
>> + * priority / index. */
>> + return act_index - 1 <= TCA_ACT_MAX_PRIO ? 0 : -EOPNOTSUPP;
>> }
>>
>> static void
>> diff --git a/tests/system-offloads-traffic.at
>> b/tests/system-offloads-traffic.at
>> index 13a8054a4..55c26a0ce 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -1245,3 +1245,46 @@ AT_CHECK([ovs-appctl --format json --pretty
>> dpif/offload/show \
>>
>> OVS_TRAFFIC_VSWITCHD_STOP
>> AT_CLEANUP
>> +
>> +AT_SETUP([offloads - many output ports - actions limit])
>> +OVS_TRAFFIC_VSWITCHD_START([], [],
>> + [-- set Open_vSwitch . other_config:hw-offload=true])
>> +
>> +AT_CHECK([ovs-appctl vlog/set tc:file:dbg dpif_netlink:file:dbg
>> vconn:file:info])
>> +
>> +m4_for([id], [1], [17], [1], [
>> + ADD_NAMESPACES(atns-id)
>> + ADD_VETH(p-id, atns-id, br0, "10.1.1.id/24")
>> +])
>> +
>> +AT_CHECK([ovs-ofctl add-flow br0 "arp,actions=NORMAL"])
>> +dnl Forward all non-matching IP traffic towards ovs-p-1 (ICMP replies).
>> +AT_CHECK([ovs-ofctl add-flow br0 "priority=0,ip,actions=output:ovs-p-1"])
>
> All other tests use the ovs-p1 naming convention, it would be nice to
> do the same here (not a blocker). The m4 empty-quote trick makes the
> macros look a little odd, but it works correctly.
As discussed off-list, we do actually use this style of port names in ipsec
tests for exactly same reason - more suitable for macro expansion. I really
do not like the brackets trick. It was how I wrote the code initially, but
it was hard to read and it didn't look good overall, so I switched to dashes,
except for one place where it's not that easy to get rid of the brackets.
I went ahead and applied the original version of the patch and backported
it down to 3.3.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
