Eelco Chaudron <[email protected]> writes: > 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.
ACK - I wasn't considering something like a bare recirc chain mixed into a separate instance of clone key allocations. Okay then, we will need both changes then. A proper clone key allocation that is decoupled from the recursion depth needs to be included with this change. >> 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. I'm not sure increasing the limits is the right path. Keys are already quite large. I think multiplying it would be too much. You're right that incrementing the limit by 3 might be acceptable but I think keeping around the overloaded counter is still a problem. >> 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. Yes. Because the limits are trying to track two things together (recursion depth and the clone-key-pool), it makes it a bit more difficult to make a single helper that does the counting right - we'd need to pass more state from the callers. >> 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. I think having a second allocation routine that uses a separate counter is the path forward, based on the discussion. > 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. We should make sure that the names make sense. Something more like clone_key_next is probably fine - I don't think we need a 'level' as that may be confusing while reviewing. We have the clone_key() to allocate, and maybe we just need something like clone_key_release() to release the last allocated one by decrementing the counter back down. > 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. If we're going to be defining the new counter anyway, we shouldn't try to add it as a stop-gap measure. I think in this case, we should implement it completely the first time (whatever we can agree is right). I think if we introduce the counter for the clone key allocations, we should only advance it when we actually allocate a clone key. This means we don't need to be 1-based for the allocation sematic anymore either. > Have not tried to code this, but seems doable. +1 > //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
