refactor parser and enable XDP parsing #11

williamtu · 2018-08-10T17:47:33Z

  1: datapath - basic BPF commands                   ok
  2: datapath - ping between two ports               ok
  3: datapath - http between two ports               ok
  4: datapath - ping between two ports on vlan       ok
  5: datapath - ping between two ports on cvlan      ok
  6: datapath - ping6 between two ports              ok
  7: datapath - ping6 between two ports on vlan      ok
  8: datapath - ping6 between two ports on cvlan     ok
  9: datapath - ping over bond                       skipped (system-bpf-traffic.at:210)
 10: datapath - ping over vxlan tunnel               ok
 11: datapath - ping over vxlan6 tunnel              ok
 12: datapath - ping over gre tunnel                 ok
 13: datapath - ping over geneve tunnel              ok
 14: datapath - ping over geneve tunnel with TLV     ok
 15: datapath - ping over geneve6 tunnel             ok
 16: datapath - clone action                         FAILED (system-bpf-traffic.at:487)
 17: datapath - mpls actions                         FAILED (system-bpf-traffic.at:526)
 18: datapath - basic truncate action                FAILED (system-bpf-traffic.at:586)
 19: datapath - truncate and output to gre tunnel    FAILED (system-bpf-traffic.at:720)
 20: ovn -- 1 LR connects 2 LSes                     ok

…into rewrite-parser

The patch does the following: 1) Refactor the original parser into data parsing and metadata parsing 2) Move the packet data parsing code to parser_common.h 3) Enable xdp.h to parse packet As a result, TC and XDP share the same packet data parsing code. If a netdev supports XDP, the packet will be first parsed at XDP, save in percpu_headers map, and passes to the upper TC metadata parsing (percpu_metadata map). If not, then the TC does full parsing. Signed-off-by: William Tu <u9012063@gmail.com>

