[ovs-dev] [PATCH v3] ovs-tcpdump: Stdout is shutdown before ovs-tcpdump exit

[Songtao Zhan]

To: d...@openvswitch.org,
i.maxim...@ovn.org

If there is a pipe behind ovs-tcpdump(such as ovs-tcpdump -i eth0
| grep "192.168.1.1"), the child process (grep "192.168.1.1") may
exit first and close the pipe when received SIGTERM. When farther
process(ovs-tcpdump) exit, stdout is flushed into broken pipe, and
then received a exception IOError. To avoid such problems, ovs-tcp
dump first close stdout before exit.

Signed-off-by: Songtao Zhan 
---
 utilities/ovs-tcpdump.in | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index a49ec9f94..270207d01 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -538,6 +538,17 @@ def main():
 print(data.decode('utf-8'))
 raise KeyboardInterrupt
 except KeyboardInterrupt:
+# If there is a pipe behind ovs-tcpdump (such as ovs-tcpdump
+# -i eth0 | grep "192.168.1.1"), the pipe is no longer available
+# after received ctrl+c.
+# If we write data to an unavailable pipe, a pipe error will be
+# reported, so we turn off stdout to avoid subsequence flushing
+# of data into the pipe.
+try:
+sys.stdout.close()
+except IOError:
+pass
+
 if pipes.poll() is None:
 pipes.terminate()

-- 
2.39.1



zhan...@chinatelecom.cn
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] learning-switch: Fix coredump of OpenFlow15 learning-switch

[Ilya Maximets]

On 3/30/23 11:13, Faicker Mo wrote:
> The OpenFlow15 Packet-Out message contains the match instead of the in_port.
> The flow.tunnel.metadata.tab is not inited but used in the loop of 
> tun_metadata_to_nx_match.
> 
> The coredump gdb backtrace is:
>  #0  memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, 
> src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467
>  #1  0x004506e8 in metadata_loc_from_match_read 
> (match=0x7ffcfac30598, is_masked=, mask=0x7ffcfac30838, 
> idx=0, map=0x0) at lib/tun-metadata.c:865
>  #2  metadata_loc_from_match_read (is_masked=, 
> mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at 
> lib/tun-metadata.c:854
>  #3  tun_metadata_to_nx_match (b=b@entry=0x892260, 
> oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598) at 
> lib/tun-metadata.c:888
>  #4  0x0047c1f8 in nx_put_raw (b=b@entry=0x892260, 
> oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, 
> cookie=, cookie@entry=0,
>  cookie_mask=, cookie_mask@entry=0) at lib/nx-match.c:1186
>  #5  0x0047d693 in oxm_put_match (b=b@entry=0x892260, 
> match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at 
> lib/nx-match.c:1343
>  #6  0x0043194e in ofputil_encode_packet_out 
> (po=po@entry=0x7ffcfac30580, protocol=) at 
> lib/ofp-packet.c:1226
>  #7  0x0040a4fe in process_packet_in (sw=sw@entry=0x891d70, 
> oh=) at lib/learning-switch.c:619
>  #8  0x0040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) 
> at lib/learning-switch.c:374
>  #9  lswitch_run (sw=0x891d70) at lib/learning-switch.c:324
>  #10 0x00406f26 in main (argc=, argv=) 
> at utilities/ovs-testcontroller.c:180
> 
> Fix that by zeroing out the po variable.
> 
> Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support")

I think, the following commit is more appropriate:

Fixes: 35eb6326d5d0 ("ofp-util: Add flow metadata to ofputil_packet_out")

> Signed-off-by: Faicker Mo 
> ---
> v2:
> - changed the init and add test case
> ---
>  lib/learning-switch.c   |  2 +-
>  tests/system-traffic.at | 21 +
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/learning-switch.c b/lib/learning-switch.c
> index 8102475ca..275c30e26 100644
> --- a/lib/learning-switch.c
> +++ b/lib/learning-switch.c
> @@ -524,7 +524,7 @@ process_packet_in(struct lswitch *sw, const struct 
> ofp_header *oh)
>  uint64_t ofpacts_stub[64 / 8];
>  struct ofpbuf ofpacts;
>  
> -struct ofputil_packet_out po;
> +struct ofputil_packet_out po = {0};

It might be better to explicitly call match_init_catchall(_metadata)
instead.  In case the 'match' structure will need a special initialization
logic in the future.

>  enum ofperr error;
>  
>  struct dp_packet pkt;
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 2558f3b24..7de26840c 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -7845,3 +7845,24 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: * 
> * *5002 *2000 *b85e *00
>  
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_BANNER([learning-switch])
> +AT_SETUP([learning-switch - OpenFlow15])
> +dnl Start ovs-testcontroller
> +AT_CHECK([ovs-testcontroller --no-chdir --detach punix:controller --pidfile 
> -v ptcp:], [0], [ignore])
> +OVS_WAIT_UNTIL([test -e controller])
> +dnl Setup ovs
> +OVS_TRAFFIC_VSWITCHD_START([-- set bridge br0 protocols=OpenFlow15 -- 
> set-controller br0 tcp:127.0.0.1:6653])
> +
> +ADD_NAMESPACES(at_ns0,  at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING], 
> [0], [dnl
> +10 packets transmitted, 10 received, 0% packet loss, time 0ms
> +])
> +kill `cat ovs-testcontroller.pid`
> +OVS_WAIT_UNTIL([! test -e controller])
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP

Thanks for adding a test.  But we don't really need a system test in order
to test this part of the code.  The same can be expressed as a test in a
common testsuite with dummy ports.  'netdev-dummy/receive' appctl commands
can be used to inject packets.  This way the test will be running in our CI
automatically on every patch.

I'm not sure though in which file the new test should live, so feel free
to create a new one like tests/testcontroller.at.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovs-tcpdump: Stdout is shutdown before ovs-tcpdump exit

[Ilya Maximets]

