On 6/1/26 5:47 PM, Eelco Chaudron wrote: > > > On 27 May 2026, at 0:11, Ilya Maximets wrote: > >> While processing OpenFlow clone() action, current code makes decisions >> to skip the datapath level clone() based on incorrect assumption that >> checking immediate OpenFlow actions is enough to make a decision. But >> that is absolutely not true. Some OpenFlow actions may be reversible >> or not reversible depending on which datapath action we'll end up >> using, e.g. dec_ttl can be implemented reversibly by setting a new TTL >> value, or it can be non-reversable if the dec_ttl datapath action is >> used. Also, things like groups, resubmits, goto-table and output to >> patch ports are completely unpredictable as there is no visibility if >> they will produce non-reversible actions or not until they are fully >> translated. But they are marked as reversible for some reason in the >> current code. This causes problems where clones are omitted in cases >> where they must not be, breaking the traffic or applying modifications >> to packets that should not be modified. >> >> There were multiple attempts to add small tweaks here and there to >> make the code more accurate without producing excessive amount of >> clone() actions on the datapath level, which are expensive. But none >> of them addressed the core of the issue - that decisions are made >> based on the wrong information which is OpenFlow action list. And so, >> none of these solutions can fully solve the problem. >> >> To fix this, we need to actually translate all the actions and check >> if the result is reversible or not on the datapath actions level. >> >> There are few tricky cases here: >> >> - All the PUSH/POP/ENCAP/DECAP actions are not reversible as they for >> the most part require packet re-parsing, except for VLAN and MPLS >> ones that can be reverted on xlate commit. However, ADD_MPLS is >> not reversible as it changes the packet type and requires extra >> manipulations with the ethernet header. >> >> - HASH is not reversible, because it sets the hash using the current >> set of headers and we need to re-hash if something changes. There >> is no hash-clear action, so it may be wrongly reused. >> >> - RECIRC is reversible, because datapath will clone whenever it's not >> the last action. >> >> - DEC_TTL is not reversible as the TTL value is not being matched in >> this case and so we don't know to which value to revert and there is >> no INC_TTL. >> >> - CHECK_PKT_LEN doesn't internally clone the packet, so we need to >> recursively check action lists in both branches. >> >> A bunch of tests added to cover most of the cases. >> >> One issue with this change is that in order to be able to make a >> cloning decision after the translation is done, we must commit all the >> pending actions before translating the clone. This way the actions >> for the clone will be the same regardless of wrapping. But it means >> that if there are header updates before the clone and there are some >> inside the clone and we omit the actual clone, then both SET actions >> will remain, potentially changing the Hi Ilya, > > Hi Ilya, > > Thanks for digging into this (again), it seems like this approach is > the right one. > > It took me quite some time to review and understand all the code, but > I feel like it's fine. > > I have one small nit below, not sure if it would cause any real > problems, but better safe than sorry. > > I still need to look at the other patch in the series, but with the > above (below) taken care of: > > Acked-by: Eelco Chaudron <[email protected]> > > [...] > >> - /* Commit datapath actions before emitting the clone action to >> - * avoid emitting those actions twice. Once inside >> - * the clone, another time for the action after clone. */ >> - xlate_commit_actions(ctx); >> + retained_state = xretain_state_save(ctx); >> xretain_base_flow_save(ctx, retained_state); >> >> - bool old_was_mpls = ctx->was_mpls; >> - bool old_conntracked = ctx->conntracked; >> + old_was_mpls = ctx->was_mpls; >> + old_conntracked = ctx->conntracked; >> >> - /* The actions are not reversible, a datapath clone action is >> - * required to encode the translation. Select the clone action >> - * based on datapath capabilities. */ >> - if (ctx->xbridge->support.clone) { /* Use clone action */ >> - /* Use clone action as datapath clone. */ >> - offset = nl_msg_start_nested(ctx->odp_actions, >> OVS_ACTION_ATTR_CLONE); >> - do_xlate_actions(actions, actions_len, ctx, true, false); >> - if (!ctx->freezing) { >> - xlate_action_set(ctx); >> - } >> - if (ctx->freezing) { >> - finish_freezing(ctx); >> - } >> - nl_msg_end_non_empty_nested(ctx->odp_actions, offset); >> - goto dp_clone_done; >> + body_offset = ctx->odp_actions->size; >> + >> + /* Translate the clone body. Pass is_last_action=true to avoid >> + * unnecessary nesting within the clone body itself. */ >> + do_xlate_actions(actions, actions_len, ctx, true, false); >> + if (!ctx->freezing) { >> + xlate_action_set(ctx); >> + } >> + if (ctx->freezing) { >> + finish_freezing(ctx); >> } >> >> - if (ctx->xbridge->support.sample_nesting > 3) { >> - /* Use sample action as datapath clone. */ >> - offset = nl_msg_start_nested(ctx->odp_actions, >> OVS_ACTION_ATTR_SAMPLE); >> - ac_offset = nl_msg_start_nested(ctx->odp_actions, >> - OVS_SAMPLE_ATTR_ACTIONS); >> - do_xlate_actions(actions, actions_len, ctx, true, false); >> - if (!ctx->freezing) { >> - xlate_action_set(ctx); >> - } >> - if (ctx->freezing) { >> - finish_freezing(ctx); >> - } >> - if (nl_msg_end_non_empty_nested(ctx->odp_actions, ac_offset)) { >> - nl_msg_cancel_nested(ctx->odp_actions, offset); >> - } else { >> + body_size = ctx->odp_actions->size - body_offset; >> + >> + if (!is_last_action && body_size > 0 >> + && !odp_actions_are_reversible( >> + (char *) ctx->odp_actions->data + body_offset, body_size)) { >> + size_t observe_shift = 0; >> + >> + if (ctx->xbridge->support.clone) { >> + nl_msg_wrap_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE, >> + body_offset, body_size); >> + observe_shift = NLA_HDRLEN; >> + } else if (ctx->xbridge->support.sample_nesting > 3) { >> + /* Use sample action as datapath clone fallback. */ >> + nl_msg_wrap_nested(ctx->odp_actions, OVS_SAMPLE_ATTR_ACTIONS, >> + body_offset, body_size); >> nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, >> UINT32_MAX); /* 100% probability. */ >> - nl_msg_end_nested(ctx->odp_actions, offset); >> + nl_msg_wrap_nested(ctx->odp_actions, OVS_ACTION_ATTR_SAMPLE, >> + body_offset, >> + ctx->odp_actions->size - body_offset); >> + observe_shift = 2 * NLA_HDRLEN; >> + } else { >> + /* Datapath does not support clone. Discard the clone body >> + * since we cannot isolate its non-reversible effects. */ >> + ctx->odp_actions->size = body_offset; >> + xlate_report_error(ctx, "Failed to compose clone action"); > > > compose_sample_action() might set the last_observe_offset pointing > into the truncated region. Should set it here? > > ctx->xout->last_observe_offset = UINT32_MAX;
Good point. I fixed that by saving the old value and setting it back here. This path should never be taken in practice, but the code should be correct. With that, I applied both patches to main and backported down to 3.3. Thanks! Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
