On 29 Jul 2026, at 11:56, Ilya Maximets wrote:
> On 7/28/26 11:01 PM, Aaron Conole wrote:
>> Ilya Maximets <[email protected]> writes:
>>
>>> On 7/28/26 4:51 PM, Aaron Conole wrote:
>>>> Hi Yang,
>>>>
>>>> yang zhuorao <[email protected]> writes:
>>>>
>>>>> clone_execute() only increments and decrements exec_level when
>>>>> clone_flow_key is true. When clone_flow_key is false the optimized
>>>>> path reuses the original flow key and calls do_execute_actions()
>>>>> directly without updating the recursion counter, making those layers
>>>>> invisible to the OVS_RECURSION_LIMIT check in ovs_execute_actions().
>>>>>
>>>>> A single action tree allows up to OVS_COPY_ACTIONS_MAX_DEPTH (16)
>>>>> nested clone actions. When the innermost clone contains a RECIRC
>>>>> that selects another flow with its own 16-layer validation budget,
>>>>> multiple trees of unaccounted recursion can be stacked. The deferred
>>>>> action threshold (OVS_DEFERRED_ACTION_THRESHOLD =
>>>>> OVS_RECURSION_LIMIT - 2 = 3) allows three synchronous recirc levels
>>>>> before clone_key() forces deferral, yielding 3 x 16 = 48 layers of
>>>>> synchronous, unaccounted clone recursion.
>>>>>
>>>>> Per-layer stack usage is approximately 296 bytes (from disassembly:
>>>>> do_execute_actions() allocates 0x98 bytes, clone_execute() allocates
>>>>> 0x20 bytes, plus saved registers and return address). 48 layers
>>>>> consume ~14,208 bytes before recirc, flow lookup, Netlink, and base
>>>>> call frames, exceeding the 16 KiB x86_64 task stack and hitting the
>>>>> guard page.
>>>>>
>>>>> An unprivileged user can trigger this from a private user/net
>>>>> namespace (clone(CLONE_NEWUSER | CLONE_NEWNET)) by installing three
>>>>> flow rules each with 16 nested clone actions linked by RECIRC, then
>>>>> injecting a single packet via OVS_PACKET_CMD_EXECUTE. The result is
>>>>> a kernel panic:
>>>>>
>>>>> BUG: TASK stack guard page was hit ...
>>>>> CPU: 0 UID: 1000 PID: 85 ... 7.2.0-rc5+
>>>>> RIP: 0010:clone_execute+0x5a/0x2c0 [openvswitch]
>>>>> [do_execute_actions / clone_execute alternating repeatedly]
>>>>> Kernel panic - not syncing: Fatal exception in interrupt
>>>>>
>>>>> Fix this by unconditionally incrementing exec_level for all
>>>>> synchronous clone recursion edges and checking OVS_RECURSION_LIMIT
>>>>> before calling do_execute_actions(). When the limit is exceeded the
>>>>> packet is dropped and -ENETDOWN is returned, consistent with the
>>>>> existing handling in ovs_execute_actions().
>>>>>
>>>>> Tested on Linux 7.2.0-rc5+ (mainline HEAD 62cc90241548), x86_64,
>>>>> CONFIG_VMAP_STACK=y, CONFIG_OPENVSWITCH=m, isolated QEMU guest.
>>>>> Confirmed that the PoC (./poc_clone_overflow 2 16 3, run as UID 1000)
>>>>> no longer triggers a panic with this patch applied.
>>>>>
>>>>> Fixes: b233504033db ("openvswitch: kernel datapath clone action")
>>>>> Cc: [email protected]
>>>>> Reported-by: yang zhuorao <[email protected]>
>>>>> Signed-off-by: yang zhuorao <[email protected]>
>>>>> ---
>>>>> Reproducer (C, single file, available upon request):
>>>>>
>>>>> gcc -O2 -Wall -o poc_clone_overflow poc_clone_overflow.c
>>>>> ./poc_clone_overflow 2 16 3 # as unprivileged user
>>>>>
>>>>> The PoC creates a private user/net namespace, installs three flows
>>>>> each with 16 nested clone actions linked by RECIRC, and injects a
>>>>> triggering packet. Prerequisites: CONFIG_OPENVSWITCH=m/y loaded,
>>>>> unprivileged user namespace creation allowed.
>>>>>
>>>>> Verified unfixed in Linus mainline (62cc90241548), net (97ac08560d23),
>>>>> and net-next (a50eba1e778a) as of 2026-07-27.
>>>>>
>>>>> net/openvswitch/actions.c | 13 +++++++++----
>>>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>>>
>>>>> --
>>>>
>>>> General - please remember to CC the netdev maintainers as well.
>>>>
>>>> [...]
>>>>
>>>>> @@ -1497,14 +1497,19 @@ static int clone_execute(struct datapath *dp,
>>>>> struct sk_buff *skb,
>>>>> if (clone) {
>>>>> int err = 0;
>>>>> if (actions) { /* Sample action */
>>>>> - if (clone_flow_key)
>>>>> - __this_cpu_inc(ovs_pcpu_storage->exec_level);
>>>>> + __this_cpu_inc(ovs_pcpu_storage->exec_level);
>>>>> +
>>>>> + if
>>>>> (unlikely(__this_cpu_read(ovs_pcpu_storage->exec_level) >
>>>>> + OVS_RECURSION_LIMIT)) {
>>>>> + __this_cpu_dec(ovs_pcpu_storage->exec_level);
>>>>> + ovs_kfree_skb_reason(skb,
>>>>> OVS_DROP_RECURSION_LIMIT);
>>>>> + return -ENETDOWN;
>>>>> + }
>>>>
>>>> There is an existing check for recursion in ovs_execute_actions() and in
>>>> there we have a log before dropping:
>>>>
>>>> net_crit_ratelimited("ovs: recursion limit reached on datapath %s,
>>>> probable configuration error\n",
>>>> ovs_dp_name(dp));
>>>>
>>>> We should have a similar log here so we're not silently dropping. But,
>>>> since we now have two places where this check occurs (and it's
>>>> complicated because of how deferred actions is processed), maybe it's
>>>> better to have a function that does the management:
>>>>
>>>> static int ovs_exec_level_enter(struct datapath *dp, struct sk_buff
>>>> *skb) {
>>>> int level;
>>>>
>>>> level = __this_cpu_inc_return(ovs_pcpu_storage->exec_level);
>>>> if (unlikely(level > OVS_RECURSION_LIMIT)) {
>>>> net_crit_ratelimited("ovs: recursion limit reached on datapath
>>>> %s, probable configuration error\n",
>>>> ovs_dp_name(dp));
>>>> ovs_kfree_skb_reason(skb, OVS_DROP_RECURSION_LIMIT);
>>>> return -ENETDOWN;
>>>> }
>>>> return 0;
>>>> }
>>>>
>>>> static void ovs_exec_level_exit(void) {
>>>> __this_cpu_dec(ovs_pcpu_storage->exec_level);
>>>> }
>>>>
>>>> Then just put the enter/exit calls in the clone_execute() and
>>>> ovs_execute_actions() spaces. That way if we need to adjust this limit
>>>> in the future, it's consolidated to just one place.
>>>>
>>>
>>>
>>> There is a different issue here though. The exec_level is used as a
>>> counter for the flow keys during the clone. And if we increment it
>>> unconditionally while not allocating new keys, we'll ran out of key
>>> slots much faster without actually using them. This will lead to
>>> much more actions to be deferred and packets dropped in the end.
>>> There is already a mismatch here as ovs_execute_actions() increments
>>> the level without allocating the key, but doing so also for non-recirc
>>> cases would significantly amplify the problem.
>>
>> That's an additional concern, but the code path at least currently
>> handles the event that we advance beyond the clone flow key per-cpu
>> array. This change makes a low-likelihood scenario a bit worse (the
>> case where we have lots of clones + sub-actions). I don't think it
>> needs to be addressed with this work since this is a security fix, but
>> we should address it via a follow up change (and I can work on that if
>> needed). AFAICT the affected actions are:
>>
>> - sample()
>> - clone()
>> - dec_ttl()
>> - check_pkt_len()
>>
>> I think in most deployments I've seen they only nest once or twice, and
>> we should have 3 'nests' worth of clone flow keys to handle it, meaning
>> that the increased pressure should still fit within the slots we have
>> (we just wouldn't support another nest, which is different from today):
>> ie: nest 1, we are 'taking' slot 2 (key[1]), and nest 2 we are 'taking'
>> slot 3 (key[2]), which leaves no more available.
>>
>> For example, check_pkt_len(..., check_pkt_len(...)) would still work as
>> long as there's no sample() wrapped around it, or a dec_ttl within it,
>> or some other more complex scenario. And it seems to be very heavy
>> handed to ask for a fix that includes both the stack overflow and the
>> clone allocation change together.
>
> While direct nesting is indeed not common, it is very common to have
> recirculation actions inside the clones or check_pkt_len, e.g.:
>
> check_pkt_len(1514, le(recirc(X)), gt(recirc(Y)))
>
> Before this patch we'd only count one level, which is recirculation
> itself. After this change it will be two levels.
>
> A lot of production setups hover at 3-4 recirculations on certain
> packet paths, e.g. pod-to-external traffic in ovn-kubernetes.
> And these setups do have at least a few clones or check_pkt_len
> actions on these paths. And so, if we start counting plain clones
> towards the limit, we could easily break real world deployments,
> given the limit is just 5.
>
> So, we need to reconsider the limits here. But we also can't blindly
> increase the limit as it will increase per-CPU memory consumption
> since that number dictates the size of the storage for keys, which
> are large.
>
>>
>>> We likely need to decouple the recursion level couter from the slot
>>> index in the keys array. Though they still should stay related as
>>> we should start deferring when we approach recursion limit even if
>>> we still have some free slots for key clones.
>>
>> Agreed, we need to have two discreet operations - one to track the stack
>> recursion and one to do the clone flow key allocation. Unless you feel
>> strongly that they should come as part of the same series, but it seems
>> a lot.
> I think, it should be done within this patch, because otherwise we'll
> be most likely breaking existing deployments.
>
> We need a few things:
>
> 1. Decouple the key array index from the execution level.
>
> 2. Potentially increase the recursion limit, since it is going to count
> more things. x2 might be a safe choice, but it is also quite a lot
> and we're risking to ran out of stack in other scenarios. Something
> like +3 may be a change to explore.
>
> We will need extra testing here to make sure we're not breaking
> existing setups. I could try and check ovn-kubernetes deployments,
> as an example of the most demanding configuration, but it will take
> a bit of time.
>
> 3. Sahiko also points out that we should not be dropping packets in
> case we reached the limit inside the clone_execute(), we should
> attempt to defer the actions first. In which case we should not
> log that the limit is reached.
>
> WDYT?
>
> Eelco, do you have some thoughts on this?
I agree we should not blindly remove the 'if (clone_flow_key)' guard from
the exec_level increment and assume everything will work correctly. We
have seen issues with this code in the past.
One approach would be to introduce a second counter, clone_nest_level,
to take over the role that exec_level currently plays for key/slot
allocation and deferral triggering. exec_level could then be incremented
unconditionally for all synchronous do_execute_actions() calls, serving
as a stack-depth safeguard, while clone_nest_level preserves the existing
deferral semantics.
For this patch, clone_nest_level could mirror the current exec_level
behaviour, i.e., increment when clone_flow_key is true, use the value as
the index into the flow_keys slot array, and defer when the slot array is
exhausted. That would close the stack overflow window without changing
deferral behaviour for existing deployments.
As a follow-up patch we could refine clone_nest_level so that it only
advances when a key slot is actually allocated rather than on every
clone_flow_key=true entry.
Have not tried to code this, but seems doable.
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