…ound Upstream commit: commit 72f17baf2352ded6a1d3f4bb2d15da8c678cd2cb Author: Stefano Brivio <sbrivio@redhat.com> Date: Thu May 3 18:13:25 2018 +0200 openvswitch: Don't swap table in nlattr_set() after OVS_ATTR_NESTED is found If an OVS_ATTR_NESTED attribute type is found while walking through netlink attributes, we call nlattr_set() recursively passing the length table for the following nested attributes, if different from the current one. However, once we're done with those sub-nested attributes, we should continue walking through attributes using the current table, instead of using the one related to the sub-nested attributes. For example, given this sequence: 1 OVS_KEY_ATTR_PRIORITY 2 OVS_KEY_ATTR_TUNNEL 3 OVS_TUNNEL_KEY_ATTR_ID 4 OVS_TUNNEL_KEY_ATTR_IPV4_SRC 5 OVS_TUNNEL_KEY_ATTR_IPV4_DST 6 OVS_TUNNEL_KEY_ATTR_TTL 7 OVS_TUNNEL_KEY_ATTR_TP_SRC 8 OVS_TUNNEL_KEY_ATTR_TP_DST 9 OVS_KEY_ATTR_IN_PORT 10 OVS_KEY_ATTR_SKB_MARK 11 OVS_KEY_ATTR_MPLS we switch to the 'ovs_tunnel_key_lens' table on attribute #3, and we don't switch back to 'ovs_key_lens' while setting attributes #9 to #11 in the sequence. As OVS_KEY_ATTR_MPLS evaluates to 21, and the array size of 'ovs_tunnel_key_lens' is 15, we also get this kind of KASan splat while accessing the wrong table: [ 7654.586496] ================================================================== [ 7654.594573] BUG: KASAN: global-out-of-bounds in nlattr_set+0x164/0xde9 [openvswitch] [ 7654.603214] Read of size 4 at addr ffffffffc169ecf0 by task handler29/87430 [ 7654.610983] [ 7654.612644] CPU: 21 PID: 87430 Comm: handler29 Kdump: loaded Not tainted 3.10.0-866.el7.test.x86_64 #1 [ 7654.623030] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.1.7 06/16/2016 [ 7654.631379] Call Trace: [ 7654.634108] [<ffffffffb65a7c50>] dump_stack+0x19/0x1b [ 7654.639843] [<ffffffffb53ff373>] print_address_description+0x33/0x290 [ 7654.647129] [<ffffffffc169b37b>] ? nlattr_set+0x164/0xde9 [openvswitch] [ 7654.654607] [<ffffffffb53ff812>] kasan_report.part.3+0x242/0x330 [ 7654.661406] [<ffffffffb53ff9b4>] __asan_report_load4_noabort+0x34/0x40 [ 7654.668789] [<ffffffffc169b37b>] nlattr_set+0x164/0xde9 [openvswitch] [ 7654.676076] [<ffffffffc167ef68>] ovs_nla_get_match+0x10c8/0x1900 [openvswitch] [ 7654.684234] [<ffffffffb61e9cc8>] ? genl_rcv+0x28/0x40 [ 7654.689968] [<ffffffffb61e7733>] ? netlink_unicast+0x3f3/0x590 [ 7654.696574] [<ffffffffc167dea0>] ? ovs_nla_put_tunnel_info+0xb0/0xb0 [openvswitch] [ 7654.705122] [<ffffffffb4f41b50>] ? unwind_get_return_address+0xb0/0xb0 [ 7654.712503] [<ffffffffb65d9355>] ? system_call_fastpath+0x1c/0x21 [ 7654.719401] [<ffffffffb4f41d79>] ? update_stack_state+0x229/0x370 [ 7654.726298] [<ffffffffb4f41d79>] ? update_stack_state+0x229/0x370 [ 7654.733195] [<ffffffffb53fe4b5>] ? kasan_unpoison_shadow+0x35/0x50 [ 7654.740187] [<ffffffffb53fe62a>] ? kasan_kmalloc+0xaa/0xe0 [ 7654.746406] [<ffffffffb53fec32>] ? kasan_slab_alloc+0x12/0x20 [ 7654.752914] [<ffffffffb53fe711>] ? memset+0x31/0x40 [ 7654.758456] [<ffffffffc165bf92>] ovs_flow_cmd_new+0x2b2/0xf00 [openvswitch] [snip] [ 7655.132484] The buggy address belongs to the variable: [ 7655.138226] ovs_tunnel_key_lens+0xf0/0xffffffffffffd400 [openvswitch] [ 7655.145507] [ 7655.147166] Memory state around the buggy address: [ 7655.152514] ffffffffc169eb80: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa [ 7655.160585] ffffffffc169ec00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 7655.168644] >ffffffffc169ec80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa [ 7655.176701] ^ [ 7655.184372] ffffffffc169ed00: fa fa fa fa 00 00 00 00 fa fa fa fa 00 00 00 05 [ 7655.192431] ffffffffc169ed80: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00 [ 7655.200490] ================================================================== Reported-by: Hangbin Liu <liuhangbin@gmail.com> Fixes: 982b52700482 ("openvswitch: Fix mask generation for nested attributes.") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com> Tested-by: Greg Rose <gvrose8192@gmail.com> Signed-off-by: Justin Pettit <jpettit@ovn.org>

There is a coredump when I add and delete bridges. When the rcu thread call ofproto_destroy__, the main thread may call ofproto_create. But the ofproto_destroy__ fun doesn't have the ofproto_mutex when access the all_ofprotos. #0 0x00007f824aa0d197 in raise () from /usr/lib64/libc.so.6 #1 0x00007f824aa0e888 in abort () from /usr/lib64/libc.so.6 #2 0x0000000000658249 in PAT_abort () #3 0x000000000065538d in patchIllInsHandler () #4 <signal handler called> #5 0x0000000000478a5b in hmap_remove (node=0x3320150, hmap=0x95fc40 <all_ofprotos>) at include/openvswitch/hmap.h:287 #6 ofproto_destroy__ (ofproto=0x3320150) at ofproto/ofproto.c:1642 #7 0x0000000000535e46 in ovsrcu_call_postponed () at lib/ovs_rcu.c:323 #8 0x0000000000536014 in ovsrcu_postpone_thread (arg=<optimized out>) at lib/ovs_rcu.c:338 #9 0x0000000000538488 in ovsthread_wrapper (aux_=<optimized out>) at lib/ovs_thread.c:682 #10 0x00007f824c130dc5 in start_thread () from /usr/lib64/libpthread.so.0 #11 0x00007f824aacf7bd in clone () from /usr/lib64/libc.so.6 Signed-off-by: Cheng Liu <liucheng11@huawei.com> Signed-off-by: Ben Pfaff <blp@ovn.org>

