> On May 9, 2025, at 6:03 PM, Aaron Conole <[email protected]> wrote:
>
> Daniel Ding <[email protected] <mailto:[email protected]>> 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 <[email protected]>
>> ---
>
> 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
> <http://ofproto-dpif.at:12063/>
> error: tests/ofproto-dpif.at <http://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.
>
Hi Aaron,
I don’t think this test needs to be added to the dummy datapath. This issue
only occurs in the netdev datapath, which clones actions when encountering
a tunnel output and the action is not the last one.
Maybe, ofproto-dpif.at is not the appropriate position for this test.
>> \ No newline at end of file
>> diff --git a/tests/ofproto-macros.at <http://ofproto-macros.at/>
>> b/tests/ofproto-macros.at <http://ofproto-macros.at/>
>> index c22fb3c79..50512f08b 100644
>> --- a/tests/ofproto-macros.at <http://ofproto-macros.at/>
>> +++ b/tests/ofproto-macros.at <http://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
Regards,
Daniel Ding
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
