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>
> ---
Hi Daniel,
Thanks for continuing to work on this.
As noted by the robot, and by my own testing, there is an error applying
for the test you add:
Applying: dpif: Fix collecting meters overflows memory.
error: patch failed: tests/ofproto-dpif.at:12063
error: tests/ofproto-dpif.at: patch does not apply
Patch failed at 0001 dpif: Fix collecting meters overflows memory.
> lib/dpif.c | 20 +++----------
> tests/ofproto-dpif.at | 29 ++++++++++++++++++
> tests/ofproto-macros.at | 66 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 99 insertions(+), 16 deletions(-)
>
> diff --git a/lib/dpif.c b/lib/dpif.c
> index d07241f1e..02429a30c 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -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));
> }
>
> /* The Linux kernel datapath throws away the tunnel information
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index a1393f7f8..f5f665e45 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12063,3 +12063,32 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap
> | wc -l`])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +
> +AT_SETUP([ofproto-dpif - collecting meters])
> +OVS_NETDEV_VSWITCHD_START
> +
> +AT_CHECK([ovs-vsctl add-br br-vm -- set Bridge br-vm datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-br br-phy -- set Bridge br-phy datapath_type=netdev])
> +AT_CHECK([ovs-vsctl add-port br-vm vx1 -- set Interface vx1 type=vxlan
> options:remote_ip=1.1.1.1 options:local_ip=1.1.1.2])
> +
> +dnl Active vxlan egress network
> +ADD_NAMESPACES(ns1)
> +ADD_VETH(p1, ns1, br-phy, "1.1.1.1/24")
> +AT_CHECK([ip link set up dev br-phy])
> +AT_CHECK([ip addr add 1.1.1.2/24 dev br-phy])
> +
> +dnl Force packet-out 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,vx1,br-vm'])
> +
> +dnl Wait egress network ready
> +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`"])
> +OVS_WAIT_UNTIL([test -n "`ovs-appctl ovs/route/show | grep 1.1.1.0`"])
> +
> +dnl Send a arp packet-out message
> +AT_CHECK([ovs-ofctl packet-out br-vm 'in_port=controller
> packet=ffffffffffff000000010102080600010800060400010000000101020101010300000000000001010102
> actions=resubmit(,2)'])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
This test will need to be reworked so that it can function via the dummy
datapath. The reason we do that is so that the test can be run even
without super user privileges. This test is checking a part of the code
that can be tested independently of having live traffic passing, so
please implement it that way.
> \ No newline at end of file
> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
> index c22fb3c79..50512f08b 100644
> --- a/tests/ofproto-macros.at
> +++ b/tests/ofproto-macros.at
This introduction here is inappropriate for the ofproto-dpif suite. The
point of this suite is to do testing via the dummy datapath rather than
a datapath that will require real ports from the system.
> @@ -384,3 +384,69 @@ m4_define([WAIT_FOR_DUMMY_PORTS], \
> | grep 'unknown\|disconnected'])])])
>
>
> +# DEL_NAMESPACES(ns [, ns ... ])
> +#
> +# Delete namespaces from the running OS
> +m4_define([DEL_NAMESPACES],
> + [m4_foreach([ns], [$@],
> + [ip netns del ns
> +])
> + ]
> +)
> +
> +
> +# ADD_NAMESPACES(ns [, ns ... ])
> +#
> +# Add new namespaces, if ns exists, the old one
> +# will be remove before new ones are installed.
> +m4_define([ADD_NAMESPACES],
> + [m4_foreach([ns], [$@],
> + [DEL_NAMESPACES(ns)
> + AT_CHECK([ip netns add ns || return 77])
> + on_exit 'DEL_NAMESPACES(ns)'
> + ip netns exec ns sysctl -w
> net.netfilter.nf_conntrack_helper=0
> + ])
> + ]
> +)
> +# NS_EXEC([namespace], [command])
> +#
> +# Execute 'command' in 'namespace'
> +m4_define([NS_EXEC],
> + [ip netns exec $1 sh << NS_EXEC_HEREDOC
> +$2
> +NS_EXEC_HEREDOC])
> +
> +# NS_CHECK_EXEC([namespace], [command], other_params...)
> +#
> +# Wrapper for AT_CHECK that executes 'command' inside 'namespace'.
> +# 'other_params' as passed as they are to AT_CHECK.
> +m4_define([NS_CHECK_EXEC],
> + [ AT_CHECK([NS_EXEC([$1], [$2])], m4_shift(m4_shift($@))) ]
> +)
> +
> +m4_define([ADD_VETH],
> + [ AT_CHECK([ip link add $1 type veth peer name ovs-$1 || return 77])
> + AT_CHECK([ip link set $1 netns $2])
> + AT_CHECK([ip link set dev ovs-$1 up])
> + AT_CHECK([ovs-vsctl add-port $3 ovs-$1])
> + NS_CHECK_EXEC([$2], [ip addr add $4 dev $1 $7])
> + NS_CHECK_EXEC([$2], [ip link set dev $1 up])
> + if test -n "$5"; then
> + NS_CHECK_EXEC([$2], [ip link set dev $1 address $5])
> + fi
> + if test -n "$6"; then
> + NS_CHECK_EXEC([$2], [ip route add default via $6])
> + fi
> + on_exit 'ip link del ovs-$1'
> + ]
> +)
> +
> +# OVS_NETDEV_VSWITCHD_START([dbinit-aux-args])
> +#
> +# Creates a database and starts ovsdb-server, starts ovs-vswitchd
> +# connected to that database.
> +# 'dbinit-aux-args' are passed as additional commands to 'ovs-vsctl init'
> +# before starting ovs-vswitchd.
> +m4_define([OVS_NETDEV_VSWITCHD_START],
> + [_OVS_VSWITCHD_START([--disable-system], [$1])
> +])
> \ No newline at end of file
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
