snprintf will automatically write \0 at the end of the string,
and the last one byte will be out of bound.
create a new function ds_put_hex_with_delimiter,
instead of chassisid_to string and format_hex_arg.
Found in sanitize test.
Signed-off-by: Changliang Wu <changliang.wu@smartx.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
Add helper to truncate the buffer to certain size which might be
useful if some earlier part of the buffer can be reused multiple
times without copying the whole buffer.
Signed-off-by: Ales Musil <amusil@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Commit 933aaf9444a6 re-aligned the fields, so the access to them is
aligned, but it also didn't initialize the added padding. 'ofpacts'
are frequently compared with memcmp() and being hashed as part of the
frozen state causing false negative comparisons and potentially frozen
state lookup failures.
Found by running make check-valgrind on 'continuation - data stack'
tests:
Conditional jump or move depends on uninitialised value(s)
at 0x4EBC82: mhash_add__ (hash.h:66)
by 0x4EBC48: mhash_add (hash.h:78)
by 0x4EB4F8: hash_add (hash.h:109)
by 0x4EBDEC: hash_add64 (hash.h:114)
by 0x4EBDAC: hash_add_words64 (hash.h:439)
by 0x4EB6D2: hash_words64_inline (hash.h:136)
by 0x4EB6A2: hash_words64__ (hash.c:73)
by 0x4595F2: hash_words64 (hash.h:371)
by 0x4593C6: hash_bytes64 (hash.h:399)
by 0x458B76: frozen_state_hash (ofproto-dpif-rid.c:143)
by 0x458CA4: recirc_alloc_id_ctx (ofproto-dpif-rid.c:280)
by 0x483B85: finish_freezing__ (ofproto-dpif-xlate.c:5229)
by 0x47171F: finish_freezing (ofproto-dpif-xlate.c:5271)
by 0x46E8BB: xlate_actions (ofproto-dpif-xlate.c:8340)
by 0x45DC7B: ofproto_trace__ (ofproto-dpif-trace.c:782)
by 0x45D816: ofproto_trace (ofproto-dpif-trace.c:851)
by 0x45E435: ofproto_unixctl_trace (ofproto-dpif-trace.c:490)
by 0x609F5E: process_command (unixctl.c:310)
by 0x6094B9: run_connection (unixctl.c:344)
by 0x609397: unixctl_server_run (unixctl.c:395)
Uninitialised value was created by a stack allocation
at 0x432A44: handle_flow_mod (ofproto.c:6346)
Fix that by properly initializing the whole space allocated for the
set-field action.
Fixes: 933aaf9444a6 ("ofp-actions: Ensure aligned accesses to masked fields.")
Acked-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Found by Coverity:
CID 397544: Incorrect expression (COPY_PASTE_ERROR)
"obs_point_src" in "(*os).obs_point_src.n_bits" looks
like a copy-paste error.
Also adding a test case to cover this situation.
Fixes: 1aa9e137fe36 ("ofp-actions: Load data from fields in sample action.")
Acked-by: Simon Horman <horms@ovn.org>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
When sample action gets used as a way of sampling traffic with
controller-generated metadata (i.e: obs_domain_id and obs_point_id),
the controller will have to increase the number of flows to ensure each
part of the pipeline contains the right metadata.
As an example, if the controller decides to sample stateful traffic, it
could store the computed metadata for each connection in the conntrack
label. However, for established connections, a flow must be created for
each different ct_label value with a sample action that contains a
different hardcoded obs_domain and obs_point id.
This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
that supports specifying the observation point and domain using an
OpenFlow field reference, so now the controller can express:
sample(...
obs_domain_id=NXM_NX_CT_LABEL[0..31],
obs_point_id=NXM_NX_CT_LABEL[32..63]
...
)
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Some openflow actions can be misaligned, e.g., actions whithin OF 1.0
replies to statistics reply messages which have a header of 12 bytes
and no additional padding.
Also, buggy controllers might incorrectly encode actions.
When decoding multiple actions in ofpacts_decode(), make sure that
when advancing to the next action it will be properly aligned
(multiple of OFPACT_ALIGNTO).
Detected by UB Sanitizer when running one of the fuzz tests:
lib/ofp-actions.c:5347:12:
runtime error: member access within misaligned address 0x0000016ba274
for type 'const struct nx_action_learn', which requires 8 byte alignment
0x0000016ba274: note: pointer points here
20 20 20 20 ff ff 00 38 00 00 23 20 00 10 20 20
^
20 20 20 20 20 20 20 20 20 20 20 20 00 03 20 00
0 0x52cece in decode_LEARN_common lib/ofp-actions.c:5347
1 0x52dcf6 in decode_NXAST_RAW_LEARN lib/ofp-actions.c:5463
2 0x548604 in ofpact_decode lib/ofp-actions.inc2:4723
3 0x53ee43 in ofpacts_decode lib/ofp-actions.c:7781
4 0x53efc1 in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7820
5 0x5409e1 in ofpacts_pull_openflow_instructions lib/ofp-actions.c:8396
6 0x5608a8 in ofputil_decode_flow_stats_reply lib/ofp-flow.c:1100
Acked-by: Adrian Moreno <amorenoz@redhat.com>
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
For example is parsing the OVN "eth.dst[40] = 1;" action, which
triggered the following warning from UndefinedBehaviorSanitizer:
lib/meta-flow.c:3210:9:
runtime error: member access within misaligned address 0x000000de4e36
for type 'const union mf_value', which requires 8 byte alignment
0x000000de4e36: note: pointer points here
00 00 00 00 01 00 00 00 00 00 00 00 00 00 70 4e de 00 00 00 00 00
^
10 51 de 00 00 00 00 00 c0 4f
0 0x5818bc in mf_format lib/meta-flow.c:3210
1 0x5b6047 in format_SET_FIELD lib/ofp-actions.c:3342
2 0x5d68ab in ofpact_format lib/ofp-actions.c:9213
3 0x5d6ee0 in ofpacts_format lib/ofp-actions.c:9237
4 0x410922 in test_parse_actions tests/test-ovn.c:1360
To avoid this we now change the internal representation of the set_field
actions, struct ofpact_set_field, such that the mask is always stored
at a correctly aligned address, multiple of OFPACT_ALIGNTO.
We also need to adapt the "ovs-ofctl show-flows - Oversized flow" test
because now the ofpact representation of the set_field action uses more
bytes in memory (for the extra alignment). Change the test to use
dec_ttl instead.
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
This is undefined behavior and was reported by UB Sanitizer:
lib/meta-flow.c:3445:16: runtime error:
member access within null pointer of type 'struct vl_mf_field'
0 0x6aad0f in mf_get_vl_mff lib/meta-flow.c:3445
1 0x6d96d7 in mf_from_oxm_header lib/nx-match.c:260
2 0x6d9e2e in nx_pull_header__ lib/nx-match.c:341
3 0x6daafa in nx_pull_header lib/nx-match.c:488
4 0x6abcb6 in mf_vl_mff_nx_pull_header lib/meta-flow.c:3605
5 0x73b9be in decode_NXAST_RAW_REG_MOVE lib/ofp-actions.c:2652
6 0x764ccd in ofpact_decode lib/ofp-actions.inc2:4681
[...]
lib/sset.c:315:12: runtime error: applying zero offset to null pointer
0 0xcc2e6a in sset_at_position lib/sset.c:315:12
1 0x5734b3 in port_dump_next ofproto/ofproto-dpif.c:4083:20
[...]
lib/ovsdb-data.c:2194:56:
runtime error: applying zero offset to null pointer
0 0x5e9530 in ovsdb_datum_added_removed lib/ovsdb-data.c:2194:56
1 0x4d6258 in update_row_ref_count ovsdb/transaction.c:335:17
2 0x4c360b in for_each_txn_row ovsdb/transaction.c:1572:33
[...]
lib/ofpbuf.c:440:30:
runtime error: applying zero offset to null pointer
0 0x75066d in ofpbuf_push_uninit lib/ofpbuf.c:440
1 0x46ac8a in ovnacts_parse lib/actions.c:4190
2 0x46ad91 in ovnacts_parse_string lib/actions.c:4208
3 0x4106d1 in test_parse_actions tests/test-ovn.c:1324
[...]
lib/ofp-actions.c:3205:22:
runtime error: applying non-zero offset 2 to null pointer
0 0x6e1641 in set_field_split_str lib/ofp-actions.c:3205:22
[...]
lib/tnl-ports.c:74:12:
runtime error: applying zero offset to null pointer
0 0xceffe7 in tnl_port_cast lib/tnl-ports.c:74:12
1 0xcf14c3 in map_insert lib/tnl-ports.c:116:13
[...]
ofproto/ofproto.c:8905:16:
runtime error: applying zero offset to null pointer
0 0x556795 in eviction_group_hash_rule ofproto/ofproto.c:8905:16
1 0x503f8d in eviction_group_add_rule ofproto/ofproto.c:9022:42
[...]
Also, it's valid to have an empty ofpact list and we should be able to
try to iterate through it.
UB Sanitizer report:
include/openvswitch/ofp-actions.h:222:12:
runtime error: applying zero offset to null pointer
0 0x665d69 in ofpact_end ./include/openvswitch/ofp-actions.h:222:12
1 0x66b2cf in ofpacts_put_openflow_actions lib/ofp-actions.c:8861:5
2 0x6ffdd1 in ofputil_encode_flow_mod lib/ofp-flow.c:447:9
[...]
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
GCC 11.2.1-2.2 emits a false-positive warnings like:
lib/ofp-packet.c: In function 'ofputil_decode_packet_in':
lib/ofp-packet.c:155:25: warning: 'reason' may be used
uninitialized [-Wmaybe-uninitialized]
lib/ofp-packet.c: In function 'ofputil_decode_packet_in_private':
lib/ofp-packet.c:886:27: warning: 'value' may be used
uninitialized [-Wmaybe-uninitialized]
Modifying callers of ofpprop_parse_* functions to always check
the return value before using the value from these functions.
Signed-off-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
It's actually undefined behavior to pass NULL to standard library
functions that manipulate arrays (e.g., qsort, memcpy, memcmp), even if
the passed number of items is 0.
UB Sanitizer reports:
ovsdb/monitor.c:408:9: runtime error: null pointer passed as argument 1,
which is declared to never be null
#0 0x406ae1 in ovsdb_monitor_columns_sort ovsdb/monitor.c:408
#1 0x406ae1 in ovsdb_monitor_add ovsdb/monitor.c:1683
[...]
lib/ovsdb-data.c:1970:5: runtime error: null pointer passed as argument 2,
which is declared to never be null
#0 0x4071c8 in ovsdb_datum_push_unsafe lib/ovsdb-data.c:1970
#1 0x471cd0 in ovsdb_datum_apply_diff_in_place lib/ovsdb-data.c:2345
[...]
ofproto/ofproto-dpif-rid.c:159:17:
runtime error: null pointer passed as argument 1,
which is declared to never be null
#0 0x4df5d8 in frozen_state_equal ofproto/ofproto-dpif-rid.c:159
#1 0x4dfd27 in recirc_find_equal ofproto/ofproto-dpif-rid.c:179
[...]
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Paolo Valerio <pvalerio@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
The encap & decap actions are extended to support MPLS packet type.
Encap & decap actions adds and removes MPLS header at start of the
packet.
The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS
header between ethernet header and the IP header. Though this behaviour
is fine for L3 VPN where an IP packet is encapsulated inside a MPLS
tunnel, it does not suffice the L2 VPN requirements. In L2 VPN the
ethernet packets must be encapsulated inside MPLS tunnel.
In this change the encap & decap actions are extended to support MPLS
packet type. The encap & decap adds and removes MPLS header at the
start of packet as depicted below.
Encapsulation:
Actions - encap(mpls),encap(ethernet)
Incoming packet -> | ETH | IP | Payload |
1 Actions - encap(mpls) [Datapath action - ADD_MPLS:0x8847]
Outgoing packet -> | MPLS | ETH | Payload|
2 Actions - encap(ethernet) [ Datapath action - push_eth ]
Outgoing packet -> | ETH | MPLS | ETH | Payload|
Decapsulation:
Incoming packet -> | ETH | MPLS | ETH | IP | Payload |
Actions - decap(),decap(packet_type(ns=0,type=0))
1 Actions - decap() [Datapath action - pop_eth)
Outgoing packet -> | MPLS | ETH | IP | Payload|
2 Actions - decap(packet_type(ns=0,type=0)) [Datapath action - POP_MPLS:0x6558]
Outgoing packet -> | ETH | IP | Payload|
Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Not a very important fix, but fuzzer times out trying to test parsing
of a huge number of actions. Fixing that by reporting an error as
soon as ofpacts oversized.
It would be great to use ofpbuf_oversized() function instead of manual
size checking, but ofpacts->header here always points to the last
pushed action, so the value that ofpbuf_oversized() would check is
always small.
Adding a unit test for this, plus the extra test for too deep nesting.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20254
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Alin-Gabriel Serdean <aserdean@ovn.org>
Apart from a cut-and-paste typo, the man page claims that mpls_labels
can be provided in hexadecimal format but that's currently not the case.
Fix mpls ofp-action formatting, add size checks on ofp-action parsing
and add some unit tests.
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
While decoding RAW_ENCAP action, decode_ed_prop() might re-allocate
ofpbuf if there is no enough space left. However, function
'decode_NXAST_RAW_ENCAP' continues to use old pointer to 'encap'
structure leading to write-after-free and incorrect decoding.
==3549105==ERROR: AddressSanitizer: heap-use-after-free on address
0x60600000011a at pc 0x0000005f6cc6 bp 0x7ffc3a2d4410 sp 0x7ffc3a2d4408
WRITE of size 2 at 0x60600000011a thread T0
#0 0x5f6cc5 in decode_NXAST_RAW_ENCAP lib/ofp-actions.c:4461:20
#1 0x5f0551 in ofpact_decode ./lib/ofp-actions.inc2:4777:16
#2 0x5ed17c in ofpacts_decode lib/ofp-actions.c:7752:21
#3 0x5eba9a in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7791:13
#4 0x5eb9fc in ofpacts_pull_openflow_actions lib/ofp-actions.c:7835:12
#5 0x64bb8b in ofputil_decode_packet_out lib/ofp-packet.c:1113:17
#6 0x65b6f4 in ofp_print_packet_out lib/ofp-print.c:148:13
#7 0x659e3f in ofp_to_string__ lib/ofp-print.c:1029:16
#8 0x659b24 in ofp_to_string lib/ofp-print.c:1244:21
#9 0x65a28c in ofp_print lib/ofp-print.c:1288:28
#10 0x540d11 in ofctl_ofp_parse utilities/ovs-ofctl.c:2814:9
#11 0x564228 in ovs_cmdl_run_command__ lib/command-line.c:247:17
#12 0x56408a in ovs_cmdl_run_command lib/command-line.c:278:5
#13 0x5391ae in main utilities/ovs-ofctl.c:179:9
#14 0x7f6911ce9081 in __libc_start_main (/lib64/libc.so.6+0x27081)
#15 0x461fed in _start (utilities/ovs-ofctl+0x461fed)
Fix that by getting a new pointer before using.
Credit to OSS-Fuzz.
Fuzzer regression test will fail only with AddressSanitizer enabled.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27851
Fixes: f839892a206a ("OF support and translation of generic encap and decap")
Acked-by: William Tu <u9012063@gmail.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
The new term is "member".
Most of these changes should not change user-visible behavior. One
place where they do is in "ovs-ofctl dump-flows", which will now output
"members:..." inside "bundle" actions instead of "slaves:...". I don't
expect this to cause real problems in most systems. The old syntax
is still supported on input for backward compatibility.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
This patch adds a new OpenFlow action, delete field, to delete a
field in packets. Currently, only the tun_metadata fields are
supported.
One use case to add this action is to support multiple versions
of geneve tunnel metadatas to be exchanged among different versions
of networks. For example, we may introduce tun_metadata2 to
replace old tun_metadata1, but still want to provide backward
compatibility to the older release. In this case, in the new
OpenFlow pipeline, we would like to support the case to receive a
packet with tun_metadata1, do some processing. And if the packet
is going to a switch in the newer release, we would like to delete
the value in tun_metadata1 and set a value into tun_metadata2.
Currently, ovs does not provide an action to remove a value in
tun_metadata if the value is present. This patch fulfills the gap
by adding the delete_field action. For example, the OpenFlow
syntax to delete tun_metadata1 is:
actions=delete_field:tun_metadata1
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: William Tu <u9012063@gmail.com>
Acked-by: William Tu <u9012063@gmail.com>
Need to free the memory before return. Detected by gcc10.
Signed-off-by: William Tu <u9012063@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
OpenFlow 1.5 changed "meter" from an instruction to an action. This commit
supports it properly.
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
These duplicate messages emitted by ofpacts_verify(), so drop them.
These were previously useful because ofpacts_verify()'s error messages were
not as good as those emitted by ofpacts_parse__(), but that's been fixed
now.
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Verification can fail for a variety of reasons but the code here always
reported "Incorrect instruction ordering".
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Otherwise, specifying something like output(port=1,max_len=5) would parse
OK and then cause a failure when it was received by the switch.
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The action expects 'type' as a parameter name so it should use 'type' when
it formats actions too.
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
This patch adds a new action 'check_pkt_larger' which checks if the
packet is larger than the given size and stores the result in the
destination register.
Usage: check_pkt_larger(len)->REGISTER
Eg. match=...,actions=check_pkt_larger(1442)->NXM_NX_REG0[0],next;
This patch makes use of the new datapath action - 'check_pkt_len'
which was recently added in the commit [1].
At the start of ovs-vswitchd, datapath is probed for this action.
If the datapath action is present, then 'check_pkt_larger'
makes use of this datapath action.
Datapath action 'check_pkt_len' takes these nlattrs
* OVS_CHECK_PKT_LEN_ATTR_PKT_LEN - 'pkt_len' to check for
* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER (optional) - Nested actions
to apply if the packet length is greater than the specified 'pkt_len'
* OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL (optional) - Nested
actions to apply if the packet length is lesser or equal to the
specified 'pkt_len'.
Let's say we have these flows added to an OVS bridge br-int
table=0, priority=100 in_port=1,ip,actions=check_pkt_larger:100->NXM_NX_REG0[0],resubmit(,1)
table=1, priority=200,in_port=1,ip,reg0=0x1/0x1 actions=output:3
table=1, priority=100,in_port=1,ip,actions=output:4
Then the action 'check_pkt_larger' will be translated as
- check_pkt_len(size=100,gt(3),le(4))
datapath will check the packet length and if the packet length is greater than 100,
it will output to port 3, else it will output to port 4.
In case, datapath doesn't support 'check_pkt_len' action, the OVS action
'check_pkt_larger' sets SLOW_ACTION so that datapath flow is not added.
This OVS action is intended to be used by OVN to check the packet length
and generate an ICMP packet with type 3, code 4 and next hop mtu
in the logical router pipeline if the MTU of the physical interface
is lesser than the packet length. More information can be found here [2]
[1] - 4d5ec89fc8
[2] - https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
Suggested-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
CC: Ben Pfaff <blp@ovn.org>
CC: Gregory Rose <gvrose8192@gmail.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
If enough large input is given ofpact_finish will fail.
Implemented ofpbuf_oversized function to check for oversized
buffer. Checks were added for parse functions and error messages
returned.
Basic manual testing performed.
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reported-by: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=12972
Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
ONF abandoned the OpenFlow specification, so that OpenFlow 1.6 will never
be completed. It did not contain much in the way of useful features, so
remove what support Open vSwitch already had.
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
The functions to put ofpacts into ofpbufs have always padded them to
OFPACT_ALIGNTO boundaries, but the underlying structures weren't
necessarily padded out. That led to difficulties in a few places where
structures were allocated on the stack instead in an ofpbuf, because
functions like ofpact_init_*() would access beyond the end of the actual
structure. This is true, for example, in test_multipath_main() in
tests/test-multipath.c, which allocates a struct ofpact_multipath on the
stack, and in lswitch_handshake() in learning-switch.c, which allocates
a struct ofpact_output on the stack.
It's possible to fix these individual cases, but it's possible that there
are others that haven't been identified. This commit addresses the issue
another way, by padding all of the ofpact structures to a full multiple
of OFPACT_ALIGNTO and adding assertions to ensure that it can't be screwed
up in the future.
This commit removes the OFPACT_*_SIZE enums, because they are now
equivalent to sizeof(struct ofpact_*) in every case.
Acked-by: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Currently, if unroll_xlate is passed to ovs-ofctl as one of actions,
let say 'ovs-ofctl add-flow br0 in_port=1,actions=unroll_xlate',
ovs-ofctl will crash. This patch fixes it by returning an error
message.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11184
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
A previous commit attempted to fix the error path when the actions nested
within clone provoked an error. However, this commit just introduced a new
problem in another case, since it made ofpacts_pull_openflow_actions__()
restore a previously valid pointer to data that might have been
reallocated.
This commit takes another approach. Instead of trying to restore anything
at all, it just defines ofpacts_pull_openflow_actions__() to clear the
output buffer when there's an error. It seems that this is less error
prone. Most of the callers don't care; this commit fixes up the ones that
do.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9975
Fixes: 20cdd1dbd546 ("ofp-actions: Avoid assertion failure for clone(ct(...bad actions...)).")
Signed-off-by: Ben Pfaff <blp@ovn.org>
Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
decode_NXAST_RAW_CT() temporarily pulls data off the beginning of its
ofpacts output ofpbuf and, on its error path, fails to push it back on.
At a higher layer, decode_NXAST_RAW_CLONE() asserts, via
ofpact_finish_CLONE(), that the ofpact_clone that it put is still in the
place where it put it, which causes an assertion failure.
The root cause here is the failure to re-push the clone header. One could
fix that, but it would be pretty easy for that to go wrong again on some
other obscure error path. Instead, this commit just makes the problem go
away by always saving and restoring 'ofpact->data' if a decode fails.
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9862
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>
Shifting a 16-bit signed int 16 bits is technically undefined behavior.
This fixes the problem. (In practice this should be harmless in this
case.)
Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9049
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
The previous implementation scans the action set of each WRITE_ACTIONS
command 13--17 times when moving the actions over. This change builds
up the list as a single scan, which should be more efficient.
Signed-off-by: Kyle Simpson <kyleandrew.simpson@gmail.com>
Co-authored-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Ben Pfaff <blp@ovn.org>
The actions encap, decap and dec_nsh_ttl were wrongly flagged as set_field
actions in ofpact_is_set_or_move_action(). This caused them to be executed
twice in the action set or a group bucket, once explicitly in
ofpacts_execute_action_set() and once again as part of the list of
set_field or move actions.
Fixes: f839892a ("OF support and translation of generic encap and decap")
Fixes: 491e05c2 ("nsh: add dec_nsh_ttl action")
Signed-off-by: Jan Scheurich <jan.scheurich@ericsson.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>
OpenFlow has little-known support for naming tables. Open vSwitch has
supported table names for ages, but it has never used or displayed them
outside of commands dedicated to table manipulation. This commit adds
support for table names in ovs-ofctl. When a table has a name, it displays
that name in flows and actions, so that, for example, the following:
table=1, arp, actions=resubmit(,2)
might become:
table=ingress_acl, arp, actions=resubmit(,mac_learning)
given appropriately named tables.
For backward compatibility, only interactive ovs-ofctl commands by default
display table names; to display them in scripts, use the new --names
option.
This feature was inspired by a talk that Kei Nohguchi presented at Open
vSwitch 2017 Fall Conference.
CC: Kei Nohguchi <kei@nohguchi.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Mark Michelson <mmichels@redhat.com>
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 supports using the ct_clear action in the kernel datapath. To
preserve compatibility with current ct_clear behavior on old kernels, we
only pass this action down to the datapath if a probe reveals the
datapath actually supports it.
Signed-off-by: Eric Garver <e@erig.me>
Acked-by: William Tu <u9012063@gmail.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
Signed-off-by: Justin Pettit <jpettit@ovn.org>
NSH ttl is a 6-bit field ranged from 0 to 63, it should be
decremented by 1 every hop, if it is 0 or it is so after
decremented, the packet should be dropped and a packet-in
message is sent to main controller.
Signed-off-by: Yi Yang <yi.y.yang@intel.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
It isn't otherwise useful and in fact hurts performance so it's disabled
without --enable-dummy.
An upcoming commit will make use of this.
Signed-off-by: Justin Pettit <jpettit@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>
This may help debugging difficult controller problems.
Reported-by: Su Wang <suwang@vmware.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Justin Pettit <jpettit@ovn.org>
