On 7/15/25 11:13 AM, Eelco Chaudron wrote:
> 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>
>
> ---
> 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(-)
Thanks, Eelco. This version seems fine overall, see a couple nits below.
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index a064f717f..3d0d366f1 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;
> - }
> + /* Re-initialize the 'aux->meter_actions' ofpbuf as it could have
> been
> + * used for sending the additional meter and/or tunnel actions. */
> + ofpbuf_reinit(&aux->meter_actions, 0);
Should we maybe use ofpbuf_clear() instead? It doesn't seem to
be necessary to reallocate the buffer.
> 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
Trailing comma is missing.
> + };
> 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..770337361 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 dnl
'dnl' here, but '\' in the command below. Might be better to just use
'\' in all cases. Modern autotest works fine with line continuations.
Even better than with dnl, as dnl sometimes ends up with the rest of
the command not showing up in the log.
> +
> "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])
May be better to use $() syntax instead of ``, since you're already
using $() above.
> +
> +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" dnl
> + ovs-vswitchd.log])
dnl vs '\'
> +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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
