mir/ovs
2
0
Fork 0
mirror of https://github.com/openvswitch/ovs synced 2025-08-22 01:51:26 +00:00
Code Issues Releases Wiki Activity

dpif: Fix infinite netlink loop in dpif_execute_helper_cb.

Browse Source 
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.

Reported-at: https://patchwork.ozlabs.org/project/openvswitch/patch/20250506022337.3242-1-danieldin186@gmail.com/
Fixes: 076caa2fb077 ("ofproto: Meter translation.")
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
This commit is contained in:
Eelco Chaudron 2025-07-15 15:12:21 +02:00
parent 50e1e57f81
commit 5c4d60671c
2 changed files with 61 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
lib/dpif.c
View File

@ -1163,7 +1163,7 @@ struct dpif_execute_helper_aux {
struct dpif *dpif; struct dpif *dpif;
const struct flow *flow; const struct flow *flow;
int error; 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 /* 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) { switch ((enum ovs_action_attr)type) {
case OVS_ACTION_ATTR_METER: case OVS_ACTION_ATTR_METER:
/* Maintain a pointer to the first meter action seen. */ /* XXX: This code collects meter actions since the last action
if (!aux->meter_action) { * execution via the datapath to be executed right before the
aux->meter_action = action; * 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; break;
case OVS_ACTION_ATTR_CT: 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_SAMPLE:
case OVS_ACTION_ATTR_RECIRC: { case OVS_ACTION_ATTR_RECIRC: {
struct dpif_execute execute; struct dpif_execute execute;
struct ofpbuf execute_actions;
uint64_t stub[256 / 8];
struct pkt_metadata *md = &packet->md; struct pkt_metadata *md = &packet->md;
if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) { if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_actions.size) {
ofpbuf_use_stub(&execute_actions, stub, sizeof stub); struct ofpbuf *execute_actions = &aux->meter_actions;
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);
}
/* The Linux kernel datapath throws away the tunnel information /* The Linux kernel datapath throws away the tunnel information
* that we supply as metadata. We have to use a "set" action to * that we supply as metadata. We have to use a "set" action to
* supply it. */ * supply it. */
if (flow_tnl_dst_is_set(&md->tunnel)) { 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 = execute_actions->data;
execute.actions_len = execute_actions.size; execute.actions_len = execute_actions->size;
} else { } else {
execute.actions = action; execute.actions = action;
execute.actions_len = NLA_ALIGN(action->nla_len); 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); dp_packet_delete(clone);
if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_action) { /* Clear the 'aux->meter_actions' ofpbuf as it could have been
ofpbuf_uninit(&execute_actions); * used for sending the additional meter and/or tunnel actions. */
ofpbuf_clear(&aux->meter_actions);
/* Do not re-use the same meters for later output actions. */
aux->meter_action = NULL;
}
break; break;
} }
@ -1306,14 +1285,20 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
static int static int
dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute) 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; struct dp_packet_batch pb;
COVERAGE_INC(dpif_execute_with_help); COVERAGE_INC(dpif_execute_with_help);
ofpbuf_init(&aux.meter_actions, 0);
dp_packet_batch_init_packet(&pb, execute->packet); dp_packet_batch_init_packet(&pb, execute->packet);
odp_execute_actions(&aux, &pb, false, execute->actions, odp_execute_actions(&aux, &pb, false, execute->actions,
execute->actions_len, dpif_execute_helper_cb); execute->actions_len, dpif_execute_helper_cb);
ofpbuf_uninit(&aux.meter_actions);
return aux.error; return aux.error;
} }

36
tests/ofproto-dpif.at
View File

@ -6621,6 +6621,42 @@ This flow is handled by the userspace slow path because it:
OVS_VSWITCHD_STOP OVS_VSWITCHD_STOP
AT_CLEANUP 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 CHECK_CONTINUATION(TITLE, N_PORTS0, N_PORTS1, ACTIONS0, ACTIONS1, [EXTRA_SETUP])
dnl dnl