The following, run inside the OVS sandbox, caused OVS to abort when Address Sanitizer was used: ovs-vsctl add-br br-int ovs-ofctl add-flow br-int "table=0,cookie=0x1234,priority=10000,icmp,actions=drop" ovs-ofctl --strict del-flows br-int "table=0,cookie=0x1234/-1,priority=10000" Sample report from Address Sanitizer: ==3029==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000043260 at pc 0x7f6b09c2459b bp 0x7ffcb67e7540 sp 0x7ffcb67e6cf0 READ of size 40 at 0x603000043260 thread T0 #0 0x7f6b09c2459a (/lib/x86_64-linux-gnu/libasan.so.5+0xb859a) #1 0x565110a748a5 in minimask_equal ../lib/flow.c:3510 #2 0x565110a9ea41 in minimatch_equal ../lib/match.c:1821 #3 0x56511091e864 in collect_rules_strict ../ofproto/ofproto.c:4516 #4 0x56511093d526 in delete_flow_start_strict ../ofproto/ofproto.c:5959 #5 0x56511093d526 in ofproto_flow_mod_start ../ofproto/ofproto.c:7949 #6 0x56511093d77b in handle_flow_mod__ ../ofproto/ofproto.c:6122 #7 0x56511093db71 in handle_flow_mod ../ofproto/ofproto.c:6099 #8 0x5651109407f6 in handle_single_part_openflow ../ofproto/ofproto.c:8406 #9 0x5651109407f6 in handle_openflow ../ofproto/ofproto.c:8587 #10 0x5651109e40da in ofconn_run ../ofproto/connmgr.c:1318 #11 0x5651109e40da in connmgr_run ../ofproto/connmgr.c:355 #12 0x56511092b129 in ofproto_run ../ofproto/ofproto.c:1826 #13 0x5651108f23cd in bridge_run__ ../vswitchd/bridge.c:2965 #14 0x565110904887 in bridge_run ../vswitchd/bridge.c:3023 #15 0x5651108e659c in main ../vswitchd/ovs-vswitchd.c:127 #16 0x7f6b093b709a in __libc_start_main ../csu/libc-start.c:308 #17 0x5651108e9009 in _start (/home/blp/nicira/ovs/_build/vswitchd/ovs-vswitchd+0x11d009) This fixes the problem, which although largely theoretical could crop up with odd implementations of memcmp(), perhaps ones optimized in various "clever" ways. All in all, it seems best to avoid the theoretical problem. Acked-by: Dumitru Ceara <dceara@redhat.com> Signed-off-by: Ben Pfaff <blp@ovn.org>

