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;
> }
> - goto dp_clone_done;
> - }
>
> - /* Datapath does not support clone, skip xlate 'oc' and
> - * report an error */
> - xlate_report_error(ctx, "Failed to compose clone action");
> + if (ctx->xout->last_observe_offset != UINT32_MAX
> + && ctx->xout->last_observe_offset >= body_offset) {
> + ctx->xout->last_observe_offset += observe_shift;
> + }
>
> -dp_clone_done:
> - /* The clone's conntrack execution should have no effect on the original
> - * packet. */
> - ctx->conntracked = old_conntracked;
> + /* Datapath's clone isolates all packet modifications, so restore
> + * base_flow to match the packet's actual state after the clone. */
> + xretain_base_flow_restore(ctx, retained_state);
> + }
> + /* When not wrapped, base_flow is NOT restored: it reflects the packet
> + * state after the inlined actions, so the commit mechanism will emit
> + * reverse actions to undo them. */
>
> - /* Popping MPLS from the clone should have no effect on the original
> - * packet. */
> + /* The clone's conntrack and MPLS state changes should have no
> + * effect on the original packet. */
> + ctx->conntracked = old_conntracked;
> ctx->was_mpls = old_was_mpls;
>
> - /* Restore the 'base_flow' for the next action. */
> - xretain_base_flow_restore(ctx, retained_state);
> -
> -xlate_done:
> xretain_state_restore_and_free(ctx, retained_state);
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