On 4/1/23 05:52, Songtao Zhan wrote:
> To: d...@openvswitch.org,
> i.maxim...@ovn.org
> 
> If there is a pipe behind ovs-tcpdump(such as ovs-tcpdump -i eth0
> | grep "192.168.1.1"), the child process (grep "192.168.1.1") may
> exit first and close the pipe when received SIGTERM. When farther
> process(ovs-tcpdump) exit, stdout is flushed into broken pipe, and
> then received a exception IOError. To avoid such problems, ovs-tcp
> dump first close stdout before exit.
> 
> Signed-off-by: Songtao Zhan 
> ---
>  utilities/ovs-tcpdump.in | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
> index a49ec9f94..b491647a5 100755
> --- a/utilities/ovs-tcpdump.in
> +++ b/utilities/ovs-tcpdump.in
> @@ -22,6 +22,7 @@ import sys
>  import time
>  import struct
>  import fcntl
> +import signal

Hi.  This include added, but not used.  That breaks the flake8 check
in the GitHub CI:

  utilities/ovs-tcpdump.in:25:1: F401 'signal' imported but unused

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] conntrack-tp: Fix clang warning.

[Ilya Maximets]

On 3/31/23 05:16, mit...@outlook.com wrote:
> From: Lin Huang 
> 
> Declaration of 'struct conn' will not be visible outside of this function.
> Declaration of 'struct conntrack' will not be visible outside of this 
> function.
> Declaration of 'struct timeout_policy' will not be visible outside of this 
> function.
> ---
>  lib/conntrack-tp.h | 7 +++
>  1 file changed, 7 insertions(+)

Hi.  Looks like you missed a sign-off tag in this version.
I can apply the patch if you reply here with the tag.  Or
you may send a v3 with a correct tag instead.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10] netdev-offload-tc: del ufid mapping if device not exist.

[Ilya Maximets]

On 4/3/23 13:00, Simon Horman wrote:
> On Fri, Mar 31, 2023 at 03:22:44PM +0200, Eelco Chaudron wrote:
>>
>>
>> On 30 Mar 2023, at 11:27, Faicker Mo wrote:
>>
>>> The device may be deleted and added with ifindex changed.
>>> The tc rules on the device will be deleted if the device is deleted.
>>> The func tc_del_filter will fail when flow del. The mapping of
>>> ufid to tc will not be deleted.
>>> The traffic will trigger the same flow(with same ufid) to put to tc
>>> on the new device. Duplicated ufid mapping will be added.
>>> If the hashmap is expanded, the old mapping entry will be the first entry,
>>> and now the dp flow can't be deleted.
>>>
>>> Signed-off-by: Faicker Mo 
>>
>> Changes look good to me, so if Simon’s tests pass over the weekend:
>>
>> Acked-by: Eelco Chaudron 
> 
> The weekend came and the weekend went.
> I checked just now and the loop has run the test 73014 times
> without recording any failures.
> 
> I think we are good here :)
> 
> Reviewed-by: Simon Horman 
> Tested-by: Simon Horman 

Thanks!  Applied.  Backported down to 2.17.

Best regards, Ilya Maximets.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] net: openvswitch: fix race on port output

[Jakub Kicinski]

On Mon, 3 Apr 2023 11:18:46 + Felix Hüttner wrote:
> On Sat, 1 Apr 2023 6:25:00 + Jakub Kicinski wrote:
> > On Fri, 31 Mar 2023 06:25:13 + Felix Hüttner wrote:  
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 253584777101..6628323b7bea 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -3199,6 +3199,7 @@ static u16 skb_tx_hash(const struct net_device *dev,
> > > }
> > >
> > > if (skb_rx_queue_recorded(skb)) {
> > > +   BUG_ON(unlikely(qcount == 0));  
> >
> > DEBUG_NET_WARN_ON()
> >  
> 
> However if this condition triggers we will be permanently stuck in the loop 
> below.
> From my understading this also means that future calls to `synchronize_net` 
> will never finish (as the packet never finishes processing).
> So the user will quite probably need to restart his system.
> I find DEBUG_NET_WARN_ON_ONCE to offer too little visiblity as 
> CONFIG_DEBUG_NET is not necessarily enabled per default.
> I as the user would see it as helpful to have this information available 
> without additional config flags.
> I would propose to use WARN_ON_ONCE

skb_tx_hash() may get called a lot, we shouldn't slow it down on
production systems just to catch buggy drivers, IMO.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6] lib, ovsdb, ovs-vsctl, vtep-ctl: Fix multiple Coverity defects

[Aaron Conole]

James Raphael Tiovalen  writes:

> This commit addresses several high and medium-impact Coverity defects by
> fixing several possible null-pointer dereferences and potentially
> uninitialized variables.
>
> There were cases when crashes were encountered when some null pointers
> were dereferenced. Null pointer checks and alternative code paths have
> been added to prevent unwanted crashes. For impossible conditions,
> non-null assertions are added. Additionally, some struct variables were
> not initialized. Zero-initializations have been added to prevent
> potential data leaks or undefined behavior.
>
> Some variables would always be initialized by either the code flow or
> the compiler and some pointers would never be null. Thus, some Coverity
> reports might be false positives. That said, it is still considered best
> practice to perform null pointer checks and initialize variables upfront
> just in case to ensure the overall resilience and security of OVS, as
> long as they do not impact performance-critical code. As a bonus, it
> would also make static analyzer tools, such as Coverity, happy.
>
> Unit tests have been successfully executed via `make check` and they
> successfully passed.
>
> Signed-off-by: James Raphael Tiovalen 
> ---

Sorry to comment on this so late - one thing I would also say can help
with review is to separate the logical changes.  For example, each flag
from Coverity could be a separate patch in a cleanup series.  It could
help to keep things organized.  Comments to follow.