…ound Upstream commit: commit 72f17baf2352ded6a1d3f4bb2d15da8c678cd2cb Author: Stefano Brivio <sbrivio@redhat.com> Date: Thu May 3 18:13:25 2018 +0200 openvswitch: Don't swap table in nlattr_set() after OVS_ATTR_NESTED is found If an OVS_ATTR_NESTED attribute type is found while walking through netlink attributes, we call nlattr_set() recursively passing the length table for the following nested attributes, if different from the current one. However, once we're done with those sub-nested attributes, we should continue walking through attributes using the current table, instead of using the one related to the sub-nested attributes. For example, given this sequence: 1 OVS_KEY_ATTR_PRIORITY 2 OVS_KEY_ATTR_TUNNEL 3 OVS_TUNNEL_KEY_ATTR_ID 4 OVS_TUNNEL_KEY_ATTR_IPV4_SRC 5 OVS_TUNNEL_KEY_ATTR_IPV4_DST 6 OVS_TUNNEL_KEY_ATTR_TTL 7 OVS_TUNNEL_KEY_ATTR_TP_SRC 8 OVS_TUNNEL_KEY_ATTR_TP_DST 9 OVS_KEY_ATTR_IN_PORT 10 OVS_KEY_ATTR_SKB_MARK 11 OVS_KEY_ATTR_MPLS we switch to the 'ovs_tunnel_key_lens' table on attribute williamtu#3, and we don't switch back to 'ovs_key_lens' while setting attributes williamtu#9 to williamtu#11 in the sequence. As OVS_KEY_ATTR_MPLS evaluates to 21, and the array size of 'ovs_tunnel_key_lens' is 15, we also get this kind of KASan splat while accessing the wrong table: [ 7654.586496] ================================================================== [ 7654.594573] BUG: KASAN: global-out-of-bounds in nlattr_set+0x164/0xde9 [openvswitch] [ 7654.603214] Read of size 4 at addr ffffffffc169ecf0 by task handler29/87430 [ 7654.610983] [ 7654.612644] CPU: 21 PID: 87430 Comm: handler29 Kdump: loaded Not tainted 3.10.0-866.el7.test.x86_64 williamtu#1 [ 7654.623030] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.1.7 06/16/2016 [ 7654.631379] Call Trace: [ 7654.634108] [<ffffffffb65a7c50>] dump_stack+0x19/0x1b [ 7654.639843] [<ffffffffb53ff373>] print_address_description+0x33/0x290 [ 7654.647129] [<ffffffffc169b37b>] ? nlattr_set+0x164/0xde9 [openvswitch] [ 7654.654607] [<ffffffffb53ff812>] kasan_report.part.3+0x242/0x330 [ 7654.661406] [<ffffffffb53ff9b4>] __asan_report_load4_noabort+0x34/0x40 [ 7654.668789] [<ffffffffc169b37b>] nlattr_set+0x164/0xde9 [openvswitch] [ 7654.676076] [<ffffffffc167ef68>] ovs_nla_get_match+0x10c8/0x1900 [openvswitch] [ 7654.684234] [<ffffffffb61e9cc8>] ? genl_rcv+0x28/0x40 [ 7654.689968] [<ffffffffb61e7733>] ? netlink_unicast+0x3f3/0x590 [ 7654.696574] [<ffffffffc167dea0>] ? ovs_nla_put_tunnel_info+0xb0/0xb0 [openvswitch] [ 7654.705122] [<ffffffffb4f41b50>] ? unwind_get_return_address+0xb0/0xb0 [ 7654.712503] [<ffffffffb65d9355>] ? system_call_fastpath+0x1c/0x21 [ 7654.719401] [<ffffffffb4f41d79>] ? update_stack_state+0x229/0x370 [ 7654.726298] [<ffffffffb4f41d79>] ? update_stack_state+0x229/0x370 [ 7654.733195] [<ffffffffb53fe4b5>] ? kasan_unpoison_shadow+0x35/0x50 [ 7654.740187] [<ffffffffb53fe62a>] ? kasan_kmalloc+0xaa/0xe0 [ 7654.746406] [<ffffffffb53fec32>] ? kasan_slab_alloc+0x12/0x20 [ 7654.752914] [<ffffffffb53fe711>] ? memset+0x31/0x40 [ 7654.758456] [<ffffffffc165bf92>] ovs_flow_cmd_new+0x2b2/0xf00 [openvswitch] [snip] [ 7655.132484] The buggy address belongs to the variable: [ 7655.138226] ovs_tunnel_key_lens+0xf0/0xffffffffffffd400 [openvswitch] [ 7655.145507] [ 7655.147166] Memory state around the buggy address: [ 7655.152514] ffffffffc169eb80: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa [ 7655.160585] ffffffffc169ec00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ 7655.168644] >ffffffffc169ec80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa [ 7655.176701] ^ [ 7655.184372] ffffffffc169ed00: fa fa fa fa 00 00 00 00 fa fa fa fa 00 00 00 05 [ 7655.192431] ffffffffc169ed80: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00 [ 7655.200490] ================================================================== Reported-by: Hangbin Liu <liuhangbin@gmail.com> Fixes: 982b52700482 ("openvswitch: Fix mask generation for nested attributes.") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> Reviewed-by: Greg Rose <gvrose8192@gmail.com> Tested-by: Greg Rose <gvrose8192@gmail.com> Signed-off-by: Justin Pettit <jpettit@ovn.org>

