While tracing NAT actions, pointer to the action may be stored in the
recirculation node for future reference. However, while translating
actions for the group bucket in xlate_group_bucket, the action list is
allocated temporarily on stack. So, in case the group translation
leads to NAT, the stack pointer can be stored in the recirculation node
and accessed later by the tracing mechanism when this stack memory is
long gone:
==396230==ERROR: AddressSanitizer: stack-use-after-return on address
0x191844 at pc 0x64222a bp 0xa5da10 sp 0xa5da08
READ of size 1 at 0x191844 thread T0
0 0x642229 in ofproto_trace_recirc_node ofproto/ofproto-dpif-trace.c:704:49
1 0x642229 in ofproto_trace ofproto/ofproto-dpif-trace.c:867:9
2 0x6434c1 in ofproto_unixctl_trace ofproto/ofproto-dpif-trace.c:489:9
3 0xc1e491 in process_command lib/unixctl.c:310:13
4 0xc1e491 in run_connection lib/unixctl.c:344:17
5 0xc1e491 in unixctl_server_run lib/unixctl.c:395:21
6 0x53eedf in main ovs/vswitchd/ovs-vswitchd.c:131:9
7 0x2be087 in __libc_start_call_main
8 0x2be14a in __libc_start_main@GLIBC_2.2.5
9 0x42dee4 in _start (vswitchd/ovs-vswitchd+0x42dee4)
Address 0x191844 is located in stack of thread T0 at offset 68 in frame
0 0x6d391f in xlate_group_bucket ofproto/ofproto-dpif-xlate.c:4751
This frame has 3 object(s):
[32, 1056) 'action_list_stub' (line 4760) <== Memory access at
offset 68 is inside
this variable
[1184, 1248) 'action_list' (line 4761)
[1280, 1344) 'action_set' (line 4762)
SUMMARY: AddressSanitizer: stack-use-after-return
ofproto/ofproto-dpif-trace.c:704:49 in ofproto_trace_recirc_node
Fix that by copying the action.
Fixes: d072d2de011b ("ofproto-dpif-trace: Improve NAT tracing.")
Reported-by: Ales Musil <amusil@redhat.com>
Reviewed-by: Adrian Moreno <amorenoz@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Trace attempts to process all the recirculations. However, if there
is a recirculation loop, i.e. if every recirculation generates another
recirculation, this process will never stop. It will grind until the
trace fills the system memory.
A simple reproducer:
make sandbox
ovs-vsctl add-br br0
ovs-vsctl add-port br0 p1
ovs-ofctl add-flow br0 "table=0,in_port=p1,ip,actions=ct(table=0)"
ovs-appctl ofproto/trace br0 in_port=p1,ip
Limit the number of recirculations trace is processing with a fairly
arbitrary number - 4096 (loosely based on the resubmit limit, but
they are not actually related).
Not adding a test for this since it's only for a trace, but also
because the test may lead to OOM event in a system if the test fails,
which is not nice.
Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation")
Reported-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
With --bare, it will produce a bare hexified payload with no spaces or
offset indicators inserted, which is useful in tests to produce frames
to pass to e.g. `ovs-ofctl receive`.
With --bad-csum, it will produce a frame that has an invalid IP checksum
(applicable to IPv4 only because IPv6 doesn't have checksums.)
The command is now more useful in tests, where we may need to produce
hex frame payloads to compare observed frames against.
As an example of the tool use, a single test case is converted to it.
The test uses both normal --bare and --bad-csum behaviors of the
command, confirming they work as advertised.
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Most of commands in ovs-ofctl and ovs-appctl can display port names
instead of port numbers by using --names option. This change adds
similar functionality to ofproto/trace.
For backward compatibility, the default behavior is the same as
before.
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Nobuhiro MIKI <nmiki@yahoo-corp.jp>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Using the SHORT version of the *_SAFE loops makes the code cleaner
and less error-prone. So, use the SHORT version and remove the extra
variable when possible.
In order to be able to use both long and short versions without changing
the name of the macro for all the clients, overload the existing name
and select the appropriate version depending on the number of arguments.
Acked-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
When ofproto/trace detects a recirc action it resumes execution at the
specified next table. However, if the ct action performs SNAT/DNAT,
e.g., ct(commit,nat(src=1.1.1.1:4000),table=42), the src/dst IPs and
ports in the oftrace_recirc_node->flow field are not updated. This leads
to misleading outputs from ofproto/trace as real packets would actually
first get NATed and might match different flows when recirculated.
Assume the first IP/port from the NAT src/dst action will be used by
conntrack for the translation and update the oftrace_recirc_node->flow
accordingly. This is not entirely correct as conntrack might choose a
different IP/port but the result is more realistic than before.
This fix covers new connections. However, for reply traffic that executes
actions of the form ct(nat, table=42) we still don't update the flow as
we don't have any information about conntrack state when tracing.
Also move the oftrace_recirc_node processing out of ofproto_trace()
and to its own function, ofproto_trace_recirc_node() for better
readability/
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
As a side effect, this also reduces a lot of log messages' severities from
ERR to WARN. They just didn't seem like messages that in general reported
anything that would prevent functioning.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Added the check for NULL in "next_ct_states" argument passed to the
"ofproto_trace()" function. Under normal scenario, this is non-NULL. A NULL
"next_ct_states" argument is passed from the "upcall_xlate()" function on
encountering XLATE_RECURSION_TOO_DEEP or XLATE_TOO_MANY_RESUBMITS error.
VMware-BZ: #2282287
Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Prior to the OVS 2.9 development cycle, any flow that sent a packet to a
controller required that the flow be slow-pathed. In some cases this led
to poor performance, so OVS 2.9 made controller actions fast-pathable. As
a side effect of the change, "ovs-appctl ofproto/trace -generate" no longer
sent packets to the controller. This usually didn't matter but it broke
the Faucet tutorial, which relied on this behavior. This commit
reintroduces the original behavior and thus should fix the tutorial.
CC: Justin Pettit <jpettit@ovn.org>
Fixes: d39ec23de384 ("ofproto-dpif: Don't slow-path controller actions.")
Reported-by: macman31 <https://github.com/macman31>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/145
Reported-by: Brad Cowie <brad@cowie.nz>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-August/047234.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
ofpacts_check__() was a huge switch statement with special cases for many
different kinds of actions. This made it unwieldy and put the special
cases far away from the rest of the code related to a given action. This
commit refactors the code to avoid the problem.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
OVS uses spaces for indentation in source code and it makes sense for it to
also use spaces for indentation in output. Spaces also consume less
horizontal space in output, which often makes it easier to read. This
commit transitions one part of output from tabs to spaces and updates
appropriate parts of the tests to match.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
This makes it clear which buckets from a group are executed and why.
The update to nsh.at provides an example.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
Usually ofproto/trace is used to debug the flow translation error.
When translation error such as recursion too deep or too many resubmit,
the issue might happen momentary; flows causing the recursion expire
when users try to debug it. This patch enables the ofproto trace
automatically when recursion is too deep or too many resubmit, by
invoking the translation again, and log the ofproto trace as warnings.
Since the log will be huge, rate limit to one per minute.
VMWare-BZ: #2054659
Signed-off-by: William Tu <u9012063@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
ofp-util had been far too large and monolithic for a long time. This
commit breaks it up into units that make some logical sense. It also
moves the pieces of ofp-parse that were specific to each unit into the
relevant unit.
Most of this commit is just moving code around.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
An upcoming commit will add another parameter for parsing and formatting
actions. It is much easier to add these parameters if they are
encapsulated in a struct, so this commit first makes that change.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
This makes traffic generated by flow_compose() look slightly more
realistic. It requires lots of updates to tests, but at least the tests
themselves should be slightly more realistic too.
At the same time, add --l7 and --l7-len options to ofproto/trace to allow
users to specify the amount or contents of payloads that they want.
Suggested-by: Brad Cowie <brad@cowie.nz>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
ofproto/trace takes a bunch of options that have weird placement and
syntax. This commit changes the syntax so that the options can be placed
anywhere and consistently use a double-dash option prefix. For
compatibility, the previous syntax is also supported.
An upcoming commit will add new options and this change allows that
upcoming commit to be less confusing.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Free the allocated memory in the pop function.
Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This patch propagates ct_zone when ofproto/trace automatically runs
through the recirculation process.
Fixes: e6bc8e749381 ("ofproto/trace: Add support for tracing conntrack recirculation")
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Reviewed-by: Greg Rose <gvrose8192@gmail.com>
Currently, flow_compose_size() is only supposed to be called after
flow_compose(). I find this API to be unintuitive.
Change flow_compose() API to take the 'size' argument, and
returns 'true' if the packet can be created, 'false' otherwise.
This change also improves error detection and reporting when
'size' is unreasonably small.
Signed-off-by: Andy Zhou <azhou@ovn.org>
Acked-by: Ilya Maximets <i.maximets@samsung.com>
Previous patch enables ofproto/trace to automatically trace a flow
that involves multiple recirculation on conntrack. However, it always
sets the ct_state to trk|est when it processes recirculated conntrack flows.
With this patch, users can customize the expected next ct_state in the
aforementioned use case.
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Previously, a user need to run ofproto/trace multiple times to derive the
final datapath actions if a flow hit conntrack actions that involves
recirculation. To improve the usability of ofproto/trace, in this patch,
we keep track of the conntrack actions, and automatically run the
recirculation process so that a user only need to execute the ofproto/trace
command once. Currently, this patch sets the default ct_state as
trk and new in the automatic recirculation process. A following patch
will provide an option to customize ct_state.
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Until now, ODP output only showed port names for in_port matches. This
commit shows them in other places port numbers appear.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>
Tested-by: Jan Scheurich <jan.scheurich@ericsson.com>
Until now, most ovs-ofctl commands have not accepted names for ports, only
numbers, and have not been able to display port names either. It's a lot
easier for users if they can use and see meaningful names instead of
arbitrary numbers. This commit adds that support.
For backward compatibility, only interactive ovs-ofctl commands by default
display port names; to display them in scripts, use the new --names
option.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Aaron Conole <aconole@redhat.com>
Supply the match mask to prerequisities checking when available. This
allows checking for zero-valued matches. Non-zero valued matches
imply the presense of corresponding mask bits, but for zero valued
matches we must explicitly check the mask, too.
This is required now only for conntrack validity checking due to the
conntrack state having and 'invalid' bit, but not 'valid' bit. One
way to match an valid conntrack state is to match on the 'tracked' bit
being one and 'invalid' bit being zero. The latter requires the
corresponding mask bit be verified.
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Joe Stringer <joe@ovn.org>
ofproto_check_ofpacts() requires ofproto_mutex, but the header did not
tell that so the trace did not take the mutex.
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Signed-off-by: Andy Zhou <azhou@ovn.org>
"ovs-appctl ofproto/trace" is invaluable for debugging, but as the users of
Open vSwitch have evolved it has failed to keep up with the times. It's
pretty easy to design OpenFlow tables and pipelines that resubmit dozens of
times. Each resubmit causes an additional tab of indentation, so the
output wraps around, sometimes again and again, and makes the output close
to unreadable.
ovn-trace pioneered better formatting for tracing in OVN logical datapaths,
mostly by not increasing indentation for tail recursion, which in practice
gets rid of almost all indentation.
This commit experiments with redoing ofproto/trace the same way. Try
looking at, for example, the testsuite output for test 2282 "ovn -- 3 HVs,
3 LRs connected via LS, source IP based routes". Without this commit, it
indents 61 levels (488 spaces!). With this commit, it indents 1 level
(4 spaces) and it's possible to actually understand what's going on almost
at a glance.
To see this for yourself, try the following command either with or without
this commit (but be sure to keep the change to ovn.at that adds an
ofproto/trace to the test):
make check TESTSUITEFLAGS='-d 2282' && less tests/testsuite.dir/2282/testsuite.log
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Lance Richardson <lrichard@redhat.com>
Acked-by: Justin Pettit <jpettit@ovn.org>
An upcoming commit will rewrite much of the ofproto/trace functionality.
As the mechanism behind this grows and evolves, it makes sense to move it
into its own file, especially since ofproto-dpif.c is too big anyway.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Lance Richardson <lrichard@redhat.com>
Acked-by: Justin Pettit <jpettit@ovn.org>
