When a meter action is encountered and stored in the auxiliary structure, and subsequently, a non-meter action is processed within a nested list during callback execution, an infinite loop is triggered.
This patch maintains the current behavior but stores all required meter actions in an ofpbuf for deferred execution. Reportde-at: https://patchwork.ozlabs.org/project/openvswitch/patch/20250506022337.3242-1-danieldin...@gmail.com/ Fixes: 076caa2fb077 ("ofproto: Meter translation.") Signed-off-by: Eelco Chaudron <echau...@redhat.com> --- v3: - Fix some review nits. v2: - Removed redundant extra newlines. - Update comment to reflect reality - Use the add_of_ports MACRO - Use 'ovs-ofctl compose-packet --bare' for packet construction - Fix some grep commands. --- lib/dpif.c | 65 +++++++++++++++++-------------------------- tests/ofproto-dpif.at | 36 ++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 40 deletions(-) diff --git a/lib/dpif.c b/lib/dpif.c index a064f717f..070fc0131 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1163,7 +1163,7 @@ struct dpif_execute_helper_aux { struct dpif *dpif; const struct flow *flow; int error; - const struct nlattr *meter_action; /* Non-NULL, if have a meter action. */ + struct ofpbuf meter_actions; }; /* This is called for actions that need the context of the datapath to be @@ -1180,10 +1180,14 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, switch ((enum ovs_action_attr)type) { case OVS_ACTION_ATTR_METER: - /* Maintain a pointer to the first meter action seen. */ - if (!aux->meter_action) { - aux->meter_action = action; - } + /* XXX: This code collects meter actions since the last action + * execution via the datapath to be executed right before the + * current action that needs to be executed by the datapath. + * This is only an approximation, but better than nothing. + * Fundamentally, we should have a mechanism by which the + * datapath could return the result of the meter action so that + * we could execute them at the right order. */ + ofpbuf_put(&aux->meter_actions, action, NLA_ALIGN(action->nla_len)); break; case OVS_ACTION_ATTR_CT: @@ -1196,43 +1200,21 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, case OVS_ACTION_ATTR_SAMPLE: case OVS_ACTION_ATTR_RECIRC: { struct dpif_execute execute; - struct ofpbuf execute_actions; - uint64_t stub[256 / 8]; struct pkt_metadata *md = &packet->md; - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) { - ofpbuf_use_stub(&execute_actions, stub, sizeof stub); - - if (aux->meter_action) { - const struct nlattr *a = aux->meter_action; - - /* XXX: This code collects meter actions since the last action - * execution via the datapath to be executed right before the - * current action that needs to be executed by the datapath. - * This is only an approximation, but better than nothing. - * Fundamentally, we should have a mechanism by which the - * datapath could return the result of the meter action so that - * we could execute them at the right order. */ - do { - ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len)); - /* Find next meter action before 'action', if any. */ - do { - a = nl_attr_next(a); - } while (a != action && - nl_attr_type(a) != OVS_ACTION_ATTR_METER); - } while (a != action); - } + if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_actions.size) { + struct ofpbuf *execute_actions = &aux->meter_actions; /* The Linux kernel datapath throws away the tunnel information * that we supply as metadata. We have to use a "set" action to * supply it. */ if (flow_tnl_dst_is_set(&md->tunnel)) { - odp_put_tunnel_action(&md->tunnel, &execute_actions, NULL); + odp_put_tunnel_action(&md->tunnel, execute_actions, NULL); } - ofpbuf_put(&execute_actions, action, NLA_ALIGN(action->nla_len)); + ofpbuf_put(execute_actions, action, NLA_ALIGN(action->nla_len)); - execute.actions = execute_actions.data; - execute.actions_len = execute_actions.size; + execute.actions = execute_actions->data; + execute.actions_len = execute_actions->size; } else { execute.actions = action; execute.actions_len = NLA_ALIGN(action->nla_len); @@ -1264,12 +1246,9 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, dp_packet_delete(clone); - if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) { - ofpbuf_uninit(&execute_actions); - - /* Do not re-use the same meters for later output actions. */ - aux->meter_action = NULL; - } + /* Clear the 'aux->meter_actions' ofpbuf as it could have been + * used for sending the additional meter and/or tunnel actions. */ + ofpbuf_clear(&aux->meter_actions); break; } @@ -1306,14 +1285,20 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, static int dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute) { - struct dpif_execute_helper_aux aux = {dpif, execute->flow, 0, NULL}; + struct dpif_execute_helper_aux aux = { + .dpif = dpif, + .flow = execute->flow, + .error = 0, + }; struct dp_packet_batch pb; COVERAGE_INC(dpif_execute_with_help); + ofpbuf_init(&aux.meter_actions, 0); dp_packet_batch_init_packet(&pb, execute->packet); odp_execute_actions(&aux, &pb, false, execute->actions, execute->actions_len, dpif_execute_helper_cb); + ofpbuf_uninit(&aux.meter_actions); return aux.error; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 7930b3f29..46b1e6707 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -6621,6 +6621,42 @@ This flow is handled by the userspace slow path because it: OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - meter with slow-path action]) + +OVS_VSWITCHD_START + +AT_CHECK([ovs-ofctl -O OpenFlow15 add-meter br0 \ + 'meter=1 pktps burst stats bands=type=drop rate=100 burst_size=100']) + +add_of_ports --pcap br0 1 2 + +AT_CHECK([ovs-appctl vlog/set dpif:dbg]) + +eth="eth_src=00:00:00:00:00:ec,eth_dst=ff:ff:ff:ff:ff:ff,arp" +arp="arp_tpa=192.168.0.1,arp_spa=192.168.0.100,arp_op=1" +packet=$(ovs-ofctl compose-packet --bare "$eth,$arp") + +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 \ + "table=1,eth_type=0x0806,actions=meter:1,clone(set_field:2->arp_op,output(port=p2,max_len=128)),output:p1"]) + +AT_CHECK([ovs-ofctl -O OpenFlow15 packet-out br0 CONTROLLER \ + "meter:1,resubmit(,1)" "$packet"]) + +dnl Verify the orinigal packet is received on port 1. +OVS_WAIT_UNTIL([test $(ovs-pcap p1-tx.pcap | grep -c "$packet") -eq 1]) + +dnl Verify the modified packet is received on port 2. +arp="arp_tpa=192.168.0.1,arp_spa=192.168.0.100,arp_op=2" +packet=$(ovs-ofctl compose-packet --bare "$eth,$arp") +OVS_WAIT_UNTIL([test $(ovs-pcap p2-tx.pcap | grep -c "$packet") -eq 1]) + +dnl Make sure all meters, and outputs datapath actions get executed. +OVS_WAIT_UNTIL([grep "sub-execute meter(0),meter(0),2 on packet arp" \ + ovs-vswitchd.log]) +OVS_WAIT_UNTIL([grep "sub-execute 1 on packet arp" ovs-vswitchd.log]) + +OVS_VSWITCHD_STOP +AT_CLEANUP dnl CHECK_CONTINUATION(TITLE, N_PORTS0, N_PORTS1, ACTIONS0, ACTIONS1, [EXTRA_SETUP]) dnl -- 2.50.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