> v6:
> - Convert some explicit null pointer checks to assertions since they are
> checking for impossible conditions.
> - Use `nullable_memset()` and `nullable_memcpy()` instead of manually
> performing null checks for `memset()` and `memcpy()`.
>
> v5:
> Improve commit message to better describe the intent of this patch.
>
> v4:
> - Fix some apply-robot checkpatch errors and warnings.
> - Fix github-robot build failure: linux clang test asan.
>
> v3:
> Fix some apply-robot checkpatch errors and warnings.
>
> v2:
> Fix some apply-robot checkpatch errors and warnings.
> ---
>  lib/dp-packet.c | 22 ---
>  lib/dpctl.c | 16 +-
>  lib/json.c  | 22 ---
>  lib/latch-unix.c|  2 +-
>  lib/memory.c| 12 ++-
>  lib/netdev-native-tnl.c | 17 +--
>  lib/odp-execute.c   |  4 
>  lib/pcap-file.c |  4 +++-
>  lib/rtnetlink.c |  5 +
>  lib/sflow_agent.c   |  6 ++
>  lib/shash.c |  5 -
>  lib/sset.c  |  2 ++
>  ovsdb/condition.c   | 10 -
>  ovsdb/file.c| 21 ++
>  ovsdb/jsonrpc-server.c  |  6 +-
>  ovsdb/monitor.c | 36 +++
>  ovsdb/ovsdb-client.c| 46 +++-
>  ovsdb/ovsdb-server.c| 17 +--
>  ovsdb/ovsdb-util.c  |  9 
>  ovsdb/ovsdb.c   |  8 ++-
>  ovsdb/query.c   |  2 ++
>  ovsdb/replication.c | 15 +++--
>  ovsdb/row.c |  4 +++-
>  ovsdb/table.c   |  2 +-
>  ovsdb/transaction.c |  2 ++
>  utilities/ovs-vsctl.c   | 47 -
>  vtep/vtep-ctl.c | 10 ++---
>  27 files changed, 259 insertions(+), 93 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index ae8ab5800..f9c58a72f 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -171,6 +171,7 @@ dp_packet_new_with_headroom(size_t size, size_t headroom)
>  struct dp_packet *
>  dp_packet_clone(const struct dp_packet *buffer)
>  {
> +ovs_assert(buffer != NULL);
>  return dp_packet_clone_with_headroom(buffer, 0);
>  }
>  
> @@ -183,9 +184,12 @@ dp_packet_clone_with_headroom(const struct dp_packet 
> *buffer, size_t headroom)
>  struct dp_packet *new_buffer;
>  uint32_t mark;
>  
> -new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
> - dp_packet_size(buffer),
> - headroom);
> +const void *data_dp = dp_packet_data(buffer);
> +ovs_assert(data_dp != NULL);

There's a bit of intermingling of null checks here.  Please stick to
one style.  Throughout the dp-packet.c file, there are positive tests
(ie 'ovs_assert(data_dp)' would match the style more).

> +
> +new_buffer = dp_packet_clone_data_with_headroom(data_dp,
> +dp_packet_size(buffer),
> +headroom);
>  /* Copy the following fields into the returned buffer: l2_pad_size,
>   * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>  memcpy(_buffer->l2_pad_size, >l2_pad_size,
> @@ -323,7 +327,9 @@ dp_packet_shift(struct dp_packet *b, int delta)
>  
>  if (delta != 0) {
>  char *dst = (char *) dp_packet_data(b) + delta;
> - 

Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack

[Numan Siddique]

On Mon, Apr 3, 2023 at 5:55 AM Dumitru Ceara  wrote:
>
> On 4/3/23 11:47, Abhiram Sangana wrote:
> >
> >
> >> On 29 Mar 2023, at 16:21, Dumitru Ceara  wrote:
> >>
> >> On 3/29/23 12:00, Abhiram Sangana wrote:
> >> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key,
> >>pb->header_.uuid.parts[0], , ofpacts_p,
> >>>header_.uuid);
> >>
> >> +if (zone_ids->drop) {
> >> +/* Table 39, Priority 1.
> >> + * ===
> >> + *
> >> + * Clear the logical registers (for consistent behavior with 
> >> packets
> >> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */
> >> +match_init_catchall();
> >> +ofpbuf_clear(ofpacts_p);
> >> +match_set_metadata(, htonll(dp_key));
> >> +for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> >> +if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) {
> >> +put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
> >> +}
> >> +}
>  Why do we need this?  Don't we load the CT zone anyway for "to-lport"
>  ACLs?  Can't we also load the drop zone in the same place?
> >>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone
> >>> for “to-lport” ACLs (reg 13) but this happens before table=39 where
> >>> registers 0 to 9 are cleared.
> >>
> >> Oh, I see now, it's because the drop zone is stored in reg9.  I wanted
> >> to suggest not allowing reg9 to be used as logical register anymore but
> >> that's not really easy because it's used in the router pipeline:
> >>
> >> /* Register to store the result of check_pkt_larger action. */
> >> #define REGBIT_PKT_LARGER"reg9[1]"
> >> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> >> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
> >> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]"
> >> #define REGBIT_KNOWN_ECMP_NH"reg9[5]"
> >> #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
> >>
> >> [...]
> >> #define REG_ORIG_TP_DPORT_ROUTER   "reg9[16..31]"
> >>
> >> Can we reuse one of the router-specific zones registers instead?
> >>
> >> #define MFF_LOG_DNAT_ZONE  MFF_REG11  /* conntrack dnat zone for gateway
> >> router
> >>   * (32 bits). */
> >> #define MFF_LOG_SNAT_ZONE  MFF_REG12  /* conntrack snat zone for gateway
> >> router
> >>   * (32 bits). */
> >
> > Currently, we seem to be allocating SNAT and DNAT zones for
> > logical switches too and loading registers 11 and 12.
> >
>
> You're right, we are.  And we're also using the SNAT zone for LB
> hairpin.  But AFAICT we don't use the DNAT zone in the switch pipeline
> (and I don't think we'll ever use it).
>
> > bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch.
> >
> > $ ovn-appctl ct-zone-list
> > bb83b543-0d9d-409b-832c-4fc235355289_dnat 1
> > sw0p1 3
> > bb83b543-0d9d-409b-832c-4fc235355289_snat 2
> > bb83b543-0d9d-409b-832c-4fc235355289_drop 4
> >
> >  cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, 
> > idle_age=27, priority=100,in_port=1 
> > actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8)
> >
> >  cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, 
> > idle_age=27, priority=100,reg15=0x1,metadata=0x1 
> > actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39)
> >
> > Is it ok to reuse these registers?
> >
>
> I think it's OK to reuse the DNAT register; Numan do you agree?

 In the switch pipeline we do NAT on the port zones and for the load
balancer in the snat zone.  So I think it should be fine.

Thanks
Numan

>
> Thanks!
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] learning-switch: Fix coredump of OpenFlow15 learning-switch

[Simon Horman]

