Skip to content
GitLab
Switch branch/tag
  1. 28 Jun, 2022 9 commits
  3. 27 Jun, 2022 8 commits
  5. 22 Jun, 2022 2 commits
  7. 15 Jun, 2022 1 commit
  9. 07 Jun, 2022 3 commits
    • Frode Nordahl's avatar
      ofproto-dpif-xlate: Fix internal CT state for non-recirc traffic. · 250e1a6d
      Frode Nordahl authored 
      In some circumstances a flow may get its ct_state set without
conscious intervention by the OVS user space code.

Commit 355fef6f optimizes out unnecessary ct_clear actions
based on an internal struct xlate_ctx->conntracked state flag.

Before this commit the xlate_ctx->conntracked state flag would
be initialized to 'false' and only set during thawing for
recirculation.

This patch checks the flow ct_state for the non-recirc case and
sets the internal conntracked state appropriately.  A system
traffic test is also added to avoid regression.

Fixes: 355fef6f

 ("ofproto-dpif-xlate: Avoid successive ct_clear datapath actions.")
Signed-off-by: Frode Nordahl's avatarFrode Nordahl <frode.nordahl@canonical.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      250e1a6d
    • Aaron Conole's avatar
      classifier: Adjust segment boundary to execute prerequisite processing. · fe870ee0
      Aaron Conole authored 
      During flow processing, the flow wildcards are checked as a series of
stages, and these stages are intended to carry dependencies in a single
direction.  But when the neighbor discovery processing, for example, was
executed there is an incorrect dependency chain - we need fields from
stage 4 to determine whether we need fields from stage 3.

We can build a set of flow rules to demonstrate this:
  table=0,priority=100,ipv6,ipv6_src=1000::/10 actions=resubmit(,1)
  table=0,priority=0 actions=NORMAL
  table=1,priority=110,ipv6,ipv6_dst=1000::3 actions=resubmit(,2)
  table=1,priority=100,ipv6,ipv6_dst=1000::4 actions=resubmit(,2)
  table=1,priority=0 actions=NORMAL
  table=2,priority=120,icmp6,nw_ttl=255,icmp_type=135,icmp_code=0,nd_sll=10:de:ad:be:ef:10 actions=NORMAL
  table=2,priority=100,tcp actions=NORMAL
  table=2,priority=100,icmp6 actions=NORMAL
  table=2,priority=0 actions=NORMAL

With this set of flows, any IPv6 packet that executes through this pipeline
will have the corresponding nd_sll field flagged as required match for
classification even if that field doesn't make sense in such a context
(for example, TCP packets).  When the corresponding flow is installed into
the kernel datapath, this field is not reflected when the revalidator
executes the dump stage (see net/openvswitch/flow_netlink.c for more details).

During the sweep stage, revalidator will compare the dumped WC with a
generated WC - these will mismatch because the generated WC will match on
the Neighbor Discovery fields, while the datapath WC will not match on
these fields.  We will then invalidate the flow and as a side effect
force an upcall.

By redefining the boundary, we shift these fields to the l4 subtable, and
cause masks to be generated matching just the requisite fields.  The list
of fields being shifted:

    struct in6_addr nd_target;
    struct eth_addr arp_sha;
    struct eth_addr arp_tha;
    ovs_be16 tcp_flags;
    ovs_be16 pad2;
    struct ovs_key_nsh nsh;

A standout field would be tcp_flags moving from l3 subtable matches to
the l4 subtable matches.  This reverts a partial performance optimization
in the case of stateless firewalling.  The tcp_flags field might have
been a good candidate to retain in the l3 segment, but it got overloaded
with ICMPv6 ND matching, and therefore we can't preserve this kind of
optimization.

Two other approaches were considered - moving the nd_target field alone
and collapsing the l3/l4 segments into a single subtable for matching.
Moving any field individually introduces ABI mismatch, and doesn't
completely address the problems with other neighbor discovery related
fields (such as nd_sll/nd_tll).  Collapsing the two subtables creates
an issue with datapath flow explosion, since the l3 and l4 fields will
be unwildcarded together (this can be seen with some of the existing
classifier tests).

A simple test is added to showcase the behavior.

Fixes: 476f36e8 ("Classifier: Staged subtable matching.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2081773


Reported-by: default avatarNuman Siddique <nusiddiq@redhat.com>
Suggested-by: default avatarIlya Maximets <i.maximets@ovn.org>
Signed-off-by: default avatarAaron Conole <aconole@redhat.com>
Acked-by: default avatarEelco Chaudron <echaudro@redhat.com>
Acked-by: default avatarCian Ferriter <cian.ferriter@intel.com>
Tested-by: default avatarNuman Siddique <numans@ovn.org>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      fe870ee0
    • Han Ding's avatar
      ovs-tcpdump: Fix error when stopping ovs-tcpdump. · ec0ec464
      Han Ding authored 
      

Sometimes we need to dump packets on more than two interfaces in a bridge
at the same time. Then when we stop dumping in order, ovs-tcpdump print
traceback and fail to delete mirror interface for some interface.

For example:
br-int has two interface tap1 and br-int. We use ovs-tcpdump dump tap1 first
and dump br-int next. Then stopping tap1 ovs-tcpdump first, and stopping
br-int second. When we stop ovs-tcpdump for br-int, the screen show the error
like this:
__main__.OVSDBException: Unable to delete Mirror m_br-int

Signed-off-by: default avatarHan Ding <handing@chinatelecom.cn>
Acked-by: default avatarMike Pattrick <mkp@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      ec0ec464
  11. 30 May, 2022 6 commits
  13. 25 May, 2022 7 commits
    • Ilya Maximets's avatar
      ovsdb: raft: Fix transaction double commit due to lost leadership. · e07377bb
      Ilya Maximets authored 
      While becoming a follower, the leader aborts all the current
'in-flight' commands, so the higher layers can re-try corresponding
transactions when the new leader is elected.  However, most of these
commands are already sent to followers as append requests, hence they
will actually be committed by the majority of the cluster members,
i.e. will be treated as committed by the new leader, unless there is
an actual network problem between servers.  However, the old leader
will decline append replies, since it's not the leader anymore and
commands are already completed with RAFT_CMD_LOST_LEADERSHIP status.

New leader will replicate the commit index back to the old leader.
Old leader will re-try the previously "failed" transaction, because
"cluster error"s are temporary.

If a transaction had some prerequisites that didn't allow double
committing or there are other database constraints (like indexes) that
will not allow a transaction to be committed twice, the server will
reply to the client with a false-negative transaction result.

If there are no prerequisites or additional database constraints,
the server will execute the same transaction again, but as a follower.

E.g. in the OVN case, this may result in creation of duplicated logical
switches / routers / load balancers.  I.e. resources with the same
non-indexed name.  That may cause issues later where ovn-nbctl will
not be able to add ports to these switches / routers.

Suggested solution is to not complete (abort) the commands, but allow
them to be completed with the commit index update from a new leader.
It the similar behavior to what we do in order to complete commands
in a backward scenario when the follower becomes a leader.  That
scenario was fixed by commit 5a9b53a5 ("ovsdb raft: Fix duplicated
transaction execution when leader failover.").

Code paths for leader and follower inside the raft_update_commit_index
were very similar, so they were refactored into one, since we also
needed an ability to complete more than one command for a follower.

Failure test added to exercise scenario of a leadership transfer.

Fixes: 1b1d2e6d ("ovsdb: Introduce experimental support for clustered databases.")
Fixes: 3c2d6274 ("raft: Transfer leadership before creating snapshots.")
Reported-at: https://bugzilla.redhat.com/2046340


Acked-by: default avatarHan Zhou <hzhou@ovn.org>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      e07377bb
    • Dumitru Ceara's avatar
      dynamic-string: Fix undefined behavior due to offsetting null pointer. · 5da86cb3
      Dumitru Ceara authored 
      

When compiled with clang and '-fsanitize=undefined' set, running
'ovsdb-client --timestamp monitor Open_vSwitch' in a sandbox triggers
the following undefined behavior (flagged by UBSan):

  lib/dynamic-string.c:207:38: runtime error: applying zero offset to null pointer
      #0 0x4ebc18 in ds_put_strftime_msec lib/dynamic-string.c:207:38
      #1 0x4ebd04 in xastrftime_msec lib/dynamic-string.c:225:5
      #2 0x552e6a in table_format_timestamp__ lib/table.c:226:12
      #3 0x552852 in table_print_timestamp__ lib/table.c:233:27
      #4 0x5506f3 in table_print_table__ lib/table.c:254:5
      #5 0x550633 in table_format lib/table.c:601:9
      #6 0x5524f3 in table_print lib/table.c:633:5
      #7 0x44dc5e in monitor_print_table ovsdb/ovsdb-client.c:1019:5
      #8 0x44c650 in monitor_print ovsdb/ovsdb-client.c:1040:13
      #9 0x44ac56 in do_monitor__ ovsdb/ovsdb-client.c:1500:21
      #10 0x44636e in do_monitor ovsdb/ovsdb-client.c:1575:5
      #11 0x442c41 in main ovsdb/ovsdb-client.c:283:5

Reported-by: default avatarIlya Maximets <i.maximets@ovn.org>
Signed-off-by: default avatarDumitru Ceara <dceara@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      5da86cb3
    • Aaron Conole's avatar
      Revert "odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP." · 369e6889
      Aaron Conole authored 
      This reverts commit c645550b ("odp-util: Always report
ODP_FIT_TOO_LITTLE for IGMP.")

Always forcing a slow path action can result in some over-broad
flows which swallow all traffic and force them to userspace, as reported
in the thread at
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387706.html
and at
https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/387689.html

Revert the ODP_FIT_TOO_LITTLE return for IGMP specifically.
Additionally, remove the userspace wc mask to prevent revalidator from
cycling flows.

Fixes: c645550b

 ("odp-util: Always report ODP_FIT_TOO_LITTLE for IGMP.")
Signed-off-by: default avatarAaron Conole <aconole@redhat.com>
Acked-by: default avatarEelco Chaudron <echaudro@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      369e6889
    • wenxu's avatar
      ofproto-dpif-xlate: Fix netdev native tunnel neigh discovery spa. · 18341166
      wenxu authored 
      

During native tunnel encapsulation process, on tunnel neighbor cache
miss OVS sends an arp/nd request.  Currently, tunnel source is used
as arp spa.

Find the spa which has the same subnet with the nexthop of tunnel dst
on egress port, if false, use the tunnel src as spa.

For example:
tunnel src is a vip with 10.0.0.7/32, tunnel dst is 10.0.1.7
the br-phy with address 192.168.0.7/24 and the default gateway is 192.168.0.1
So the spa of arp request for 192.168.0.1 should be 192.168.0.7 but not 10.0.0.7

Signed-off-by: default avatarwenxu <wenxu@chinatelecom.cn>
Acked-by: default avatarEelco Chaudron <echaudro@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      18341166
    • wenxu's avatar
      ovs-router: Expose the ovs_router_get_netdev_source_address function. · 748e4b2b
      wenxu authored 
      

Rename get_src_addr to ovs_router_get_netdev_source_address and expose
this function to be used in the next commit.

Signed-off-by: default avatarwenxu <wenxu@chinatelecom.cn>
Acked-by: default avatarEelco Chaudron <echaudro@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      748e4b2b
    • lic121's avatar
      ofproto-dpif: Trigger revalidation if ct tp changes. · 34390bb3
      lic121 authored 
      Once ct_zone timeout policy changes, revalidator is supposed to be
triggered.

Fixes: 993cae67

 ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables")
Signed-off-by: default avatarlic121 <lic121@chinatelecom.cn>
Acked-by: default avatarEelco Chaudron <echaudro@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      34390bb3
    • hxie's avatar
      Carefully release NBL in Windows · 1adb07e2
      hxie authored 
      OvsExtSendNBLComplete always release NBLs with flag SINGL_SOURCE, this is an
efficient way, which requires all the NBLs having the same source port, when
cloned/fragment NBLs are released, the parent NBLs will be released as well,
the problem is that a parent NBL may have a different source port from the
cloned/fragment NBL, so releasing the parent NBLs with flag SINGLE_SOURCE
is not corrct, see:
    https://github.com/microsoft/hcsshim/issues/1056



When this happens, commands 'Get-NetAdapter' and 'Get-HnsEndpoint' in the
Windows node show that one net-adapter/hns-endpoint is in 'disconnected'
state, meanwhile, following processes are affected, ecah of them has one
thread pending on a lock:
  vmcompute.exe
  containerd.exe
  antrea-agent.exe

To fix this issue, we may check SourcePortId in each parent NBLs before
released.

A simple way to reprodue this issue:
   1, Enable encap mode
   2, create 2 nodes, nodeA and nodeB
   3, create podA with image k8s.gcr.io/e2e-test-images/agnhost:2.21 on
      nodeA, run 'iperf/iperf.exe -s -p 9000 -D'
   4, create podB with same image on nodeB, run command
      'iperf/iperf.exe -c <podA-ip> -p 9000'
   5, delete podB
   6, run 'Get-NetAdapter' on nodeB, you will find the leaked net adapter.

Signed-off-by: default avatarHongsheng Xie <hxie@vmware.com>
Signed-off-by: default avatarAlin-Gabriel Serdean <aserdean@ovn.org>
      1adb07e2
  15. 17 May, 2022 4 commits
    • Rosemarie O'Riorden's avatar
      tests: Properly kill ovsdb test processes. · 1ccaba44
      Rosemarie O'Riorden authored 
      The FreeBSD CI builds keep failing because processes of tests are not
properly killed. This leaves the build hanging until it times out, and
ultimately fails.

Changes name of pidfile pid2 to 2.pid so that the
on_exit 'kill `cat *.pid`' will capture all pidfiles.

Fixes pidfile name logic in test that uses OVSDB_SERVER_SHUTDOWN_N, so
that all pidfile names match the form *.pid.

Replaces unnecessary --pidfile="`pwd`"/pid with just --pidfile, because
by default this argument creates a pidfile named <proc-name>.pid.

Removes extra [test ! -e pid || kill `cat pid`] that run upon AT_CHECK
failure, because those processes are killed with on_exit. Also adds
on_exit in tests where it was missing.

Fixes: 56120500 ("tests: Get rid of overly specific --pidfile and --unixctl options.")
Fixes: 0be15ad7 ("ovsdb-server.at: Add unit test for record/replay.")
Fixes: 7964ffe7 ("ovsdb: relay: Add support for transaction forwarding.")
Fixes: e879d33e ("ovsdb/jsonrpc-server: ovsdb-server closes accepted connections immediately.")
Reported-at: https://github.com/cirruslabs/cirrus-ci-docs/issues/910


Signed-off-by: default avatarRosemarie O'Riorden <roriorden@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      1ccaba44
    • Han Ding's avatar
      ovs-save: Get highest ofp version error. · 260b091c
      Han Ding authored 
      

When setting just one ofp version to protocols of bridge, The function
get_highest_ofp_version in ovs-save parse it error.

For example:
$ ovs-vsctl get bridge br-int protocols
[OpenFlow15]

$ ovs-vsctl get bridge br-int protocols |
  sed 's/[][]//g' | sed 's/\ //g' | awk -F ',' '{ print (NF>1)? $(NF) : "OpenFlow14" }'
OpenFlow14

Signed-off-by: default avatarHan Ding <handing@chinatelecom.cn>
Acked-by: default avatarAdrian Moreno <amorenoz@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      260b091c
    • Dumitru Ceara's avatar
      netdev-linux: Properly access 32-bit aligned rtnl_link_stats64 structs. · 7606bb12
      Dumitru Ceara authored 
      

Detected by UB Sanitizer when running system tests:

  lib/netdev-linux.c:6250:26:
  runtime error: member access within misaligned address 0x00000229a204
  for type 'const struct rpl_rtnl_link_stats64', which requires 8 byte
  alignment
  0x00000229a204: note: pointer points here
    c4 00 17 00 01 00 00 00  00 00 00 00 01 00 00 00
                ^
    00 00 00 00 6e 00 00 00  00 00 00 00 6e 00 00 00

  0  0x89f10e in netdev_stats_from_rtnl_link_stats64 lib/netdev-linux.c:6250
  1  0x89f10e in get_stats_via_netlink lib/netdev-linux.c:6298
  2  0x8a039a in netdev_linux_get_stats lib/netdev-linux.c:2227
  3  0x68e149 in netdev_get_stats lib/netdev.c:1599
  4  0x407b21 in iface_refresh_stats vswitchd/bridge.c:2687
  5  0x419eb6 in iface_create vswitchd/bridge.c:2134
  6  0x419eb6 in bridge_add_ports__ vswitchd/bridge.c:1170
  7  0x41f71c in bridge_add_ports vswitchd/bridge.c:1181
  8  0x41f71c in bridge_reconfigure vswitchd/bridge.c:898
  9  0x429f59 in bridge_run vswitchd/bridge.c:3331
  10 0x430af3 in main vswitchd/ovs-vswitchd.c:129
  11 0x7fbdfd43eb74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
  12 0x4072fd in _start (/root/ovs/vswitchd/ovs-vswitchd+0x4072fd)

Signed-off-by: default avatarDumitru Ceara <dceara@redhat.com>
Acked-by: default avatarAaron Conole <aconole@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      7606bb12
    • Dumitru Ceara's avatar
      treewide: Avoid offsetting NULL pointers. · 0688b9f2
      Dumitru Ceara authored 
      

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: default avatarDumitru Ceara <dceara@redhat.com>
Acked-by: default avatarAaron Conole <aconole@redhat.com>
Signed-off-by: default avatarIlya Maximets <i.maximets@ovn.org>
      0688b9f2