Should use flow->actions not &flow->actions. here is ASAN report: ================================================================= ==57189==ERROR: AddressSanitizer: stack-buffer-overflow on address 0xffff428fa0e8 at pc 0xffff7f61a520 bp 0xffff428f9420 sp 0xffff428f9498 READ of size 196 at 0xffff428fa0e8 thread T150 (revalidator22) #0 0xffff7f61a51f in __interceptor_memcpy (/lib64/libasan.so.4+0xa251f) williamtu#1 0xaaaad26a3b2b in ofpbuf_put lib/ofpbuf.c:426 williamtu#2 0xaaaad26a30cb in ofpbuf_clone_data_with_headroom lib/ofpbuf.c:248 williamtu#3 0xaaaad26a2e77 in ofpbuf_clone_with_headroom lib/ofpbuf.c:218 williamtu#4 0xaaaad26a2dc3 in ofpbuf_clone lib/ofpbuf.c:208 williamtu#5 0xaaaad23e3993 in ukey_set_actions ofproto/ofproto-dpif-upcall.c:1640 williamtu#6 0xaaaad23e3f03 in ukey_create__ ofproto/ofproto-dpif-upcall.c:1696 williamtu#7 0xaaaad23e553f in ukey_create_from_dpif_flow ofproto/ofproto-dpif-upcall.c:1806 williamtu#8 0xaaaad23e65fb in ukey_acquire ofproto/ofproto-dpif-upcall.c:1984 williamtu#9 0xaaaad23eb583 in revalidate ofproto/ofproto-dpif-upcall.c:2625 williamtu#10 0xaaaad23dee5f in udpif_revalidator ofproto/ofproto-dpif-upcall.c:1076 williamtu#11 0xaaaad26b84ef in ovsthread_wrapper lib/ovs-thread.c:708 williamtu#12 0xffff7e74a8bb in start_thread (/lib64/libpthread.so.0+0x78bb) williamtu#13 0xffff7e0665cb in thread_start (/lib64/libc.so.6+0xd55cb) Address 0xffff428fa0e8 is located in stack of thread T150 (revalidator22) at offset 328 in frame #0 0xaaaad23e4cab in ukey_create_from_dpif_flow ofproto/ofproto-dpif-upcall.c:1762 This frame has 4 object(s): [32, 96) 'actions' [128, 192) 'buf' [224, 328) 'full_flow' [384, 2432) 'stub' <== Memory access at offset 328 partially underflows this variable HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext (longjmp and C++ exceptions *are* supported) Thread T150 (revalidator22) created by T0 here: #0 0xffff7f5b0f7f in __interceptor_pthread_create (/lib64/libasan.so.4+0x38f7f) williamtu#1 0xaaaad26b891f in ovs_thread_create lib/ovs-thread.c:792 williamtu#2 0xaaaad23dc62f in udpif_start_threads ofproto/ofproto-dpif-upcall.c:639 williamtu#3 0xaaaad23daf87 in ofproto_set_flow_table ofproto/ofproto-dpif-upcall.c:446 williamtu#4 0xaaaad230ff7f in dpdk_evs_cfg_set vswitchd/bridge.c:1134 williamtu#5 0xaaaad2310097 in bridge_reconfigure vswitchd/bridge.c:1148 williamtu#6 0xaaaad23279d7 in bridge_run vswitchd/bridge.c:3944 williamtu#7 0xaaaad23365a3 in main vswitchd/ovs-vswitchd.c:240 williamtu#8 0xffff7dfb1adf in __libc_start_main (/lib64/libc.so.6+0x20adf) williamtu#9 0xaaaad230a3d3 (/usr/sbin/ovs-vswitchd-2.7.0-1.1.RC5.001.asan+0x26f3d3) SUMMARY: AddressSanitizer: stack-buffer-overflow (/lib64/libasan.so.4+0xa251f) in __interceptor_memcpy Shadow bytes around the buggy address: 0x200fe851f3c0: 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2 00 00 00 00 0x200fe851f3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f3f0: 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00 00 00 0x200fe851f400: f2 f2 f2 f2 f8 f8 f8 f8 f8 f8 f8 f8 f2 f2 f2 f2 =>0x200fe851f410: 00 00 00 00 00 00 00 00 00 00 00 00 00[f2]f2 f2 0x200fe851f420: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x200fe851f460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==57189==ABORTING Acked-by: Numan Siddique <numans@ovn.org> Signed-off-by: Linhaifeng <haifeng.lin@huawei.com> Signed-off-by: Ben Pfaff <blp@ovn.org>