On Thu, Mar 30, 2023 at 05:13:25PM +0800, Faicker Mo wrote:
> The OpenFlow15 Packet-Out message contains the match instead of the in_port.
> The flow.tunnel.metadata.tab is not inited but used in the loop of 
> tun_metadata_to_nx_match.
> 
> The coredump gdb backtrace is:
>  #0  memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, 
> src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467
>  #1  0x004506e8 in metadata_loc_from_match_read 
> (match=0x7ffcfac30598, is_masked=, mask=0x7ffcfac30838, 
> idx=0, map=0x0) at lib/tun-metadata.c:865
>  #2  metadata_loc_from_match_read (is_masked=, 
> mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at 
> lib/tun-metadata.c:854
>  #3  tun_metadata_to_nx_match (b=b@entry=0x892260, 
> oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598) at 
> lib/tun-metadata.c:888
>  #4  0x0047c1f8 in nx_put_raw (b=b@entry=0x892260, 
> oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, 
> cookie=, cookie@entry=0,
>  cookie_mask=, cookie_mask@entry=0) at lib/nx-match.c:1186
>  #5  0x0047d693 in oxm_put_match (b=b@entry=0x892260, 
> match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at 
> lib/nx-match.c:1343
>  #6  0x0043194e in ofputil_encode_packet_out 
> (po=po@entry=0x7ffcfac30580, protocol=) at 
> lib/ofp-packet.c:1226
>  #7  0x0040a4fe in process_packet_in (sw=sw@entry=0x891d70, 
> oh=) at lib/learning-switch.c:619
>  #8  0x0040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) 
> at lib/learning-switch.c:374
>  #9  lswitch_run (sw=0x891d70) at lib/learning-switch.c:324
>  #10 0x00406f26 in main (argc=, argv=) 
> at utilities/ovs-testcontroller.c:180
> 
> Fix that by zeroing out the po variable.
> 
> Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support")
> Signed-off-by: Faicker Mo 

Reviewed-by: Simon Horman 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] ci: Change scheduled jobs to use the latest OVS stable branch.

[Dumitru Ceara]

On 4/3/23 13:47, Ilya Maximets wrote:
> On 4/3/23 10:33, Dumitru Ceara wrote:
>> Until now weekly OVN jobs would try to compile against OVS master
>> branch.  But that potentially contains changes that break API.  For
>> example a recent OVS commit [0] changed the signature of the
>> daemonize_start() function.  In order to avoid build failures due
>> to such changes, adapt the weekly OVN CI job to compile against the most
>> recent OVS stable branch commit.  Most likely that won't contain changes
>> that break APIs used by OVN.
> 
> FWIW, OVN is using non-public APIs, so the breakage is still very possible,
> if the internal API change is needed to fix a bug in OVS.
> 

Right and there are quite a few places where OVN does that.  We should
fix it on the long term, I agree.

> To be clear, I'm not against the change as it will definitely reduce the
> probability of a CI breakage, just pointing out that the issue is not fully
> solved.
> 

I pushed this patch to the main branch.  Thanks Ilya and Ales for the
review and feedback!

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 22.12 ovn] controller: lflow: do not use tcp as default IP protocol for ct_snat_to_vip action

[Dumitru Ceara]

On 4/1/23 10:46, Lorenzo Bianconi wrote:
> Fix non-tcp haripin use-case if the load-balancer is configured without
> ports.
> 
> Fixes: 022ea339c8e2 ("lflow: Use learn() action to generate LB hairpin reply 
> flows.")
> Tested-by: Ying Xu 
> Reviewed-by: Simon Horman 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2157846
> Signed-off-by: Lorenzo Bianconi 
> Acked-by: Ales Musil 
> Signed-off-by: Dumitru Ceara 
> ---

Thanks, Lorenzo, for the backport!  I applied this one and the other
corresponding ones to all stable branches down to 21.12.

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] ci: Change scheduled jobs to use the latest OVS stable branch.

[Ilya Maximets]

On 4/3/23 10:33, Dumitru Ceara wrote:
> Until now weekly OVN jobs would try to compile against OVS master
> branch.  But that potentially contains changes that break API.  For
> example a recent OVS commit [0] changed the signature of the
> daemonize_start() function.  In order to avoid build failures due
> to such changes, adapt the weekly OVN CI job to compile against the most
> recent OVS stable branch commit.  Most likely that won't contain changes
> that break APIs used by OVN.

FWIW, OVN is using non-public APIs, so the breakage is still very possible,
if the internal API change is needed to fix a bug in OVS.

To be clear, I'm not against the change as it will definitely reduce the
probability of a CI breakage, just pointing out that the issue is not fully
solved.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] net: openvswitch: fix race on port output

[Felix Hüttner via dev]

Thanks for the review

On Sat, 1 Apr 2023 6:25:00 + Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 06:25:13 + Felix Hüttner wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 253584777101..6628323b7bea 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -3199,6 +3199,7 @@ static u16 skb_tx_hash(const struct net_device *dev,
> > }
> >
> > if (skb_rx_queue_recorded(skb)) {
> > +   BUG_ON(unlikely(qcount == 0));
>
> DEBUG_NET_WARN_ON()
>

However if this condition triggers we will be permanently stuck in the loop 
below.
>From my understading this also means that future calls to `synchronize_net` 
>will never finish (as the packet never finishes processing).
So the user will quite probably need to restart his system.
I find DEBUG_NET_WARN_ON_ONCE to offer too little visiblity as CONFIG_DEBUG_NET 
is not necessarily enabled per default.
I as the user would see it as helpful to have this information available 
without additional config flags.
I would propose to use WARN_ON_ONCE

> > hash = skb_get_rx_queue(skb);
> > if (hash >= qoffset)
> > hash -= qoffset;
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index ca3ebfdb3023..33b317e5f9a5 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -913,7 +913,7 @@ static void do_output(struct datapath *dp, struct 
> > sk_buff *skb, int
> out_port,
> >  {
> > struct vport *vport = ovs_vport_rcu(dp, out_port);
> >
> > -   if (likely(vport)) {
> > +   if (likely(vport && vport->dev->reg_state == NETREG_REGISTERED)) {
>
> Without looking too closely netif_carrier_ok() seems like a more
> appropriate check for liveness on the datapath?

Yes, will use that in v2

> > u16 mru = OVS_CB(skb)->mru;
> > u32 cutlen = OVS_CB(skb)->cutlen;
> >
> > --
> > 2.40.0
> >
> > Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für 
> > die Verwertung
> durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der vorgesehene 
> Empfänger
> sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen 
> diese E Mail.
> Hinweise zum Datenschutz finden Sie
> hier warz%2F=05%7C01%7C%7Cbc601e5604854cc671e208db32691a22%7Cd04f47175a6e4b98b3f96918e0385
> f4c%7C0%7C0%7C638159199209626766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C=OiRwLDMENMut92J%2Fl0Hs6n8sTWFQO1kc
> Dy7mN%2B4AX8Q%3D=0>.
>
> You gotta get rid of this to work upstream.

working on it
Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die 
Verwertung durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der 
vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in 
Kenntnis und löschen diese E Mail. Hinweise zum Datenschutz finden Sie 
hier.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v10] netdev-offload-tc: del ufid mapping if device not exist.

[Simon Horman]

On Fri, Mar 31, 2023 at 03:22:44PM +0200, Eelco Chaudron wrote:
> 
> 
> On 30 Mar 2023, at 11:27, Faicker Mo wrote:
> 
> > The device may be deleted and added with ifindex changed.
> > The tc rules on the device will be deleted if the device is deleted.
> > The func tc_del_filter will fail when flow del. The mapping of
> > ufid to tc will not be deleted.
> > The traffic will trigger the same flow(with same ufid) to put to tc
> > on the new device. Duplicated ufid mapping will be added.
> > If the hashmap is expanded, the old mapping entry will be the first entry,
> > and now the dp flow can't be deleted.
> >
> > Signed-off-by: Faicker Mo 
> 
> Changes look good to me, so if Simon’s tests pass over the weekend:
> 
> Acked-by: Eelco Chaudron 

The weekend came and the weekend went.
I checked just now and the loop has run the test 73014 times
without recording any failures.

I think we are good here :)

Reviewed-by: Simon Horman 
Tested-by: Simon Horman 

> 
> 
> > +])
> > +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.3 | FORMAT_PING], [0], 
> > [dnl
> > +2 packets transmitted, 2 received, 0% packet loss, time 0ms
> > +])
> > +
> > +AT_CHECK([ovs-appctl revalidator/purge], [0])
> > +dnl Fix purge fail occasionally
> 
> FYI, I noticed when debugging some other TC-related tests that the reason 
> could be that the purge is called before the TC flows get installed in the 
> kernel.
> 
> > +AT_CHECK([ovs-appctl revalidator/purge], [0])
> 
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] utilities: disable OVSDB inactivity probes for non-daemon ovn-nbctl

[Ilya Maximets]

On 3/31/23 17:17, Dumitru Ceara wrote:
> On 3/31/23 16:51, Vladislav Odintsov wrote:
>> As I understood from Ilya, in case of one-command run of ovn-sbctl 
>> (non-daemon mode), it doesn’t make sense to have client -> server inactivity 
>> probes. If cable is unplugged, will just hit tcp session timeout, IIUC.
>> Please correct me if I’m wrong.
>>
> 
> You're right but the default timeouts are quite large for TCP sessions:
> 
> With keepalives default:
> tcp_keepalive_time - INTEGER
>   How often TCP sends out keepalive messages when keepalive is enabled.
>   Default: 2hours.
> 
> tcp_keepalive_probes - INTEGER
>   How many keepalive probes TCP sends out, until it decides that the
>   connection is broken. Default value: 9.
> 
> tcp_keepalive_intvl - INTEGER
>   How frequently the probes are send out. Multiplied by
>   tcp_keepalive_probes it is time to kill not responding connection,
>   after probes started. Default value: 75sec i.e. connection
>   will be aborted after ~11 minutes of retries.
> 
> DB clients don't even enable tcp keepalives IIRC.
> 
> So then we depend retransmits timing out:
> tcp_retries2 - INTEGER
>   This value influences the timeout of an alive TCP connection,
>   when RTO retransmissions remain unacknowledged.
>   Given a value of N, a hypothetical TCP connection following
>   exponential backoff with an initial RTO of TCP_RTO_MIN would
>   retransmit N times before killing the connection at the (N+1)th RTO.
> 
>   The default value of 15 yields a hypothetical timeout of 924.6
>   seconds and is a lower bound for the effective timeout.
>   TCP will effectively time out at the first RTO which exceeds the
>   hypothetical timeout.
> 
>   RFC 1122 recommends at least 100 seconds for the timeout,
>   which corresponds to a value of at least 8.
> 
> Isn't it possible that the connection suddenly goes down in between a
> transaction request and its reply but with all TCP segments being
> ACKed?

Right.  If the client already sent the request and only waits for a reply,
the retransmission timeout will not have any effect.  An idle TCP session
may stay open practically forever, because it is designed to survive network
disruptions.  There is no such thing as a TCP timeout in a general case.

> 
>>> On 31 Mar 2023, at 15:55, Dumitru Ceara  wrote:
>>>
>>> On 3/31/23 14:46, Vladislav Odintsov wrote:
 Hi Dumitru,

> On 31 Mar 2023, at 15:01, Dumitru Ceara  wrote:
>
> On 3/31/23 11:43, Ales Musil wrote:
>> On Thu, Mar 23, 2023 at 8:25 PM Vladislav Odintsov 
>> wrote:
>>
>>> For large OVN_Southbound DBs defatult interval of 5000 ms could be not
>>> sufficient.  This patch disables OVSDB inactivity probes for ovn-*ctl
>>> running
>>> in non-daemon mode.
>>>
>>> Signed-off-by: Vladislav Odintsov 
>>> ---
>>> utilities/ovn-dbctl.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
>>> index 369a6a663..4307a5cae 100644
>>> --- a/utilities/ovn-dbctl.c
>>> +++ b/utilities/ovn-dbctl.c
>>> @@ -208,6 +208,9 @@ ovn_dbctl_main(int argc, char *argv[],
>>>if (daemon_mode) {
>>>server_loop(dbctl_options, idl, argc, argv_);
>>>} else {
>>> +/* Disable OVSDB probe interval for non-daemon mode. */
>>> +ovsdb_idl_set_probe_interval(idl, 0);
>
> I think I'd avoid using the idl function directly and call instead:
>
> set_idl_probe_interval(idl, 0);
>
> Just to keep it aligned with all other uses in OVN.  I can patch that at
> apply time if it looks OK to you.

 I’ve got no objections here.
 Small nit: set_idl_probe_interval function needs also a remote. Like this:

 set_idl_probe_interval(idl, db, 0);

 Also, please correct typo in commit message: defatult -> default.

>>>
>>> In light of the ovs-discuss thread [0] is it maybe better to just set
>>> this probe interval to a very high value instead?  That's for the case
>>> when ovn-nbctl/sbctl daemon <-> ovsdb-server connection dies because of
>>> for example cable being unplugged somewhere on the way between the two.
>>>
>>> [0]
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-March/052324.html
>>>
>
>>> +
>>>struct ctl_command *commands;
>>>size_t n_commands;
>>>char *error;
>>> --
>>> 2.36.1
>>>
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>> Looks good to me, thanks.
>>
>> Reviewed-by: Ales Musil 
>>
>
> Vladislav, Ales, I was thinking of backporting this to stable branches
> too, what do you think?
>
> Thanks,
> Dumitru


 Regards,
 Vladislav Odintsov

Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack

[Dumitru Ceara]

On 4/3/23 11:47, Abhiram Sangana wrote:
> 
> 
>> On 29 Mar 2023, at 16:21, Dumitru Ceara  wrote:
>>
>> On 3/29/23 12:00, Abhiram Sangana wrote:
>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key,
>>pb->header_.uuid.parts[0], , ofpacts_p,
>>>header_.uuid);
>>
>> +if (zone_ids->drop) {
>> +/* Table 39, Priority 1.
>> + * ===
>> + *
>> + * Clear the logical registers (for consistent behavior with 
>> packets
>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */
>> +match_init_catchall();
>> +ofpbuf_clear(ofpacts_p);
>> +match_set_metadata(, htonll(dp_key));
>> +for (int i = 0; i < MFF_N_LOG_REGS; i++) {
>> +if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) {
>> +put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
>> +}
>> +}
 Why do we need this?  Don't we load the CT zone anyway for "to-lport"
 ACLs?  Can't we also load the drop zone in the same place?
>>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone
>>> for “to-lport” ACLs (reg 13) but this happens before table=39 where
>>> registers 0 to 9 are cleared.
>>
>> Oh, I see now, it's because the drop zone is stored in reg9.  I wanted
>> to suggest not allowing reg9 to be used as logical register anymore but
>> that's not really easy because it's used in the router pipeline:
>>
>> /* Register to store the result of check_pkt_larger action. */
>> #define REGBIT_PKT_LARGER"reg9[1]"
>> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
>> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
>> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]"
>> #define REGBIT_KNOWN_ECMP_NH"reg9[5]"
>> #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
>>
>> [...]
>> #define REG_ORIG_TP_DPORT_ROUTER   "reg9[16..31]"
>>
>> Can we reuse one of the router-specific zones registers instead?
>>
>> #define MFF_LOG_DNAT_ZONE  MFF_REG11  /* conntrack dnat zone for gateway
>> router
>>   * (32 bits). */
>> #define MFF_LOG_SNAT_ZONE  MFF_REG12  /* conntrack snat zone for gateway
>> router
>>   * (32 bits). */
> 
> Currently, we seem to be allocating SNAT and DNAT zones for
> logical switches too and loading registers 11 and 12.
> 

You're right, we are.  And we're also using the SNAT zone for LB
hairpin.  But AFAICT we don't use the DNAT zone in the switch pipeline
(and I don't think we'll ever use it).

> bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch.
> 
> $ ovn-appctl ct-zone-list
> bb83b543-0d9d-409b-832c-4fc235355289_dnat 1
> sw0p1 3
> bb83b543-0d9d-409b-832c-4fc235355289_snat 2
> bb83b543-0d9d-409b-832c-4fc235355289_drop 4
> 
>  cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, 
> idle_age=27, priority=100,in_port=1 
> actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8)
> 
>  cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, 
> idle_age=27, priority=100,reg15=0x1,metadata=0x1 
> actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39)
> 
> Is it ok to reuse these registers?
> 

I think it's OK to reuse the DNAT register; Numan do you agree?

Thanks!

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] controller: Clear tunnels from old integration bridge

[Ales Musil]

After integration bridge change the tunnels would
stay on the old bridge preventing new tunnels creation
and disrupting traffic. Detect the bridge change
and clear the tunnels from the old integration bridge.

Reported-at: https://bugzilla.redhat.com/2173635
Signed-off-by: Ales Musil 
---
v2: Make sure that we don't clear tunnels belonging to
other ovn-controllers on the same host.
---
 controller/encaps.c |  42 -
 controller/encaps.h |   4 +-
 controller/ovn-controller.c |  26 +-
 tests/ovn.at| 168 
 4 files changed, 236 insertions(+), 4 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index 2662eaf98..4a79030b8 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -386,6 +386,21 @@ chassis_tzones_overlap(const struct sset *transport_zones,
 return false;
 }
 
+static void
+clear_old_tunnels(const struct ovsrec_bridge *old_br_int, const char *prefix,
+  size_t prefix_len)
+{
+for (size_t i = 0; i < old_br_int->n_ports; i++) {
+const struct ovsrec_port *port = old_br_int->ports[i];
+const char *id = smap_get(>external_ids, "ovn-chassis-id");
+if (id && !strncmp(port->name, prefix, prefix_len)) {
+VLOG_DBG("Clearing old tunnel port \"%s\" (%s) from bridge "
+ "\"%s\".", port->name, id, old_br_int->name);
+ovsrec_bridge_update_ports_delvalue(old_br_int, port);
+}
+}
+}
+
 void
 encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_bridge *br_int,
@@ -393,12 +408,37 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct sbrec_chassis *this_chassis,
const struct sbrec_sb_global *sbg,
const struct ovsrec_open_vswitch_table *ovs_table,
-   const struct sset *transport_zones)
+   const struct sset *transport_zones,
+   const struct ovsrec_bridge_table *bridge_table,
+   const char *br_int_name)
 {
 if (!ovs_idl_txn || !br_int) {
 return;
 }
 
+if (!br_int_name) {
+/* The controller has just started, we need to look through all
+ * bridges for old tunnel ports. */
+char *tunnel_prefix = xasprintf("ovn%s-", get_chassis_idx(ovs_table));
+size_t prefix_len = strlen(tunnel_prefix);
+
+const struct ovsrec_bridge *br;
+OVSREC_BRIDGE_TABLE_FOR_EACH (br, bridge_table) {
+if (!strcmp(br->name, br_int->name)) {
+continue;
+}
+clear_old_tunnels(br, tunnel_prefix, prefix_len);
+}
+
+free(tunnel_prefix);
+} else if (strcmp(br_int_name, br_int->name)) {
+/* The integration bridge was changed, clear tunnel ports from
+ * the old one. */
+const struct ovsrec_bridge *old_br_int =
+get_bridge(bridge_table, br_int_name);
+clear_old_tunnels(old_br_int, "", 0);
+}
+
 const struct sbrec_chassis *chassis_rec;
 
 struct tunnel_ctx tc = {
diff --git a/controller/encaps.h b/controller/encaps.h
index 867c6f28c..cf38dac1a 100644
--- a/controller/encaps.h
+++ b/controller/encaps.h
@@ -35,7 +35,9 @@ void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct sbrec_chassis *,
 const struct sbrec_sb_global *,
 const struct ovsrec_open_vswitch_table *,
-const struct sset *transport_zones);
+const struct sset *transport_zones,
+const struct ovsrec_bridge_table *bridge_table,
+const char *br_int_name);
 
 bool encaps_cleanup(struct ovsdb_idl_txn *ovs_idl_txn,
 const struct ovsrec_bridge *br_int);
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 76be2426e..242d93823 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -536,6 +536,21 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
 *br_int_ = br_int;
 }
 
+static void
+consider_br_int_change(const struct ovsrec_bridge *br_int, char **current_name)
+{
+ovs_assert(current_name);
+
+if (!*current_name) {
+*current_name = xstrdup(br_int->name);
+}
+
+if (strcmp(*current_name, br_int->name)) {
+free(*current_name);
+*current_name = xstrdup(br_int->name);
+}
+}
+
 static void
 update_ssl_config(const struct ovsrec_ssl_table *ssl_table)
 {
@@ -4918,6 +4933,8 @@ main(int argc, char *argv[])
 char *ovn_version = ovn_get_internal_version();
 VLOG_INFO("OVN internal version is : [%s]", ovn_version);
 
+char *current_br_int_name = NULL;
+
 /* Main loop. */
 exiting = false;
 restart = false;
@@ -5070,7 +5087,9 @@ main(int argc, char *argv[])
chassis,
sbrec_sb_global_first(ovnsb_idl_loop.idl),
ovs_table,
-   _zones);
+  

Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack

[Abhiram Sangana]

> On 29 Mar 2023, at 16:21, Dumitru Ceara  wrote:
> 
> On 3/29/23 12:00, Abhiram Sangana wrote:
> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key,
>pb->header_.uuid.parts[0], , ofpacts_p,
>>header_.uuid);
> 
> +if (zone_ids->drop) {
> +/* Table 39, Priority 1.
> + * ===
> + *
> + * Clear the logical registers (for consistent behavior with 
> packets
> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */
> +match_init_catchall();
> +ofpbuf_clear(ofpacts_p);
> +match_set_metadata(, htonll(dp_key));
> +for (int i = 0; i < MFF_N_LOG_REGS; i++) {
> +if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) {
> +put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p);
> +}
> +}
>>> Why do we need this?  Don't we load the CT zone anyway for "to-lport"
>>> ACLs?  Can't we also load the drop zone in the same place?
>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone
>> for “to-lport” ACLs (reg 13) but this happens before table=39 where
>> registers 0 to 9 are cleared.
> 
> Oh, I see now, it's because the drop zone is stored in reg9.  I wanted
> to suggest not allowing reg9 to be used as logical register anymore but
> that's not really easy because it's used in the router pipeline:
> 
> /* Register to store the result of check_pkt_larger action. */
> #define REGBIT_PKT_LARGER"reg9[1]"
> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]"
> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]"
> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]"
> #define REGBIT_KNOWN_ECMP_NH"reg9[5]"
> #define REGBIT_KNOWN_LB_SESSION "reg9[6]"
> 
> [...]
> #define REG_ORIG_TP_DPORT_ROUTER   "reg9[16..31]"
> 
> Can we reuse one of the router-specific zones registers instead?
> 
> #define MFF_LOG_DNAT_ZONE  MFF_REG11  /* conntrack dnat zone for gateway
> router
>   * (32 bits). */
> #define MFF_LOG_SNAT_ZONE  MFF_REG12  /* conntrack snat zone for gateway
> router
>   * (32 bits). */

Currently, we seem to be allocating SNAT and DNAT zones for
logical switches too and loading registers 11 and 12.

bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch.

$ ovn-appctl ct-zone-list
bb83b543-0d9d-409b-832c-4fc235355289_dnat 1
sw0p1 3
bb83b543-0d9d-409b-832c-4fc235355289_snat 2
bb83b543-0d9d-409b-832c-4fc235355289_drop 4

 cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, 
idle_age=27, priority=100,in_port=1 
actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8)

 cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, 
idle_age=27, priority=100,reg15=0x1,metadata=0x1 
actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39)

Is it ok to reuse these registers?

Thanks,
Abhiram

> Regards,
> Dumitru
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] ci: Change scheduled jobs to use the latest OVS stable branch.

[Ales Musil]

On Mon, Apr 3, 2023 at 10:33 AM Dumitru Ceara  wrote:

