On Wed, 2 Apr 2025 at 00:16, Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 4/1/25 5:21 PM, Aaron Conole via dev wrote:
> > Daniel Ding <danieldin...@gmail.com> writes:
> >
> >> In order to collect meters since the last meter action
> >> to the current action in the dpif_execute_helper_cb,
> >> the next entry is simply obtained using nl_attr_next, and
> >> not considering the wrong nested nl without end_nested.
> >> This will cause memory access to be overflowed.
> >>
> >> Gdb debug looks like this:
> >>
> >> (gdb) print a
> >> $6 = (const struct nlattr *) 0xaaaad175a69c
> >> (gdb) n
> >> 1206 a = nl_attr_next(a);
> >> (gdb) n
> >> 1207 } while (a != action &&
> >> (gdb) n
> >> 1206 a = nl_attr_next(a);
> >> (gdb) print a
> >> $7 = (const struct nlattr *) 0xaaaad175a69c
> >> (gdb) print *a
> >> $8 = {nla_len = 0, nla_type = 0}
> >> (gdb)
> >>
> >> Fixes: 076caa2fb077 ("ofproto: Meter translation.")
> >> Signed-off-by: Daniel Ding <danieldin...@gmail.com>
> >> ---
> >> lib/dpif.c | 43 ++++++++++++++++--------------------------
> >> tests/system-dpdk.at | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 61 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/lib/dpif.c b/lib/dpif.c
> >> index d07241f1e..05dfd2919 100644
> >> --- a/lib/dpif.c
> >> +++ b/lib/dpif.c
> >> @@ -1158,11 +1158,14 @@ dpif_flow_dump_next(struct
> dpif_flow_dump_thread *thread,
> >> return n;
> >> }
> >>
> >> +enum { EXECUTE_HELPER_MAX_METER = 32 };
> >> +
> >> 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. */
> >> + size_t meter_count;
> >> + const struct nlattr *meter_actions[EXECUTE_HELPER_MAX_METER + 1];
> >> };
> >>
> >> /* This is called for actions that need the context of the datapath to
> be
> >> @@ -1179,9 +1182,9 @@ 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;
> >> + /* Records the meter actions. */
> >> + if (aux->meter_count < EXECUTE_HELPER_MAX_METER) {
> >> + aux->meter_actions[aux->meter_count++] = action;
> >> }
> >
> > Can we have a coverage counter that trips for the else case? Maybe a
> > rate limited warning log as well. At least if we're not going to do the
> > search as described by Ilya in:
> >
> > https://patchwork.ozlabs.org/comment/3487109/
>
> If we have more than one meter, it's very likely we'll collect wrong ones
> here. Executing just the first one may be better in this case, if we're
> not going for the proper solution. i.e. just drop that accumulation
> logic and the traversal below and just put one meter.
> It appears to me that it works somewhat like that today anyways, since
> we're
> missing all the nested meters.
>
> And we can work on making the proper logic later.
>
>
Thanks Ilya Maximets for your review. The first meter may not be the best
solution, but collecting from the first meter can bring many compatibility
issues, and there is no need to do so in this case.
---
@@ -71,6 +71,7 @@ COVERAGE_DEFINE(dpif_meter_set);
COVERAGE_DEFINE(dpif_port_add);
COVERAGE_DEFINE(dpif_port_del);
COVERAGE_DEFINE(dpif_purge);
+COVERAGE_DEFINE(dpif_execute_meters_discard);
static const struct dpif_class *base_dpif_classes[] = {
#if defined(__linux__) || defined(_WIN32)
@@ -1182,6 +1183,8 @@ dpif_execute_helper_cb(void *aux_, struct
dp_packet_batch *packets_,
/* Maintain a pointer to the first meter action seen. */
if (!aux->meter_action) {
aux->meter_action = action;
+ } else {
+ COVERAGE_INC(dpif_execute_meters_discard);
}
break;
@@ -1202,22 +1205,7 @@ dpif_execute_helper_cb(void *aux_, struct
dp_packet_batch *packets_,
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);
+ ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len));
}
>
> >> break;
> >>
> >> @@ -1197,27 +1200,15 @@ dpif_execute_helper_cb(void *aux_, struct
> dp_packet_batch *packets_,
> >> uint64_t stub[256 / 8];
> >> 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_count) {
> >> 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 {
> >> + if (aux->meter_count) {
> >> + size_t i;
> >> + for (i = 0; i < aux->meter_count; i++) {
> >> + const struct nlattr *a = aux->meter_actions[i];
> >> 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
> >> @@ -1261,11 +1252,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) {
> >> + if (flow_tnl_dst_is_set(&md->tunnel) || aux->meter_count) {
> >> ofpbuf_uninit(&execute_actions);
> >> -
> >> - /* Do not re-use the same meters for later output actions.
> */
> >> - aux->meter_action = NULL;
> >> + aux->meter_count = 0;
> >> }
> >> break;
> >> }
> >> @@ -1303,7 +1292,7 @@ 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, execute->flow, 0, 0,
> {NULL}};
> >> struct dp_packet_batch pb;
> >>
> >> COVERAGE_INC(dpif_execute_with_help);
> >> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> >> index 1c97bf777..43b41a9f2 100644
> >> --- a/tests/system-dpdk.at
> >> +++ b/tests/system-dpdk.at
> >
> > The test suite you've submitted is broken on my system. Additionally,
> > it will only run for DPDK testing, but it really can be run on all
> > users of the userspace datapath.
> >
> > I think it should be written for system-traffic.at rather than as DPDK
> > specific suite.
>
> It shouldn't actually be even a system test. It should be a normal test
> in the tests/ofproto-dpif.at. The issue doesn't require any real
> interfaces
> to be triggered.
>
> > I didn't actually test with the supplied fix, but the
> > test suite below will invoke a hang with ``make check-system-userspace``
> > like:
> >
> > Thread 1 (LWP 4033950 "ovs-vswitchd"):
> > #0 0x00000000004bf397 in nl_attr_type (nla=nla@entry=0x2cf879d8) at
> lib/netlink.c:642
> > #1 0x0000000000490778 in dpif_execute_helper_cb
> > (aux_=aux_@entry=0x7ffc1e9b0d60,
> packets_=packets_@entry=0x7ffc1e9b0a80, action=action@entry=0x2cf7d2d4,
> should_steal=should_steal@entry=false) at lib/dpif.c:1222
> > ...
> >
> >
> > Ex suite:
> >
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2813,6 +2813,40 @@ OVS_WAIT_UNTIL([test $(ovs-pcap p1.pcap | grep -c
> "${packet}") -eq 5])
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +dnl Setup dpif collecting meters
> > +AT_SETUP([datapath - meter execution helpers])
> > +AT_KEYWORDS([meter meters])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +
> > +ADD_BR([br-vm])
> > +
> > +dnl Active vxlan egress network
> > +AT_CHECK([ip link set up dev br0])
> > +AT_CHECK([ip addr add 1.1.1.2/24 dev br0])
> > +AT_CHECK([ovs-vsctl add-port br-vm p1 -- set Interface p1 type=vxlan
> options:remote_ip=1.1.1.1 options:local_ip=1.1.1.2])
> > +
> > +ADD_NAMESPACES(at_ns1)
> > +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1],
> [0], [ignore])
> > +
> > +ADD_VETH(p0, at_ns1, br0, "1.1.1.1/24", "e4:11:22:33:44:55")
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "table=0,priority=0 actions=NORMAL"])
> > +AT_CHECK([ovs-ofctl add-flow br-vm "table=0,priority=0 actions=NORMAL"])
> > +
> > +AT_CHECK([ping -c 3 1.1.1.1], [0], [stdout])
> > +OVS_WAIT_UNTIL([test -n "`ovs-appctl tnl/arp/show | grep 1.1.1.1`"])
> > +
> > +dnl Force arps into collecting meters
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br-vm 'meter=1 pktps
> bands=type=drop rate=1'])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br-vm 'table=2,arp
> actions=meter:1,set_field:2->arp_op,p1,br-vm'])
> > +
> > +dnl Send an arp packet-out message
> > +AT_CHECK([ovs-ofctl packet-out br-vm 'in_port=controller
> packet=ffffffffffff000000010102080600010800060400010000000101020101010300000000000001010102
> actions=resubmit(,2)'])
> > +
> > +dnl Clean up
> > +OVS_TRAFFIC_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > AT_BANNER([MPLS])
> >
> > AT_SETUP([mpls - encap header dp-support])
> >
> > _______________________________________________
> > 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