OVS uses memcmp to compare actions of existing and new flows, but 'struct ofp_ed_prop_nsh_md_type' and corresponding ofpact structure has 3 bytes of padding that never initialized and passed around within OF data structures and messages. Uninitialized bytes in MemcmpInterceptorCommon at offset 21 inside [0x7090000003f8, 136) WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x4a184e in bcmp (vswitchd/ovs-vswitchd+0x4a184e) williamtu#1 0x896c8a in ofpacts_equal lib/ofp-actions.c:9121:31 williamtu#2 0x564403 in replace_rule_finish ofproto/ofproto.c:5650:37 williamtu#3 0x563462 in add_flow_finish ofproto/ofproto.c:5218:13 williamtu#4 0x54a1ff in ofproto_flow_mod_finish ofproto/ofproto.c:8091:17 williamtu#5 0x5433b2 in handle_flow_mod__ ofproto/ofproto.c:6216:17 williamtu#6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17 williamtu#7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16 williamtu#8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21 williamtu#9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13 williamtu#10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9 williamtu#11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5 williamtu#12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9 williamtu#13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5 williamtu#14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9 williamtu#15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6) williamtu#16 0x46b92d in _start (vswitchd/ovs-vswitchd+0x46b92d) Uninitialized value was stored to memory at #0 0x4745aa in __msan_memcpy.part.0 (vswitchd/ovs-vswitchd) williamtu#1 0x54529f in rule_actions_create ofproto/ofproto.c:3134:5 williamtu#2 0x54915e in ofproto_rule_create ofproto/ofproto.c:5284:11 williamtu#3 0x55d419 in add_flow_init ofproto/ofproto.c:5123:17 williamtu#4 0x54841f in ofproto_flow_mod_init ofproto/ofproto.c:7987:17 williamtu#5 0x543250 in handle_flow_mod__ ofproto/ofproto.c:6206:13 williamtu#6 0x56a2fc in handle_flow_mod ofproto/ofproto.c:6190:17 williamtu#7 0x565bda in handle_single_part_openflow ofproto/ofproto.c:8504:16 williamtu#8 0x540b25 in handle_openflow ofproto/ofproto.c:8685:21 williamtu#9 0x6697fd in ofconn_run ofproto/connmgr.c:1329:13 williamtu#10 0x668e6e in connmgr_run ofproto/connmgr.c:356:9 williamtu#11 0x53f1bc in ofproto_run ofproto/ofproto.c:1890:5 williamtu#12 0x4ead0c in bridge_run__ vswitchd/bridge.c:3250:9 williamtu#13 0x4e9bc8 in bridge_run vswitchd/bridge.c:3309:5 williamtu#14 0x51c072 in main vswitchd/ovs-vswitchd.c:127:9 williamtu#15 0x7f23a99011a2 in __libc_start_main (/lib64/libc.so.6) Uninitialized value was created by an allocation of 'ofpacts_stub' in the stack frame of function 'handle_flow_mod' #0 0x569e80 in handle_flow_mod ofproto/ofproto.c:6170 This could cause issues with flow modifications or other operations. To reproduce, some NSH tests could be run under valgrind or clang MemorySantizer. Ex. "nsh - md1 encap over a veth link" test. Fix that by clearing padding bytes while encoding and decoding. OVS will still accept OF messages with non-zero padding from controllers. New tests added to tests/ofp-actions.at. Fixes: 1fc11c5 ("Generic encap and decap support for NSH") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Acked-by: Jan Scheurich <jan.scheurich@ericsson.com>

