On 1/3/22 11:19, Eelco Chaudron wrote: > Hi Ilya, > > As you reviewed the v1, I’m wondering if this can get into the next release?
Hi. Sorry, this one fell through the cracks. It seems to be a bug fix, so we'll need to backport it anyway. I'll try to get to this patch in a near future. Best regards, Ilya Maximets. > > I've also added the original authors of the fixes patches; maybe they can > review? > > Eelco > > On 21 Jun 2021, at 11:20, Eelco Chaudron wrote: > >> For patch ports, the is_last_action value is not propagated and is >> always set to true. This causes non-reversible actions to modify the >> packet, and the original content is not preserved when processing >> the remaining actions. >> >> This patch propagates the is_last_action flag for patch port related >> actions. In addition, it also fixes a general last action propagation >> to the individual actions. >> >> Fixed check_pkt_larger as last action, as it is a valid case for the >> drop action, so it should not be skipped. >> >> Fixes: feee58b95 ("ofproto-dpif-xlate: Keep track of the last action") >> Fixes: 5b34f8fc3 ("Add a new OVS action check_pkt_larger") >> Signed-off-by: Eelco Chaudron <echau...@redhat.com> >> >> --- >> v2: Fixed additional last action propagation to individual actions. >> Fixed problem with the check_pkt_larger now that that the correct >> last action state is passed. >> >> ofproto/ofproto-dpif-xlate.c | 27 ++++++++++++--------------- >> tests/ofproto-dpif.at | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 40 insertions(+), 15 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 7108c8a30..7b35f6a32 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -460,7 +460,7 @@ static void xlate_commit_actions(struct xlate_ctx *ctx); >> >> static void >> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, >> - struct xport *out_dev); >> + struct xport *out_dev, bool is_last_action); >> >> static void >> ctx_trigger_freeze(struct xlate_ctx *ctx) >> @@ -3598,7 +3598,7 @@ propagate_tunnel_data_to_flow(struct xlate_ctx *ctx, >> struct eth_addr dmac, >> static int >> native_tunnel_output(struct xlate_ctx *ctx, const struct xport *xport, >> const struct flow *flow, odp_port_t tunnel_odp_port, >> - bool truncate) >> + bool truncate, bool is_last_action) >> { >> struct netdev_tnl_build_header_params tnl_params; >> struct ovs_action_push_tnl tnl_push_data; >> @@ -3728,7 +3728,7 @@ native_tunnel_output(struct xlate_ctx *ctx, const >> struct xport *xport, >> entry->tunnel_hdr.hdr_size = tnl_push_data.header_len; >> entry->tunnel_hdr.operation = ADD; >> >> - patch_port_output(ctx, xport, out_dev); >> + patch_port_output(ctx, xport, out_dev, is_last_action); >> >> /* Similar to the stats update in revalidation, the x_cache entries >> * are populated by the previous translation are used to update the >> @@ -3822,7 +3822,7 @@ xlate_flow_is_protected(const struct xlate_ctx *ctx, >> const struct flow *flow, co >> */ >> static void >> patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, >> - struct xport *out_dev) >> + struct xport *out_dev, bool is_last_action) >> { >> struct flow *flow = &ctx->xin->flow; >> struct flow old_flow = ctx->xin->flow; >> @@ -3864,8 +3864,9 @@ patch_port_output(struct xlate_ctx *ctx, const struct >> xport *in_dev, >> if (!process_special(ctx, out_dev) && may_receive(out_dev, ctx)) { >> if (xport_stp_forward_state(out_dev) && >> xport_rstp_forward_state(out_dev)) { >> + >> xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, >> - false, true, clone_xlate_actions); >> + false, is_last_action, clone_xlate_actions); >> if (!ctx->freezing) { >> xlate_action_set(ctx); >> } >> @@ -3880,7 +3881,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct >> xport *in_dev, >> mirror_mask_t old_mirrors2 = ctx->mirrors; >> >> xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, >> - false, true, clone_xlate_actions); >> + false, is_last_action, clone_xlate_actions); >> ctx->mirrors = old_mirrors2; >> ctx->base_flow = old_base_flow; >> ctx->odp_actions->size = old_size; >> @@ -4107,7 +4108,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, struct >> flow *flow, >> static void >> compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, >> const struct xlate_bond_recirc *xr, bool check_stp, >> - bool is_last_action OVS_UNUSED, bool truncate) >> + bool is_last_action, bool truncate) >> { >> const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port); >> struct flow_wildcards *wc = ctx->wc; >> @@ -4144,7 +4145,7 @@ compose_output_action__(struct xlate_ctx *ctx, >> ofp_port_t ofp_port, >> if (truncate) { >> xlate_report_error(ctx, "Cannot truncate output to patch port"); >> } >> - patch_port_output(ctx, xport, xport->peer); >> + patch_port_output(ctx, xport, xport->peer, is_last_action); >> return; >> } >> >> @@ -4239,7 +4240,8 @@ compose_output_action__(struct xlate_ctx *ctx, >> ofp_port_t ofp_port, >> xr->recirc_id); >> } else if (is_native_tunnel) { >> /* Output to native tunnel port. */ >> - native_tunnel_output(ctx, xport, flow, odp_port, truncate); >> + native_tunnel_output(ctx, xport, flow, odp_port, truncate, >> + is_last_action); >> flow->tunnel = flow_tnl; /* Restore tunnel metadata */ >> >> } else if (terminate_native_tunnel(ctx, flow, wc, >> @@ -6742,7 +6744,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t >> ofpacts_len, >> const struct ofpact_set_field *set_field; >> const struct mf_field *mf; >> bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len) >> - && ctx->action_set.size; >> + && !ctx->action_set.size; >> >> if (ctx->error) { >> break; >> @@ -7145,11 +7147,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t >> ofpacts_len, >> break; >> >> case OFPACT_CHECK_PKT_LARGER: { >> - if (last) { >> - /* If this is last action, then there is no need to >> - * translate the action. */ >> - break; >> - } >> const struct ofpact *remaining_acts = ofpact_next(a); >> size_t remaining_acts_len = ofpact_remaining_len(remaining_acts, >> ofpacts, >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 31064ed95..adf24d671 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -8416,6 +8416,34 @@ AT_CHECK([sed -n >> 's/=[[0-9]][[0-9]]\(\.[[0-9]][[0-9]]*\)\{0,1\}s/=?s/p' stdout], >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> >> + >> +AT_SETUP([ofproto-dpif - patch ports - meter (clone)]) >> + >> +OVS_VSWITCHD_START( >> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \ >> + add-port br0 p1 -- set Interface p1 type=patch \ >> + options:peer=p2 ofport_request=2 -- \ >> + add-br br1 -- \ >> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \ >> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \ >> + fail-mode=secure -- \ >> + add-port br1 p2 -- set Interface p2 type=patch \ >> + options:peer=p1 -- \ >> + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3]) >> + >> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br1 'meter=1 pktps stats >> bands=type=drop rate=2']) >> +AT_CHECK([ovs-ofctl del-flows br0]) >> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 >> in_port=local,ip,actions=2,1]) >> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br1 >> in_port=1,ip,actions=meter:1,3]) >> + >> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy >> 'in_port(100),eth(src=f8:bc:12:44:34:b6,dst=f8:bc:12:46:58:e0),eth_type(0x0800),ipv4(src=10.1.1.22,dst=10.0.0.3,proto=6,tos=0,ttl=64,frag=no),tcp(src=53295,dst=8080)'], >> [0], [stdout]) >> +AT_CHECK([tail -1 stdout], [0], >> + [Datapath actions: clone(meter(0),3),1 >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> + >> dnl ---------------------------------------------------------------------- >> AT_BANNER([ofproto-dpif -- megaflows]) >> >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev