On Tue, 1 Apr 2025 at 23:21, Aaron Conole <acon...@redhat.com> 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:
>
>
I add a dpif_execute_meters_discard coverage counter to record it. The
warning log may not have some valuaful information. And the else case
is not an actual impact for the packet_out message.
https://patchwork.ozlabs.org/comment/3487109/
>
> > 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. 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
> ...
>
>
You are right, and I will try to write this case for ofproto.at.
> 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