If datapath flow doesn't have one of the fields of gtpu metadata, e.g. 'tunnel(gtpu())', uninitialized stack memory will be used instead. ==3485429==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x853a1b in format_u8x lib/odp-util.c:3474:13 williamtu#1 0x86ee9c in format_odp_tun_gtpu_opt lib/odp-util.c:3713:5 williamtu#2 0x86a099 in format_odp_tun_attr lib/odp-util.c:3973:13 williamtu#3 0x83afe6 in format_odp_key_attr__ lib/odp-util.c:4179:9 williamtu#4 0x838afb in odp_flow_format lib/odp-util.c:4563:17 williamtu#5 0x738422 in log_flow_message lib/dpif.c:1750:5 williamtu#6 0x738e2f in log_flow_put_message lib/dpif.c:1784:9 williamtu#7 0x7371a4 in dpif_operate lib/dpif.c:1377:21 williamtu#8 0x7363ef in dpif_flow_put lib/dpif.c:1035:5 williamtu#9 0xc7aab7 in dpctl_put_flow lib/dpctl.c:1171:13 williamtu#10 0xc65a4f in dpctl_unixctl_handler lib/dpctl.c:2701:17 williamtu#11 0xaaad04 in process_command lib/unixctl.c:308:13 williamtu#12 0xaa87f7 in run_connection lib/unixctl.c:342:17 williamtu#13 0xaa842e in unixctl_server_run lib/unixctl.c:393:21 williamtu#14 0x51c09c in main vswitchd/ovs-vswitchd.c:128:9 williamtu#15 0x7f88344391a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) williamtu#16 0x46b92d in _start (vswitchd/ovs-vswitchd+0x46b92d) Uninitialized value was stored to memory at #0 0x87da17 in scan_gtpu_metadata lib/odp-util.c:5221:27 williamtu#1 0x874588 in parse_odp_key_mask_attr__ lib/odp-util.c:5862:9 williamtu#2 0x83ee14 in parse_odp_key_mask_attr lib/odp-util.c:5808:18 williamtu#3 0x83e8b5 in odp_flow_from_string lib/odp-util.c:6065:18 williamtu#4 0xc7a4f3 in dpctl_put_flow lib/dpctl.c:1145:13 williamtu#5 0xc65a4f in dpctl_unixctl_handler lib/dpctl.c:2701:17 williamtu#6 0xaaad04 in process_command lib/unixctl.c:308:13 williamtu#7 0xaa87f7 in run_connection lib/unixctl.c:342:17 williamtu#8 0xaa842e in unixctl_server_run lib/unixctl.c:393:21 williamtu#9 0x51c09c in main vswitchd/ovs-vswitchd.c:128:9 williamtu#10 0x7f88344391a2 in __libc_start_main (/lib64/libc.so.6+0x271a2) Uninitialized value was created by an allocation of 'msgtype_ma' in the stack frame of function 'scan_gtpu_metadata' #0 0x87d440 in scan_gtpu_metadata lib/odp-util.c:5187 Fix that by initializing fields to all zeroes by default. Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21426 Fixes: 3c6d05a ("userspace: Add GTP-U support.") Acked-by: Yi Yang <yangyi01@inspur.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 williamtu#1 0x5f0551 in ofpact_decode ./lib/ofp-actions.inc2:4777:16 williamtu#2 0x5ed17c in ofpacts_decode lib/ofp-actions.c:7752:21 williamtu#3 0x5eba9a in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7791:13 williamtu#4 0x5eb9fc in ofpacts_pull_openflow_actions lib/ofp-actions.c:7835:12 williamtu#5 0x64bb8b in ofputil_decode_packet_out lib/ofp-packet.c:1113:17 williamtu#6 0x65b6f4 in ofp_print_packet_out lib/ofp-print.c:148:13 williamtu#7 0x659e3f in ofp_to_string__ lib/ofp-print.c:1029:16 williamtu#8 0x659b24 in ofp_to_string lib/ofp-print.c:1244:21 williamtu#9 0x65a28c in ofp_print lib/ofp-print.c:1288:28 williamtu#10 0x540d11 in ofctl_ofp_parse utilities/ovs-ofctl.c:2814:9 williamtu#11 0x564228 in ovs_cmdl_run_command__ lib/command-line.c:247:17 williamtu#12 0x56408a in ovs_cmdl_run_command lib/command-line.c:278:5 williamtu#13 0x5391ae in main utilities/ovs-ofctl.c:179:9 williamtu#14 0x7f6911ce9081 in __libc_start_main (/lib64/libc.so.6+0x27081) williamtu#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: f839892 ("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>

ovsdb_cs_send_transaction() returns the pointer to the same 'request_id' object that is used internally. This leads to situation where transaction in idl and CS module has the same 'request_id' object. However, CS module is able to destroy this transaction id at any time, e.g. if connection state chnaged, but idl transaction might be still around at this moment and application might still use it. Found by running 'make check-ovsdb-cluster' with AddressSanitizer: ==79922==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000167a98 at pc 0x000000626acf bp 0x7ffcdb38a4c0 sp 0x7ffcdb38a4b8 READ of size 8 at 0x604000167a98 thread T0 #0 0x626ace in json_destroy lib/json.c:354:18 williamtu#1 0x56d1ab in ovsdb_idl_txn_destroy lib/ovsdb-idl.c:2528:5 williamtu#2 0x53a908 in do_vsctl utilities/ovs-vsctl.c:3008:5 williamtu#3 0x539251 in main utilities/ovs-vsctl.c:203:17 williamtu#4 0x7f7f7e376081 in __libc_start_main (/lib64/libc.so.6+0x27081) williamtu#5 0x461fed in _start (utilities/ovs-vsctl+0x461fed) 0x604000167a98 is located 8 bytes inside of 40-byte region [0x604000167a90,0x604000167ab8) freed by thread T0 here: #0 0x503ac7 in free (utilities/ovs-vsctl+0x503ac7) williamtu#1 0x626aae in json_destroy lib/json.c:378:9 williamtu#2 0x6adfa2 in ovsdb_cs_run lib/ovsdb-cs.c:625:13 williamtu#3 0x567731 in ovsdb_idl_run lib/ovsdb-idl.c:394:5 williamtu#4 0x56fed1 in ovsdb_idl_txn_commit_block lib/ovsdb-idl.c:3187:9 williamtu#5 0x53a4df in do_vsctl utilities/ovs-vsctl.c:2898:14 williamtu#6 0x539251 in main utilities/ovs-vsctl.c:203:17 williamtu#7 0x7f7f7e376081 in __libc_start_main previously allocated by thread T0 here: #0 0x503dcf in malloc (utilities/ovs-vsctl+0x503dcf) williamtu#1 0x594656 in xmalloc lib/util.c:138:15 williamtu#2 0x626431 in json_create lib/json.c:1451:25 williamtu#3 0x626972 in json_integer_create lib/json.c:263:25 williamtu#4 0x62da0f in jsonrpc_create_id lib/jsonrpc.c:563:12 williamtu#5 0x62d9a8 in jsonrpc_create_request lib/jsonrpc.c:570:23 williamtu#6 0x6af3a6 in ovsdb_cs_send_transaction lib/ovsdb-cs.c:1357:35 williamtu#7 0x56e3d5 in ovsdb_idl_txn_commit lib/ovsdb-idl.c:3147:27 williamtu#8 0x56fea9 in ovsdb_idl_txn_commit_block lib/ovsdb-idl.c:3186:22 williamtu#9 0x53a4df in do_vsctl utilities/ovs-vsctl.c:2898:14 williamtu#10 0x539251 in main utilities/ovs-vsctl.c:203:17 williamtu#11 0x7f7f7e376081 in __libc_start_main Fixes: 1c337c4 ("ovsdb-idl: Break into two layers.") Acked-by: Dumitru Ceara <dceara@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

williamtu added 2 commits July 19, 2018 13:55

Merge branch 'fix-flow-dump' of https://github.com/YiHungWei/ovs-ebpf …

824c171

…into rewrite-parser

williamtu requested review from joestringer, yifsun and YiHungWei August 10, 2018 17:47

williamtu closed this Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor parser and enable XDP parsing #11

refactor parser and enable XDP parsing #11

williamtu commented Aug 10, 2018

refactor parser and enable XDP parsing #11

refactor parser and enable XDP parsing #11

Conversation

williamtu commented Aug 10, 2018