> Until now weekly OVN jobs would try to compile against OVS master
> branch.  But that potentially contains changes that break API.  For
> example a recent OVS commit [0] changed the signature of the
> daemonize_start() function.  In order to avoid build failures due
> to such changes, adapt the weekly OVN CI job to compile against the most
> recent OVS stable branch commit.  Most likely that won't contain changes
> that break APIs used by OVN.
>
> [0]
> https://github.com/openvswitch/ovs/commit/07cf5810de8da12c700324bc421bde92376abe06
>
> Signed-off-by: Dumitru Ceara 
> ---
> V2:
> - Addressed Ales' comments:
>   - move the git checkout operations in the yaml file
>   - drop changes to build scripts
> ---
>  .github/workflows/test.yml | 31 +++
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
> index 90dc8a6f19..2f4c7f91a8 100644
> --- a/.github/workflows/test.yml
> +++ b/.github/workflows/test.yml
> @@ -70,15 +70,23 @@ jobs:
>if: github.event_name == 'schedule'
>uses: actions/checkout@v3
>
> -# Weekly runs test using OVS master instead of the
> -# submodule.
> -- name: checkout OVS master
> +# Weekly runs test using the tip of the most recent stable OVS branch
> +# instead of the submodule.
> +- name: checkout OVS
>if: github.event_name == 'schedule'
>uses: actions/checkout@v3
>with:
>  repository: 'openvswitch/ovs'
> +fetch-depth: 0
>  path: 'ovs'
> -ref: 'master'
> +
> +- name: checkout OVS most recent stable branch.
> +  if: github.event_name == 'schedule'
> +  run: |
> +git checkout \
> +  $(git branch -a -l '*branch-*' | sed 's/remotes\/origin\///' | \
> +sort -V | tail -1)
> +  working-directory: ovs
>
>  - name: update APT cache
>run:  sudo apt update
> @@ -156,15 +164,22 @@ jobs:
>  - name: checkout without submodule
>if: github.event_name == 'schedule'
>uses: actions/checkout@v3
> -# Weekly runs test using OVS master instead of the
> -# submodule.
> -- name: checkout OVS master
> +# Weekly runs test using the tip of the most recent stable OVS branch
> +# instead of the submodule.
> +- name: checkout OVS
>if: github.event_name == 'schedule'
>uses: actions/checkout@v3
>with:
>  repository: 'openvswitch/ovs'
> +fetch-depth: 0
>  path: 'ovs'
> -ref: 'master'
> +- name: checkout OVS most recent stable branch.
> +  if: github.event_name == 'schedule'
> +  run: |
> +git checkout \
> +  $(git branch -a -l '*branch-*' | sed 's/remotes\/origin\///' | \
> +sort -V | tail -1)
> +  working-directory: ovs
>  - name: install dependencies
>run:  brew install automake libtool
>  - name: update PATH
> --
> 2.31.1
>
>
Looks good to me, thanks!

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] ci: Change scheduled jobs to use the latest OVS stable branch.

[Dumitru Ceara]

On 3/31/23 11:58, Ales Musil wrote:
> On Mon, Mar 27, 2023 at 5:39 PM Dumitru Ceara  wrote:
> 
>> Until now weekly OVN jobs would try to compile against OVS master
>> branch.  But that potentially contains changes that break API.  For
>> example a recent OVS commit [0] changed the signature of the
>> daemonize_start() function.  In order to avoid build failures due
>> to such changes, adapt the weekly OVN CI job to compile against the most
>> recent OVS stable branch commit.  Most likely that won't contain changes
>> that break APIs used by OVN.
>>
>> [0]
>> https://github.com/openvswitch/ovs/commit/07cf5810de8da12c700324bc421bde92376abe06
>>
>> Signed-off-by: Dumitru Ceara 
>>
> 
> Hi Dumitru,
> this unfortunately breaks the container CI. As it is kinda unexpected to
> do something with the git repos inside the build-x.sh. Would it be possible
> to have it in the workflow instead?. That would also make it way more
> obvious what
> is the intention.
> 

Hi Ales,

Thanks for your review!  You're right it's more clear to just add the
checkout to the workflow directly.  I did that in v2:

https://patchwork.ozlabs.org/project/ovn/patch/20230403083304.2720797-1-dce...@redhat.com/

Regards,
Dumitru

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v2] ci: Change scheduled jobs to use the latest OVS stable branch.

[Dumitru Ceara]

Until now weekly OVN jobs would try to compile against OVS master
branch.  But that potentially contains changes that break API.  For
example a recent OVS commit [0] changed the signature of the
daemonize_start() function.  In order to avoid build failures due
to such changes, adapt the weekly OVN CI job to compile against the most
recent OVS stable branch commit.  Most likely that won't contain changes
that break APIs used by OVN.

[0] 
https://github.com/openvswitch/ovs/commit/07cf5810de8da12c700324bc421bde92376abe06

Signed-off-by: Dumitru Ceara 
---
V2:
- Addressed Ales' comments:
  - move the git checkout operations in the yaml file
  - drop changes to build scripts
---
 .github/workflows/test.yml | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index 90dc8a6f19..2f4c7f91a8 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -70,15 +70,23 @@ jobs:
   if: github.event_name == 'schedule'
   uses: actions/checkout@v3
 
-# Weekly runs test using OVS master instead of the
-# submodule.
-- name: checkout OVS master
+# Weekly runs test using the tip of the most recent stable OVS branch
+# instead of the submodule.
+- name: checkout OVS
   if: github.event_name == 'schedule'
   uses: actions/checkout@v3
   with:
 repository: 'openvswitch/ovs'
+fetch-depth: 0
 path: 'ovs'
-ref: 'master'
+
+- name: checkout OVS most recent stable branch.
+  if: github.event_name == 'schedule'
+  run: |
+git checkout \
+  $(git branch -a -l '*branch-*' | sed 's/remotes\/origin\///' | \
+sort -V | tail -1)
+  working-directory: ovs
 
 - name: update APT cache
   run:  sudo apt update
@@ -156,15 +164,22 @@ jobs:
 - name: checkout without submodule
   if: github.event_name == 'schedule'
   uses: actions/checkout@v3
-# Weekly runs test using OVS master instead of the
-# submodule.
-- name: checkout OVS master
+# Weekly runs test using the tip of the most recent stable OVS branch
+# instead of the submodule.
+- name: checkout OVS
   if: github.event_name == 'schedule'
   uses: actions/checkout@v3
   with:
 repository: 'openvswitch/ovs'
+fetch-depth: 0
 path: 'ovs'
-ref: 'master'
+- name: checkout OVS most recent stable branch.
+  if: github.event_name == 'schedule'
+  run: |
+git checkout \
+  $(git branch -a -l '*branch-*' | sed 's/remotes\/origin\///' | \
+sort -V | tail -1)
+  working-directory: ovs
 - name: install dependencies
   run:  brew install automake libtool
 - name: update PATH
-- 
2.31.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev