[ovs-dev] [PATCH ovn 3/3] northd: Use dynamic mac-binding for virtual port IPs.

[Han Zhou]

Today ARP resolve flows (static mac-bindings) are programmed for virtual
ports once their virtual parent is claimed. As a result, during virtual
parent failover, the traffic won't switch to the new parent until the
ARP resolve flow is updated by ovn-northd, triggered by the port-binding
update. When the scale is big, the ovn-northd compute may take a long
time, e.g. 10s or even more, which is too long data plane break during
virtual parent failover.

This patch removes the dependency of ovn-northd from the failover
scenario by removing the ARP resolve flows, so that it relies on dynamic
mac-bindings to resolve virtual parent's MAC for the virtual IP. This
avoids the logical flow recompute during failover thus make it much
faster. Functionally there is no difference.

Signed-off-by: Han Zhou 
---
 northd/northd.c | 103 
 northd/ovn-northd.8.xml |  37 +++
 tests/ovn.at|  55 ++---
 tests/system-ovn.at |   1 -
 4 files changed, 11 insertions(+), 185 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 770a5b50e2c0..65cfb7975d1b 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12455,109 +12455,6 @@ build_arp_resolve_flows_for_lrouter_port(
 }
 }
 }
-} else if (op->od->n_router_ports && !lsp_is_router(op->nbsp)
-   && !strcmp(op->nbsp->type, "virtual")) {
-/* This is a virtual port. Add ARP replies for the virtual ip with
- * the mac of the present active virtual parent.
- * If the logical port doesn't have virtual parent set in
- * Port_Binding table, then add the flow to set eth.dst to
- * 00:00:00:00:00:00 and advance to next table so that ARP is
- * resolved by router pipeline using the arp{} action.
- * The MAC_Binding entry for the virtual ip might be invalid. */
-
-const char *vip = smap_get(>nbsp->options,
-   "virtual-ip");
-const char *virtual_parents = smap_get(>nbsp->options,
-   "virtual-parents");
-
-if (!vip || !virtual_parents || !op->sb) {
-return;
-}
-
-bool is_ipv4 = strchr(vip, '.') ? true : false;
-if (is_ipv4) {
-ovs_be32 ipv4;
-if (!ip_parse(vip, )) {
- return;
-}
-} else {
-struct in6_addr ipv6;
-if (!ipv6_parse(vip, )) {
- return;
-}
-}
-
-if (!op->sb->virtual_parent || !op->sb->virtual_parent[0] ||
-!op->sb->chassis) {
-/* The virtual port is not claimed yet. */
-for (size_t i = 0; i < op->od->n_router_ports; i++) {
-struct ovn_port *peer = ovn_port_get_peer(
-ports, op->od->router_ports[i]);
-if (!peer || !peer->nbrp) {
-continue;
-}
-
-if (find_lrp_member_ip(peer, vip)) {
-ds_clear(match);
-ds_put_format(
-match, "outport == %s && " "%s == %s", peer->json_key,
-is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6, vip);
-
-const char *arp_actions =
-  "eth.dst = 00:00:00:00:00:00; next;";
-ovn_lflow_add_with_hint(lflows, peer->od,
-S_ROUTER_IN_ARP_RESOLVE, 100,
-ds_cstr(match),
-arp_actions,
->nbsp->header_);
-break;
-}
-}
-} else {
-struct ovn_port *vp =
-ovn_port_find(ports, op->sb->virtual_parent);
-if (!vp || !vp->nbsp) {
-return;
-}
-
-for (size_t i = 0; i < vp->n_lsp_addrs; i++) {
-bool found_vip_network = false;
-const char *ea_s = vp->lsp_addrs[i].ea_s;
-for (size_t j = 0; j < vp->od->n_router_ports; j++) {
-/* Get the Logical_Router_Port that the
-* Logical_Switch_Port is connected to, as
-* 'peer'. */
-struct ovn_port *peer =
-ovn_port_get_peer(ports, vp->od->router_ports[j]);
-if (!peer || !peer->nbrp) {
-continue;
-}
-
-if (!find_lrp_member_ip(peer, vip)) {
-continue;
-}
-
-ds_clear(match);
-ds_put_format(
-match, "outport == %s && " "%s == %s", peer->json_key,
-is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6, vip);
-
-   

[ovs-dev] [PATCH ovn 1/3] ovn.at: Fix virtual port tests.

[Han Zhou]

The check_virtual_offlows_not_present() function in the case "virtual
ports" has the wrong table id 45, which should be 44. However,
correcting the table id makes the case failing, because the two ACLs
added by the case were in fact overlapping:

check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir") && 
ip' allow
check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir6") && 
ip' allow

Because ip4 v.s. ip6 is not specified, both ACLs would generate OVS
flows for both ip4 and ip6 when the virtual ports are resisdent on the
chassis, and the OVS flows would remain on the chassis if one of the
ports are released but the other is remaining. This is why the
check_virtual_offlows_not_present() would always fail.

This patch corrects the table id and fixes the ACLs with proper IP
protocol, and updates the check_virtual_offlows_xxx() functions so that
only ipv4 flows are dumpped and checked which is what those functions
are used for.

Signed-off-by: Han Zhou 
---
 tests/ovn.at | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index dc5c5df3f747..e7542db42503 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20921,8 +20921,8 @@ check ovn-nbctl lsp-set-addresses sw1-lr0 
00:00:00:00:ff:02
 check ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
 
 # Add an ACL that matches on sw0-vir/sw0-vir6 being bound locally.
-check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir") && 
ip' allow
-check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir6") && 
ip' allow
+check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir") && 
ip4' allow
+check ovn-nbctl acl-add sw0 to-lport 1000 'is_chassis_resident("sw0-vir6") && 
ip6' allow
 
 check ovn-nbctl ls-add public
 check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 
2001:db8::1/64
@@ -21007,9 +21007,8 @@ check_virtual_offlows_present() {
 lr0_dp_key=$(printf "%x" $(fetch_column Datapath_Binding tunnel_key 
external_ids:name=lr0))
 lr0_public_dp_key=$(printf "%x" $(fetch_column Port_Binding tunnel_key 
logical_port=lr0-public))
 
-AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=44 | 
ofctl_strip_all | grep "priority=2000"], [0], [dnl
+AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=44,ip | 
ofctl_strip_all | grep "priority=2000"], [0], [dnl
  table=44, priority=2000,ip,metadata=0x$sw0_dp_key actions=resubmit(,45)
- table=44, priority=2000,ipv6,metadata=0x$sw0_dp_key actions=resubmit(,45)
 ])
 
 AT_CHECK_UNQUOTED([as $hv ovs-ofctl dump-flows br-int table=11 | 
ofctl_strip_all | \
@@ -21020,7 +21019,7 @@ check_virtual_offlows_present() {
 
 check_virtual_offlows_not_present() {
 hv=$1
-AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | 
grep "priority=2000"], [1], [dnl
+AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=44,ip | ofctl_strip_all 
| grep "priority=2000"], [1], [dnl
 ])
 
 AT_CHECK([as $hv ovs-ofctl dump-flows br-int table=11 | ofctl_strip_all | \
-- 
2.30.2

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

[ovs-dev] [PATCH ovn 0/3] virtual port faster failover.

[Han Zhou]

Han Zhou (3):
  ovn.at: Fix virtual port tests.
  system-ovn.at: Add system test for virtual port with floating IP.
  northd: Use dynamic mac-binding for virtual port IPs.

 northd/northd.c | 103 
 northd/ovn-northd.8.xml |  37 ++
 tests/atlocal.in|   3 +
 tests/ovn.at|  64 +++---
 tests/system-ovn.at | 145 
 5 files changed, 163 insertions(+), 189 deletions(-)

-- 
2.30.2

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

[ovs-dev] [PATCH ovn 2/3] system-ovn.at: Add system test for virtual port with floating IP.

[Han Zhou]

Signed-off-by: Han Zhou 
---
 tests/atlocal.in|   3 +
 tests/system-ovn.at | 146 
 2 files changed, 149 insertions(+)

diff --git a/tests/atlocal.in b/tests/atlocal.in
index 0b9a312761c9..5526adac5241 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -187,6 +187,9 @@ find_command dhcpd
 # Set HAVE_BFDD_BEACON
 find_command bfdd-beacon
 
+# Set HAVE_ARPING
+find_command arping
+
 # Turn off proxies.
 unset http_proxy
 unset https_proxy
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 563858e70384..cccb8ec4aa95 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -10660,3 +10660,149 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port 
patch-.*/d
 /connection dropped.*/d"])
 AT_CLEANUP
 ])
+
+
+#  ls1p1 (virtual parent of VIP)
+#   \
+#   ls1 -- lr1 (floating-ip) -- public1 (localnet) -- ext1
+#   /
+#  ls1p2 (virtual parent of VIP)
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([virtual port with floating IP])
+AT_SKIP_IF([test "$HAVE_ARPING" = no])
+
+CHECK_CONNTRACK()
+CHECK_CONNTRACK_NAT()
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+ADD_BR([br-ex], [set Bridge br-ex fail-mode=standalone])
+
+check ovs-vsctl set Open_vSwitch . 
external-ids:ovn-bridge-mappings=provider:br-ex
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+-- set Open_vSwitch . external-ids:system-id=hv1 \
+-- set Open_vSwitch . 
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+-- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+-- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+-- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+# Add routers
+check ovn-nbctl lr-add lr1
+check ovn-nbctl set logical_router lr1 
options:always_learn_from_arp_request=false
+
+# Add switches
+check ovn-nbctl ls-add public1
+check ovn-nbctl ls-add ls1
+
+# Add ls1 ports
+check ovn-nbctl lsp-add ls1 ls1p1 \
+-- lsp-set-addresses ls1p1 "00:00:00:00:01:11 10.0.0.11"
+
+check ovn-nbctl lsp-add ls1 ls1p2 \
+-- lsp-set-addresses ls1p2 "00:00:00:00:01:12 10.0.0.12"
+
+check ovn-nbctl lsp-add ls1 ls1-to-lr1 \
+-- lsp-set-type ls1-to-lr1 router \
+-- lsp-set-options ls1-to-lr1 router-port=lr1-to-ls1 \
+-- lsp-set-addresses ls1-to-lr1 router
+
+# Add ls1 virtual port
+check ovn-nbctl lsp-add ls1 vip \
+-- lsp-set-addresses vip "00:00:00:00:01:88 10.0.0.88" \
+-- lsp-set-type vip virtual \
+-- set logical_switch_port vip options:virtual-ip=10.0.0.88 \
+-- set logical_switch_port vip options:virtual-parents=ls1p1,ls1p2
+
+# Add lr1 ports
+check ovn-nbctl lrp-add lr1 lr1-to-ls1 "00:00:00:0f:01:01" 10.0.0.1/24 \
+-- lrp-add lr1 lr1-to-public1 "00:00:00:0f:02:01" 172.0.0.1/24 \
+-- lrp-set-gateway-chassis lr1-to-public1 hv1 10
+
+# Add floating-ip
+check ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.0.0.88 10.0.0.88 vip 
10:54:00:00:00:88
+
+# Add public1 ports
+check ovn-nbctl lsp-add public1 public1-to-lr1 \
+-- lsp-set-type public1-to-lr1 router \
+-- lsp-set-options public1-to-lr1 router-port=lr1-to-public1 \
+-- lsp-set-addresses public1-to-lr1 router \
+-- lsp-add public1 ln1 \
+-- lsp-set-type ln1 localnet \
+-- lsp-set-options ln1 network_name=provider \
+-- lsp-set-addresses ln1 unknown
+
+check ovn-nbctl --wait=hv sync
+
+ADD_NAMESPACES(ns_ls1p1)
+ADD_VETH(ls1p1, ns_ls1p1, br-int, "10.0.0.11/24", "00:00:00:00:01:11", 
"10.0.0.1")
+
+ADD_NAMESPACES(ns_ls1p2)
+ADD_VETH(ls1p2, ns_ls1p2, br-int, "10.0.0.12/24", "00:00:00:00:01:12", 
"10.0.0.1")
+
+ADD_NAMESPACES(ns_ext1)
+ADD_VETH(ln1, ns_ext1, br-ex, "172.0.0.99/24", "0a:0a:b6:fc:03:01", 
"172.0.0.1")
+
+# Claim vip at ls1p1: configure the virtual IP and send GARP.
+NS_CHECK_EXEC([ns_ls1p1], [ip addr del 10.0.0.11/24 dev ls1p1;
+   ip addr add 10.0.0.88/24 dev ls1p1;
+   ip route add default via 10.0.0.1])
+NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 10.0.0.88])
+wait_for_ports_up vip
+check ovn-nbctl --wait=hv sync
+
+# ping virtual IP from ext1
+NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.88 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+# ping floating virtual IP from ext1
+NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 172.0.0.88 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+# Move virtual IP to ls1p2
+NS_CHECK_EXEC([ns_ls1p1], [ip addr del 10.0.0.88/24 dev ls1p1])
+NS_CHECK_EXEC([ns_ls1p2], [ip addr del 10.0.0.12/24 dev ls1p2;
+   ip addr add 10.0.0.88/24 dev ls1p2;
+   ip route add default via 10.0.0.1])

Re: [ovs-dev] [PATCH ovn 0/2] fix IP fragmented traffic hitting LB

[Ales Musil]

On Wed, Feb 22, 2023 at 4:58 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> Lorenzo Bianconi (2):
>   northd: move defrag router pipeline stage forward
>   northd: add flows to defrag IP traffic
>
>  northd/northd.c |  50 ++--
>  northd/ovn-northd.8.xml |  60 +++--
>  tests/ovn-northd.at | 526 
>  tests/ovn.at|  52 ++--
>  tests/system-ovn.at |   8 +-
>  5 files changed, 360 insertions(+), 336 deletions(-)
>
> --
> 2.39.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Hi Lorenzo,

as discussed earlier, adding my opinion on this topic.
First of all I agree that we need a separate defrag stage, however
there I see two major drawbacks with your suggestion.

1) We will send to CT all packets that are fragmented even if they are
not to be load balanced.
2) Fragmented traffic for LBs will go through CT twice (or three times if
it's a new connection).

I would suggest rearranging it slightly differently.
To solve both issues from above the defrag stage should apply only for
packets that are destined to LB VIP e.g.

match=(ip && ip.dst == VIP), action(ct_next;)

And the post-defrag/pre-dnat stage would populate all registers that are
required
for the dnat stage itself e.g.

match=(ip && ip.dst == VIP && PROTO && PROTO.dst == VIP_PORT),
action=(reg00 = VIP, reg9 = PROTO.dst, next;)

One thing that is not completely clear is why there is a ct_dnat; in the
current defrag stage as
from my point of view it would be enough to have ct_next. Unless I'm
missing something
it should be completely fine to replace it with ct_next as there shouldn't
be any unDNAT
happening because this is for original traffic and not reply.

Adding mainters to CC to get more insights especially why there is ct_dnat.

Thanks,
Ales

-- 

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 v6] netdev-offload-tc: del ufid mapping if device not exist

[Faicker Mo]

I can run the fail-test more easier.
There exists a flow in verbose log like this, 
recirc_id(0),in_port(2),eth(src=aa:1a:54:e9:c5:56,dst=86:29:2a:05:94:90),eth_type(0x0800),ipv4(frag=no),
 packets:1, bytes:84, used:12.240s, actions:3
The env is strict. The hmap should be expanded exactly only once.The flow 
number needs to be controlled.

By the way, should I updated the patch to v7 in here because I found it's need 
to be changed when I rebased to the newest master?





From: Simon Horman 
Date: 2023-02-23 01:12:46
To:  Faicker Mo 
Cc:  d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device 
not exist>On Wed, Feb 22, 2023 at 04:19:37PM +0100, Simon Horman wrote:
>> On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
>> > It's not easy to add a fail test without the changed code.
>> > But I test it failed with the old code manually following these steps,
>> > 1. Apply this patch(with test in it)
>> > 2. Revert the changed code in netdev-offload-tc.c
>> > 3. Run the test
>> > 
>> > 
>> > Yes, the fail-test above sometimes may pass because of the env and the 
>> > chance.Maybe run the fail-test several times.
>> 
>> Thanks, I see this now.
>> 
>> * Without the C-code changes in this patch I saw the test fail 3 times,
>>   each time within 5 attempts.
>> 
>> * With the C-code change I am yet to see the test fail,
>>   so far I'm up to 80 attempts.
>
>It did eventually fail on the 257th attempt.
>
>I'll try again with '-v' and see if I can capture anything useful.
>
>> I do wonder if this warrants a Fixes tag.
>> And if so, if it should be:
>> 
>> Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the 
>> right place")
>> 
>> That notwithstanding, I am happy with this patch.
>> 
>> Reviewed-by: Simon Horman 




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

Re: [ovs-dev] [RFC] dpdk: Allow retaining cap_sys_rawio privileges

[0-day Robot]

Bleep bloop.  Greetings Aaron Conole, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 103 characters long (recommended limit is 79)
#112 FILE: lib/daemon-unix.c:838:
VLOG_WARN("hw port access requested, but no userspace 
ioport support.  Dropping.");

Lines checked: 390, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Simon Horman]

On Wed, Feb 22, 2023 at 04:19:37PM +0100, Simon Horman wrote:
> On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
> > It's not easy to add a fail test without the changed code.
> > But I test it failed with the old code manually following these steps,
> > 1. Apply this patch(with test in it)
> > 2. Revert the changed code in netdev-offload-tc.c
> > 3. Run the test
> > 
> > 
> > Yes, the fail-test above sometimes may pass because of the env and the 
> > chance.Maybe run the fail-test several times.
> 
> Thanks, I see this now.
> 
> * Without the C-code changes in this patch I saw the test fail 3 times,
>   each time within 5 attempts.
> 
> * With the C-code change I am yet to see the test fail,
>   so far I'm up to 80 attempts.

It did eventually fail on the 257th attempt.

I'll try again with '-v' and see if I can capture anything useful.

> I do wonder if this warrants a Fixes tag.
> And if so, if it should be:
> 
> Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the right 
> place")
> 
> That notwithstanding, I am happy with this patch.
> 
> Reviewed-by: Simon Horman 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC] dpdk: Allow retaining cap_sys_rawio privileges

[Aaron Conole]

Apologies - I mis-typed Gaetan's email when I entered it into my mail
file.  CC'd correctly on this email (but I can resend the patch, if you
think it is better).

Aaron Conole  writes:

> Open vSwitch generally tries to let the underlying operating system
> managed the low level details of hardware, for example DMA mapping,
> bus arbitration, etc.  However, when using DPDK, the underlying
> operating system yields control of many of these details to userspace
> for management.
>
> In the case of some DPDK port drivers, configuring rte_flow or even
> allocating resources may require access to iopl/ioperm calls, which
> are guarded by the CAP_SYS_RAWIO privilege on linux systems.  These
> calls are dangerous, and can allow a process to completely compromise
> a system.  However, they are needed in the case of some userspace
> driver code which manages the hardware (for example, the mlx
> implementation of backend support for rte_flow).
>
> Here, we create an opt-in flag passed to the command line to allow
> this access.  We need to do this before ever accessing the database,
> because we want to drop all privileges asap, and cannot wait for
> a connection to the database to be established and functional before
> dropping.  There may be distribution specific ways to do capability
> management as well (using for example, systemd), but they are not
> as universal to the vswitchd as a flag.
>
> Signed-off-by: Aaron Conole 
> ---
>  NEWS   |  4 
>  lib/daemon-unix.c  | 31 ++-
>  lib/daemon.c   |  2 +-
>  lib/daemon.h   |  4 ++--
>  ovsdb/ovsdb-client.c   |  6 +++---
>  ovsdb/ovsdb-server.c   |  4 ++--
>  tests/test-netflow.c   |  2 +-
>  tests/test-sflow.c |  2 +-
>  tests/test-unixctl.c   |  2 +-
>  utilities/ovs-ofctl.c  |  4 ++--
>  utilities/ovs-testcontroller.c |  4 ++--
>  vswitchd/ovs-vswitchd.8.in |  8 
>  vswitchd/ovs-vswitchd.c| 11 ++-
>  13 files changed, 59 insertions(+), 25 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 85b3496214..65f35dcdd5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,10 @@ Post-v3.1.0
> in order to create OVSDB sockets with access mode of 0770.
> - QoS:
>   * Added new configuration option 'jitter' for a linux-netem QoS type.
> +   - DPDK:
> + * ovs-vswitchd will keep the CAP_SYS_RAWIO capability when started
> +   with the --hw-rawio-access command line option.  This allows the
> +   process extra privileges when mapping physical interconnect memory.
>  
>  
>  v3.1.0 - 16 Feb 2023
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index 1a7ba427d7..8b895a48de 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -88,7 +88,8 @@ static bool switch_user = false;
>  static uid_t uid;
>  static gid_t gid;
>  static char *user = NULL;
> -static void daemon_become_new_user__(bool access_datapath);
> +static void daemon_become_new_user__(bool access_datapath,
> + bool access_hardware_ports);
>  
>  static void check_already_running(void);
>  static int lock_pidfile(FILE *, int command);
> @@ -443,13 +444,13 @@ monitor_daemon(pid_t daemon_pid)
>   * daemonize_complete()) or that it failed to start up (by exiting with a
>   * nonzero exit code). */
>  void
> -daemonize_start(bool access_datapath)
> +daemonize_start(bool access_datapath, bool access_hardware_ports)
>  {
>  assert_single_threaded();
>  daemonize_fd = -1;
>  
>  if (switch_user) {
> -daemon_become_new_user__(access_datapath);
> +daemon_become_new_user__(access_datapath, access_hardware_ports);
>  switch_user = false;
>  }
>  
> @@ -807,7 +808,8 @@ daemon_become_new_user_unix(void)
>  /* Linux specific implementation of daemon_become_new_user()
>   * using libcap-ng.   */
>  static void
> -daemon_become_new_user_linux(bool access_datapath OVS_UNUSED)
> +daemon_become_new_user_linux(bool access_datapath OVS_UNUSED,
> + bool access_hardware_ports OVS_UNUSED)
>  {
>  #if defined __linux__ &&  HAVE_LIBCAPNG
>  int ret;
> @@ -826,7 +828,17 @@ daemon_become_new_user_linux(bool access_datapath 
> OVS_UNUSED)
>  if (access_datapath && !ret) {
>  ret = capng_update(CAPNG_ADD, cap_sets, CAP_NET_ADMIN)
>|| capng_update(CAPNG_ADD, cap_sets, CAP_NET_RAW)
> -  || capng_update(CAPNG_ADD, cap_sets, 
> CAP_NET_BROADCAST);
> +  || capng_update(CAPNG_ADD, cap_sets, CAP_NET_BROADCAST)
> +#ifdef DPDK_NETDEV
> +  || (access_hardware_ports &&
> +  capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO))
> +#else
> +;
> +if (access_hardware_ports) {
> +VLOG_WARN("hw port access requested, but no userspace 
> ioport support.  

[ovs-dev] [RFC] dpdk: Allow retaining cap_sys_rawio privileges

[Aaron Conole]

Open vSwitch generally tries to let the underlying operating system
managed the low level details of hardware, for example DMA mapping,
bus arbitration, etc.  However, when using DPDK, the underlying
operating system yields control of many of these details to userspace
for management.

In the case of some DPDK port drivers, configuring rte_flow or even
allocating resources may require access to iopl/ioperm calls, which
are guarded by the CAP_SYS_RAWIO privilege on linux systems.  These
calls are dangerous, and can allow a process to completely compromise
a system.  However, they are needed in the case of some userspace
driver code which manages the hardware (for example, the mlx
implementation of backend support for rte_flow).

Here, we create an opt-in flag passed to the command line to allow
this access.  We need to do this before ever accessing the database,
because we want to drop all privileges asap, and cannot wait for
a connection to the database to be established and functional before
dropping.  There may be distribution specific ways to do capability
management as well (using for example, systemd), but they are not
as universal to the vswitchd as a flag.

Signed-off-by: Aaron Conole 
---
 NEWS   |  4 
 lib/daemon-unix.c  | 31 ++-
 lib/daemon.c   |  2 +-
 lib/daemon.h   |  4 ++--
 ovsdb/ovsdb-client.c   |  6 +++---
 ovsdb/ovsdb-server.c   |  4 ++--
 tests/test-netflow.c   |  2 +-
 tests/test-sflow.c |  2 +-
 tests/test-unixctl.c   |  2 +-
 utilities/ovs-ofctl.c  |  4 ++--
 utilities/ovs-testcontroller.c |  4 ++--
 vswitchd/ovs-vswitchd.8.in |  8 
 vswitchd/ovs-vswitchd.c| 11 ++-
 13 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/NEWS b/NEWS
index 85b3496214..65f35dcdd5 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,10 @@ Post-v3.1.0
in order to create OVSDB sockets with access mode of 0770.
- QoS:
  * Added new configuration option 'jitter' for a linux-netem QoS type.
+   - DPDK:
+ * ovs-vswitchd will keep the CAP_SYS_RAWIO capability when started
+   with the --hw-rawio-access command line option.  This allows the
+   process extra privileges when mapping physical interconnect memory.
 
 
 v3.1.0 - 16 Feb 2023
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index 1a7ba427d7..8b895a48de 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -88,7 +88,8 @@ static bool switch_user = false;
 static uid_t uid;
 static gid_t gid;
 static char *user = NULL;
-static void daemon_become_new_user__(bool access_datapath);
+static void daemon_become_new_user__(bool access_datapath,
+ bool access_hardware_ports);
 
 static void check_already_running(void);
 static int lock_pidfile(FILE *, int command);
@@ -443,13 +444,13 @@ monitor_daemon(pid_t daemon_pid)
  * daemonize_complete()) or that it failed to start up (by exiting with a
  * nonzero exit code). */
 void
-daemonize_start(bool access_datapath)
+daemonize_start(bool access_datapath, bool access_hardware_ports)
 {
 assert_single_threaded();
 daemonize_fd = -1;
 
 if (switch_user) {
-daemon_become_new_user__(access_datapath);
+daemon_become_new_user__(access_datapath, access_hardware_ports);
 switch_user = false;
 }
 
@@ -807,7 +808,8 @@ daemon_become_new_user_unix(void)
 /* Linux specific implementation of daemon_become_new_user()
  * using libcap-ng.   */
 static void
-daemon_become_new_user_linux(bool access_datapath OVS_UNUSED)
+daemon_become_new_user_linux(bool access_datapath OVS_UNUSED,
+ bool access_hardware_ports OVS_UNUSED)
 {
 #if defined __linux__ &&  HAVE_LIBCAPNG
 int ret;
@@ -826,7 +828,17 @@ daemon_become_new_user_linux(bool access_datapath 
OVS_UNUSED)
 if (access_datapath && !ret) {
 ret = capng_update(CAPNG_ADD, cap_sets, CAP_NET_ADMIN)
   || capng_update(CAPNG_ADD, cap_sets, CAP_NET_RAW)
-  || capng_update(CAPNG_ADD, cap_sets, CAP_NET_BROADCAST);
+  || capng_update(CAPNG_ADD, cap_sets, CAP_NET_BROADCAST)
+#ifdef DPDK_NETDEV
+  || (access_hardware_ports &&
+  capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO))
+#else
+;
+if (access_hardware_ports) {
+VLOG_WARN("hw port access requested, but no userspace 
ioport support.  Dropping.");
+}
+#endif
+;
 }
 } else {
 ret = -1;
@@ -854,7 +866,7 @@ daemon_become_new_user_linux(bool access_datapath 
OVS_UNUSED)
 }
 
 static void
-daemon_become_new_user__(bool access_datapath)
+daemon_become_new_user__(bool access_datapath, bool access_hardware_ports)
 {
 /* If vlog file has been created, change its owner to the 

Re: [ovs-dev] [PATCH ovn 1/2] northd: move defrag router pipeline stage forward

[0-day Robot]

Bleep bloop.  Greetings Lorenzo Bianconi, I am a robot and I have tried out 
your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 101 characters long (recommended limit is 79)
#69 FILE: northd/northd.c:326:
 * | |  (>= IP_INPUT)|   | | E |
NEXT_HOP_IPV6 (>= POST_DEFRAG ) |

Lines checked: 1324, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn 1/2] northd: move defrag router pipeline stage forward

[Lorenzo Bianconi]

Introduce a post_defrag router ingress pipeline stage and move current defrag
code in post_defrag stage. This is a preliminary patch to just defrag IP
fragment traffic (without performing DNAT) before accessing L4 info (e.g.
L4 protocol port) since they are not available in all IP fragment.

Signed-off-by: Lorenzo Bianconi 
---
 northd/northd.c |  46 ++--
 northd/ovn-northd.8.xml |  45 ++--
 tests/ovn-northd.at | 526 
 tests/ovn.at|  52 ++--
 tests/system-ovn.at |   8 +-
 5 files changed, 343 insertions(+), 334 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 770a5b50e..97589e31d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -158,22 +158,23 @@ enum ovn_stage {
 PIPELINE_STAGE(ROUTER, IN,  IP_INPUT,3, "lr_in_ip_input") \
 PIPELINE_STAGE(ROUTER, IN,  UNSNAT,  4, "lr_in_unsnat")   \
 PIPELINE_STAGE(ROUTER, IN,  DEFRAG,  5, "lr_in_defrag")   \
-PIPELINE_STAGE(ROUTER, IN,  LB_AFF_CHECK,6, "lr_in_lb_aff_check") \
-PIPELINE_STAGE(ROUTER, IN,  DNAT,7, "lr_in_dnat") \
-PIPELINE_STAGE(ROUTER, IN,  LB_AFF_LEARN,8, "lr_in_lb_aff_learn") \
-PIPELINE_STAGE(ROUTER, IN,  ECMP_STATEFUL,   9, "lr_in_ecmp_stateful") \
-PIPELINE_STAGE(ROUTER, IN,  ND_RA_OPTIONS,   10, "lr_in_nd_ra_options") \
-PIPELINE_STAGE(ROUTER, IN,  ND_RA_RESPONSE,  11, "lr_in_nd_ra_response") \
-PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_PRE,  12, "lr_in_ip_routing_pre")  \
-PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,  13, "lr_in_ip_routing")  \
-PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 14, "lr_in_ip_routing_ecmp") \
-PIPELINE_STAGE(ROUTER, IN,  POLICY,  15, "lr_in_policy")  \
-PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP, 16, "lr_in_policy_ecmp") \
-PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE, 17, "lr_in_arp_resolve") \
-PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN, 18, "lr_in_chk_pkt_len") \
-PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS, 19, "lr_in_larger_pkts") \
-PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT, 20, "lr_in_gw_redirect") \
-PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST, 21, "lr_in_arp_request") \
+PIPELINE_STAGE(ROUTER, IN,  POST_DEFRAG, 6, "lr_in_post_defrag")  \
+PIPELINE_STAGE(ROUTER, IN,  LB_AFF_CHECK,7, "lr_in_lb_aff_check") \
+PIPELINE_STAGE(ROUTER, IN,  DNAT,8, "lr_in_dnat") \
+PIPELINE_STAGE(ROUTER, IN,  LB_AFF_LEARN,9, "lr_in_lb_aff_learn") \
+PIPELINE_STAGE(ROUTER, IN,  ECMP_STATEFUL,   10, "lr_in_ecmp_stateful") \
+PIPELINE_STAGE(ROUTER, IN,  ND_RA_OPTIONS,   11, "lr_in_nd_ra_options") \
+PIPELINE_STAGE(ROUTER, IN,  ND_RA_RESPONSE,  12, "lr_in_nd_ra_response") \
+PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_PRE,  13, "lr_in_ip_routing_pre")  \
+PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING,  14, "lr_in_ip_routing")  \
+PIPELINE_STAGE(ROUTER, IN,  IP_ROUTING_ECMP, 15, "lr_in_ip_routing_ecmp") \
+PIPELINE_STAGE(ROUTER, IN,  POLICY,  16, "lr_in_policy")  \
+PIPELINE_STAGE(ROUTER, IN,  POLICY_ECMP, 17, "lr_in_policy_ecmp") \
+PIPELINE_STAGE(ROUTER, IN,  ARP_RESOLVE, 18, "lr_in_arp_resolve") \
+PIPELINE_STAGE(ROUTER, IN,  CHK_PKT_LEN, 19, "lr_in_chk_pkt_len") \
+PIPELINE_STAGE(ROUTER, IN,  LARGER_PKTS, 20, "lr_in_larger_pkts") \
+PIPELINE_STAGE(ROUTER, IN,  GW_REDIRECT, 21, "lr_in_gw_redirect") \
+PIPELINE_STAGE(ROUTER, IN,  ARP_REQUEST, 22, "lr_in_arp_request") \
   \
 /* Logical router egress stages. */   \
 PIPELINE_STAGE(ROUTER, OUT, CHECK_DNAT_LOCAL,   0,   \
@@ -322,7 +323,7 @@ enum ovn_stage {
  * | |  (>= IP_INPUT)| E | INPORT_ETH_ADDR | X |   
 |
  * +-+---+ G |   (< IP_INPUT)  | X |   
 |
  * | R1  |   SRC_IPV4 for ARP-REQ| 0 | | R |   
 |
- * | |  (>= IP_INPUT)|   | | E | 
NEXT_HOP_IPV6 (>= DEFRAG ) |
+ * | |  (>= IP_INPUT)|   | | E |
NEXT_HOP_IPV6 (>= POST_DEFRAG ) |
  * +-+---+---+-+ G |   
 |
  * | R2  |UNUSED | X | | 0 |   
 |
  * | |   | R | |   |   
 |
@@ -10074,13 +10075,13 @@ add_ecmp_symmetric_reply_flows(struct hmap *lflows,
   route->is_src_route ? "dst" : "src",
   cidr);
 free(cidr);
-ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_DEFRAG, 100,
+

[ovs-dev] [PATCH ovn 2/2] northd: add flows to defrag IP traffic

[Lorenzo Bianconi]

Introduce a priority-100 flow in the ingress router defrag stage in
order to just perform IP traffic defragmentation without doing any dnat
operation. This change is necessary since the logical flow reported
below fails for IP fragmented traffic since L4 port info is available
just in the first fragment:

table=5 (lr_in_defrag   ), priority=110  , match=(ip && ip4.dst == 
172.16.0.111 && udp), action=(reg0 = 172.16.0.111; reg9[16..31] = udp.dst; 
ct_dnat;)

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2170885
Fixes: d91f359b7694 ("northd: Add VIP port to established flows in DNAT table 
for Load Balancers")
Signed-off-by: Lorenzo Bianconi 
---
 northd/northd.c |  4 
 northd/ovn-northd.8.xml | 15 +--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 97589e31d..27dc07c5a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -14238,6 +14238,10 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;");
 ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1", "next;");
 
+/* Add flow for defrag ip traffic. */
+ovn_lflow_add(lflows, od, S_ROUTER_IN_DEFRAG, 100,
+  "ip && ip.is_frag", "ct_next;");
+
 /* Ingress DNAT and DEFRAG Table (Priority 50/70).
  *
  * The defrag stage needs to have flows for ICMP in order to get
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 03eced0e4..ee3b05044 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3277,10 +3277,21 @@ icmp6 {
 
 
   This is to send packets to connection tracker for tracking and
-  defragmentation.  It contains a priority-0 flow that simply moves traffic
-  to the next table.
+  defragmentation.
 
 
+
+  
+A priority 100 flow is added with match ip 
+ip.is_frag and action ct_next;
+  
+
+  
+A priority 0 flow is added which matches on all packets and applies
+the action next;.
+  
+
+
 Ingress Table 6: POST_DEFRAG
 
 
-- 
2.39.2

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

[ovs-dev] [PATCH ovn 0/2] fix IP fragmented traffic hitting LB

[Lorenzo Bianconi]

Lorenzo Bianconi (2):
  northd: move defrag router pipeline stage forward
  northd: add flows to defrag IP traffic

 northd/northd.c |  50 ++--
 northd/ovn-northd.8.xml |  60 +++--
 tests/ovn-northd.at | 526 
 tests/ovn.at|  52 ++--
 tests/system-ovn.at |   8 +-
 5 files changed, 360 insertions(+), 336 deletions(-)

-- 
2.39.2

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

[ovs-dev] [PATCH v9] netdev-dpdk: add control plane protection support

[Robin Jarry]

Some control protocols are used to maintain link status between
forwarding engines (e.g. LACP). When the system is not sized properly,
the PMD threads may not be able to process all incoming traffic from the
configured Rx queues. When a signaling packet of such protocols is
dropped, it can cause link flapping, worsening the situation.

Use the RTE flow API to redirect these protocols into a dedicated Rx
queue. The assumption is made that the ratio between control protocol
traffic and user data traffic is very low and thus this dedicated Rx
queue will never get full. The RSS redirection table is re-programmed to
only use the other Rx queues. The RSS table size is stored in the
netdev_dpdk structure at port initialization to avoid requesting the
information again when changing the port configuration.

The additional Rx queue will be assigned a PMD core like any other Rx
queue. Polling that extra queue may introduce increased latency and
a slight performance penalty at the benefit of preventing link flapping.

This feature must be enabled per port on specific protocols via the
cp-protection option. This option takes a coma-separated list of
protocol names. It is only supported on ethernet ports. This feature is
experimental.

If the user has already configured multiple Rx queues on the port, an
additional one will be allocated for control plane packets. If the
hardware cannot satisfy the requested number of requested Rx queues, the
last Rx queue will be assigned for control plane. If only one Rx queue
is available, the cp-protection feature will be disabled. If the
hardware does not support the RTE flow matchers/actions, the feature
will be disabled.

It cannot be enabled when other_config:hw-offload=true as it may
conflict with the offloaded RTE flows. Similarly, if hw-offload is
enabled while some ports already have cp-protection enabled, RTE flow
offloading will be disabled on these ports.

Example use:

 ovs-vsctl add-bond br-phy bond0 phy0 phy1 -- \
   set interface phy0 type=dpdk options:dpdk-devargs=:ca:00.0 -- \
   set interface phy0 options:cp-protection=lacp -- \
   set interface phy1 type=dpdk options:dpdk-devargs=:ca:00.1 -- \
   set interface phy1 options:cp-protection=lacp

As a starting point, only one protocol is supported: LACP. Other
protocols can be added in the future. NIC compatibility should be
checked.

To validate that this works as intended, I used a traffic generator to
generate random traffic slightly above the machine capacity at line rate
on a two ports bond interface. OVS is configured to receive traffic on
two VLANs and pop/push them in a br-int bridge based on tags set on
patch ports.

   +--+
   | DUT  |
   |++|
   ||   br-int   || 
in_port=patch10,actions=mod_dl_src:$patch11,mod_dl_dst:$tgen1,output:patch11
   |||| 
in_port=patch11,actions=mod_dl_src:$patch10,mod_dl_dst:$tgen0,output:patch10
   || patch10patch11 ||
   |+---|---|+|
   ||   | |
   |+---|---|+|
   || patch00patch01 ||
   ||  tag:10tag:20  ||
   ||||
   ||   br-phy   || default flow, action=NORMAL
   ||||
   ||   bond0|| balance-slb, lacp=passive, lacp-time=fast
   ||phy0   phy1 ||
   |+--|-|---+|
   +---|-|+
   | |
   +---|-|+
   | port0  port1 | balance L3/L4, lacp=active, lacp-time=fast
   | lag  | mode trunk VLANs 10, 20
   |  |
   |switch|
   |  |
   |  vlan 10vlan 20  |  mode access
   |   port2  port3   |
   +-|--|-+
 |  |
   +-|--|-+
   |   tgen0  tgen1   |  Random traffic that is properly balanced
   |  |  across the bond ports in both directions.
   |  traffic generator   |
   +--+

Without cp-protection, the bond0 links are randomly switching to
"defaulted" when one of the LACP packets sent by the switch is dropped
because the RX queues are full and the PMD threads did not process them
fast enough. When that happens, all traffic must go through a single
link which causes above line rate traffic to be dropped.

 ~# ovs-appctl lacp/show-stats bond0
  bond0 statistics 
 member: phy0:
   TX PDUs: 347246
   RX PDUs: 14865
   RX Bad PDUs: 0
   RX Marker Request PDUs: 0
   Link Expired: 168
   Link Defaulted: 0
   Carrier Status Changed: 0
 member: phy1:
   TX PDUs: 347245
   RX PDUs: 14919
   RX Bad PDUs: 0
   RX Marker Request PDUs: 0
   Link Expired: 147
   Link Defaulted: 1
   Carrier Status Changed: 0

When cp-protection is enabled, no LACP packet is dropped and the bond
links remain enabled at all times, maximizing the throughput. Neither
the "Link Expired" nor the "Link Defaulted" counters are incremented
anymore.

This feature may be considered as 

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

[Simon Horman]

On Wed, Feb 22, 2023 at 06:33:50PM +0800, Faicker Mo wrote:
> It's not easy to add a fail test without the changed code.
> But I test it failed with the old code manually following these steps,
> 1. Apply this patch(with test in it)
> 2. Revert the changed code in netdev-offload-tc.c
> 3. Run the test
> 
> 
> Yes, the fail-test above sometimes may pass because of the env and the 
> chance.Maybe run the fail-test several times.

Thanks, I see this now.

* Without the C-code changes in this patch I saw the test fail 3 times,
  each time within 5 attempts.

* With the C-code change I am yet to see the test fail,
  so far I'm up to 80 attempts.

I do wonder if this warrants a Fixes tag.
And if so, if it should be:

Fixes: 262a07956fab ("netdev-tc-offloads: Delete ufid tc mapping in the right 
place")

That notwithstanding, I am happy with this patch.

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

Re: [ovs-dev] [PATCH ovn v2] dbctl: Fix a couple of memory leaks

[Simon Horman]

On Wed, Feb 22, 2023 at 01:56:51PM +0100, Ales Musil wrote:
> Nothing is being freed wherever we are calling
> ctl_fatal which is fine because the program is
> about to shutdown anyway however one of the
> leaks was caught by address sanitizer.
> Fix most of the leaks that are happening before
> call to ctl_fatal.
> 
> Direct leak of 64 byte(s) in 1 object(s) allocated from:
> #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o
> #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9
> #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12
> #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12
> #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23
> #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5
> #6 0x82c117 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:98:5
> #7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Indirect leak of 10 byte(s) in 1 object(s) allocated from:
> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) 
> (BuildId: 3a287416f70de43f52382f0336980c876f655f90)
> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> #5 0x82c041 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:91:5
> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Indirect leak of 8 byte(s) in 2 object(s) allocated from:
> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> #5 0x82c0b6 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:96:9
> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Signed-off-by: Ales Musil 
> ---
> v2: Address comments from Simon:
> - Rearrange the cleanup according to suggestion.
> - Allow free(NULL).

Minor nit not withstanding, this looks good to me.

Reviewed-by: Simon Horman 

> ---
>  utilities/ovn-dbctl.c | 76 ++-
>  1 file changed, 46 insertions(+), 30 deletions(-)

...

> @@ -1240,40 +1242,54 @@ dbctl_client(const struct ovn_dbctl_options 
> *dbctl_options,
>  
>  ctl_timeout_setup(timeout);
>  
> +char *cmd_result = NULL;
> +char *cmd_error = NULL;
>  struct jsonrpc *client;
> +int exit_status;
> +char *error_str;
> +
>  int error = unixctl_client_create(socket_name, );
>  if (error) {
> -ctl_fatal("%s: could not connect to %s daemon (%s); "
> -  "unset %s to avoid using daemon",
> -  socket_name, program_name, ovs_strerror(error),
> -  dbctl_options->daemon_env_var_name);
> +error_str = xasprintf("%s: could not connect to %s daemon (%s); "
> +  "unset %s to avoid using daemon",
> +  socket_name, program_name, ovs_strerror(error),
> +  dbctl_options->daemon_env_var_name);
> +goto log_error;
>  }
>  
> -char *cmd_result;
> -char *cmd_error;
> +

nit: there are now two blank lines here

>  error = unixctl_client_transact(client, "run",
>  args.n, args.names,
>  _result, _error);
>  if (error) {
> -ctl_fatal("%s: transaction error (%s)",
> -  socket_name, ovs_strerror(error));
> +error_str = xasprintf("%s: transaction error (%s)",
> +  socket_name, ovs_strerror(error));
> +goto log_error;
>  }
> -svec_destroy();
>  
> -int exit_status;
>  if (cmd_error) {
> -exit_status = EXIT_FAILURE;
>  fprintf(stderr, "%s: %s", program_name, cmd_error);
> -} else {
> -exit_status = EXIT_SUCCESS;
> -fputs(cmd_result, stdout);
> +goto error;
>  }
> +
> +exit_status = EXIT_SUCCESS;
> +fputs(cmd_result, stdout);
> +goto cleanup;
> +
> +log_error:
> +VLOG_ERR("%s", error_str);
> +ovs_error(0, "%s", error_str);
> +free(error_str);
> +
> +error:
> +exit_status = EXIT_FAILURE;
> +
> +cleanup:
>  free(cmd_result);
>  free(cmd_error);
>  jsonrpc_close(client);
> -for (int i = 0; i < argc; i++) {
> -free(argv[i]);
> -}
> -

Re: [ovs-dev] [PATCH v3 2/2] ipfix: make template and stats interval configurable

[Adrian Moreno]

On 2/20/23 19:00, Ilya Maximets wrote:

On 2/10/23 17:03, Adrián Moreno wrote:

From: Adrian Moreno 

Add options to the IPFIX table configure the interval to send statistics
and template information.

Signed-off-by: Adrian Moreno 

---
- v3:
- Removed unit tests which generate errors in Intel CI. Will submit in
  independent series.
- v2:
   - Fixed a potential race condition in unit test.
- v1:
   - Added unit test.
---
  NEWS |  2 ++
  ofproto/ofproto-dpif-ipfix.c | 38 ++--
  ofproto/ofproto.h|  9 +
  vswitchd/bridge.c| 17 
  vswitchd/vswitch.ovsschema   | 12 +++-
  vswitchd/vswitch.xml | 20 +++
  6 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 1a49cdffe..8ab609dfb 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
  {"name": "Open_vSwitch",
   "version": "8.3.1",


You're adding new columns.  Need to bump the version to 8.4.0.

The numbering scheme is described here:
   https://docs.openvswitch.org/en/latest/ref/ovsdb.7/#schemas



Thanks for the pointer, I did not know about that policy!

I'll resend the patch.

--
Adrián Moreno

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

Re: [ovs-dev] [PATCH v5 4/5] ovs-router: Introduce src option in ovs/route/add command.

[Simon Horman]

On Wed, Feb 22, 2023 at 07:29:51PM +0900, Nobuhiro MIKI wrote:
> When adding a route with ovs/route/add command, the source address
> in "ovs_router_entry" structure is always the FIRST address that the
> interface has. See "ovs_router_get_netdev_source_address"
> function for more information.
> 
> If an interface has multiple ipv4 and/or ipv6 addresses, there are use
> cases where the user wants to control the source address. This patch
> therefore addresses this issue by adding a src parameter.
> 
> Note that same constraints also exist when caching routes from
> Kernel FIB with Netlink, but are not dealt with in this patch.
> 
> Signed-off-by: Nobuhiro MIKI 

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

Re: [ovs-dev] [PATCH v5 3/5] ofproto: Fix mam page for tunnel related commands.

[Simon Horman]

On Wed, Feb 22, 2023 at 07:29:50PM +0900, Nobuhiro MIKI wrote:
> These commands already support both IPv4 and IPv6.
> 
> Signed-off-by: Nobuhiro MIKI 

Reviewed-by: Simon Horman 

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

[ovs-dev] [PATCH ovn v2] dbctl: Fix a couple of memory leaks

[Ales Musil]

Nothing is being freed wherever we are calling
ctl_fatal which is fine because the program is
about to shutdown anyway however one of the
leaks was caught by address sanitizer.
Fix most of the leaks that are happening before
call to ctl_fatal.

Direct leak of 64 byte(s) in 1 object(s) allocated from:
#0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o
#1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9
#2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12
#3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12
#4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23
#5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5
#6 0x82c117 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:98:5
#7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Indirect leak of 10 byte(s) in 1 object(s) allocated from:
#0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) 
(BuildId: 3a287416f70de43f52382f0336980c876f655f90)
#1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
#2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
#3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
#4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
#5 0x82c041 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:91:5
#6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Indirect leak of 8 byte(s) in 2 object(s) allocated from:
#0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
#1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
#2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
#3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
#4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
#5 0x82c0b6 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:96:9
#6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Signed-off-by: Ales Musil 
---
v2: Address comments from Simon:
- Rearrange the cleanup according to suggestion.
- Allow free(NULL).
---
 utilities/ovn-dbctl.c | 76 ++-
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
index f0ee0ba05..dbe6a745a 100644
--- a/utilities/ovn-dbctl.c
+++ b/utilities/ovn-dbctl.c
@@ -109,6 +109,15 @@ static void server_loop(const struct ovn_dbctl_options 
*dbctl_options,
 struct ovsdb_idl *idl, int argc, char *argv[]);
 static void ovn_dbctl_exit(int status);
 
+static void
+destroy_argv(int argc, char **argv)
+{
+for (int i = 0; i < argc; i++) {
+free(argv[i]);
+}
+free(argv);
+}
+
 int
 ovn_dbctl_main(int argc, char *argv[],
const struct ovn_dbctl_options *dbctl_options)
@@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
 char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
_options, _parsed_options);
 if (error_s) {
+destroy_argv(argc, argv_);
 ctl_fatal("%s", error_s);
 }
 
@@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
 bool daemon_mode = false;
 if (get_detach()) {
 if (argc != optind) {
+destroy_argv(argc, argv_);
 ctl_fatal("non-option arguments not supported with --detach "
   "(use --help for help)");
 }
@@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
 if (error) {
 ovsdb_idl_destroy(idl);
 idl = the_idl = NULL;
+destroy_argv(argc, argv_);
 
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
 ctl_fatal("%s", error);
 }
 
@@ -239,21 +247,15 @@ cleanup:
 }
 free(commands);
 if (error) {
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
+destroy_argv(argc, argv_);
 ctl_fatal("%s", error);
 }
 }
 
 ovsdb_idl_destroy(idl);
 idl = the_idl = NULL;
+destroy_argv(argc, argv_);
 
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
 exit(EXIT_SUCCESS);
 }
 
@@ -1240,40 +1242,54 @@ dbctl_client(const struct ovn_dbctl_options 
*dbctl_options,
 
 ctl_timeout_setup(timeout);
 
+char *cmd_result = NULL;
+char *cmd_error = NULL;
 struct jsonrpc *client;
+int exit_status;
+char *error_str;
+
 int error = unixctl_client_create(socket_name, );
 if (error) {
-ctl_fatal("%s: could not connect to %s 

Re: [ovs-dev] [PATCH ovn] dbctl: Fix a couple of memory leaks

[Ales Musil]

On Wed, Feb 22, 2023 at 1:38 PM Ilya Maximets  wrote:

>  +
>  +if (cmd_result) {
>  +free(cmd_result);
>  +}
>  +
>  +if (cmd_error) {
>  +free(cmd_error);
>  +}
> >>>
> >>> I don't think the cmd_result and cmd_error conditions are required
> >>> as free should do nothing when passed NULL.
> >>>
> >>
> >> Isn't free behavior for NULL platform dependent? I was under the
> impression
> >> that it is.
> >> But I might be mistaken.
> >
> > Maybe: not so secret fact is that I'm a Linux person.
>
> It's part of a C standard, so not platform dependent.
> And this is also part of a coding style. :)
>
> Best regards, Ilya Maximets.
>
>
Great, that's settled then.

Thanks,
Ales.

-- 

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 1/1] tc: Fix cleaning chains

[Simon Horman]

On Wed, Feb 22, 2023 at 12:30:17PM +0200, Roi Dayan via dev wrote:
> Sometimes there is a need to clean empty chains as done in
> delete_chains_from_netdev().  The cited commit doesn't remove
> the chain completely which cause adding ingress_block later to fail.
> This can be reproduced with adding bond as ovs port which makes ovs
> use ingress_block for it.
> While at it add the netdev name that fails to the log.
> 
> Fixes: e1e5eac5b016 ("tc: Add TCA_KIND flower to delete and get operation to 
> avoid rtnl_lock().")
> Signed-off-by: Roi Dayan 

I think this needs an ack from Eelco (CCed).

But it looks good to me.

Reviewed-by: Simon Horman 

> ---
>  lib/netdev-offload-tc.c | 7 ---
>  lib/tc.c| 4 +++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 4fb9d9f2127a..9dd0aa2e2a85 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -524,7 +524,7 @@ delete_chains_from_netdev(struct netdev *netdev, struct 
> tcf_id *id)
>   */
>  HMAP_FOR_EACH_POP (chain_node, node, ) {
>  id->chain = chain_node->chain;
> -tc_del_flower_filter(id);
> +tc_del_filter(id, NULL);
>  free(chain_node);
>  }
>  }
> @@ -2860,8 +2860,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>  error = tc_add_del_qdisc(ifindex, true, block_id, hook);
>  
>  if (error && error != EEXIST) {
> -VLOG_INFO("failed adding ingress qdisc required for offloading: %s",
> -  ovs_strerror(error));
> +VLOG_INFO("failed adding ingress qdisc required for offloading "
> +  "on %s: %s",
> +  netdev_get_name(netdev), ovs_strerror(error));
>  return error;
>  }
>  
> diff --git a/lib/tc.c b/lib/tc.c
> index 4c07e22162e7..5c32c6f971da 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2354,7 +2354,9 @@ tc_del_filter(struct tcf_id *id, const char *kind)
>  struct ofpbuf request;
>  
>  request_from_tcf_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, );
> -nl_msg_put_string(, TCA_KIND, kind);
> +if (kind) {
> +nl_msg_put_string(, TCA_KIND, kind);
> +}
>  return tc_transact(, NULL);
>  }
>  
> -- 
> 2.38.0
> 
> ___
> 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 ovn] dbctl: Fix a couple of memory leaks

[Ilya Maximets]

 +
 +if (cmd_result) {
 +free(cmd_result);
 +}
 +
 +if (cmd_error) {
 +free(cmd_error);
 +}
>>>
>>> I don't think the cmd_result and cmd_error conditions are required
>>> as free should do nothing when passed NULL.
>>>
>>
>> Isn't free behavior for NULL platform dependent? I was under the impression
>> that it is.
>> But I might be mistaken.
> 
> Maybe: not so secret fact is that I'm a Linux person.

It's part of a C standard, so not platform dependent.
And this is also part of a coding style. :)

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

Re: [ovs-dev] [PATCH ovn] dbctl: Fix a couple of memory leaks

[Simon Horman]

On Wed, Feb 22, 2023 at 11:24:04AM +0100, Ales Musil wrote:
> On Wed, Feb 22, 2023 at 10:59 AM Simon Horman 
> wrote:

...

Hi Ales,

> Hi Simon,
> thank you for your review.

you are welcome.

> > > ---
> > >  utilities/ovn-dbctl.c | 76 +++
> > >  1 file changed, 48 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
> > > index f0ee0ba05..f96cab64e 100644
> > > --- a/utilities/ovn-dbctl.c
> > > +++ b/utilities/ovn-dbctl.c
> > > @@ -109,6 +109,15 @@ static void server_loop(const struct
> > ovn_dbctl_options *dbctl_options,
> > >  struct ovsdb_idl *idl, int argc, char *argv[]);
> > >  static void ovn_dbctl_exit(int status);
> > >
> > > +static void
> > > +destroy_argv(int argc, char **argv)
> > > +{
> > > +for (int i = 0; i < argc; i++) {
> > > +free(argv[i]);
> > > +}
> > > +free(argv);
> > > +}
> > > +
> >
> > This destroy_argv seems to be the dual of ovs_cmdl_env_parse_all(),
> > does it belong in lib/command-line.c ?
> >
> 
> It might fit more into command-line.c however getting something into ovs
> and then
> reflect that in ovn is way longer process.

Fair enough.
I agree it makes sense to start with an OVN fix.

> > Also, do all other users of ovs_cmdl_env_parse_all() - in both OVS and OVN
> > -
> > unwind correctly on error?
> >
> 
> I can see only single usage and that's here. So before this patch the
> answer would be no.

Great, thanks for checking.

> > >  int
> > >  ovn_dbctl_main(int argc, char *argv[],
> > > const struct ovn_dbctl_options *dbctl_options)
> > > @@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
> > >  char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
> > > _options,
> > _parsed_options);
> > >  if (error_s) {
> > > +destroy_argv(argc, argv_);
> > >  ctl_fatal("%s", error_s);
> > >  }
> > >
> > > @@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
> > >  bool daemon_mode = false;
> > >  if (get_detach()) {
> > >  if (argc != optind) {
> > > +destroy_argv(argc, argv_);
> > >  ctl_fatal("non-option arguments not supported with --detach
> > "
> > >"(use --help for help)");
> > >  }
> > > @@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
> > >  if (error) {
> > >  ovsdb_idl_destroy(idl);
> > >  idl = the_idl = NULL;
> > > +destroy_argv(argc, argv_);
> > >
> > > -for (int i = 0; i < argc; i++) {
> > > -free(argv_[i]);
> > > -}
> > > -free(argv_);
> > >  ctl_fatal("%s", error);
> > >  }
> > >
> > > @@ -239,21 +247,15 @@ cleanup:
> > >  }
> > >  free(commands);
> > >  if (error) {
> > > -for (int i = 0; i < argc; i++) {
> > > -free(argv_[i]);
> > > -}
> > > -free(argv_);
> > > +destroy_argv(argc, argv_);
> > >  ctl_fatal("%s", error);
> > >  }
> > >  }
> > >
> > >  ovsdb_idl_destroy(idl);
> > >  idl = the_idl = NULL;
> > > +destroy_argv(argc, argv_);
> > >
> > > -for (int i = 0; i < argc; i++) {
> > > -free(argv_[i]);
> > > -}
> > > -free(argv_);
> > >  exit(EXIT_SUCCESS);
> > >  }
> > >
> > > @@ -1240,27 +1242,31 @@ dbctl_client(const struct ovn_dbctl_options
> > *dbctl_options,
> > >
> > >  ctl_timeout_setup(timeout);
> > >
> > > +char *err = NULL;
> >
> > perhaps error_str is a nicer name for this?
> >
> > > +char *cmd_error = NULL;
> > > +char *cmd_result = NULL;
> > > +int exit_status = EXIT_SUCCESS;
> > >  struct jsonrpc *client;
> >
> > I'm not sure if reverse xmas tree - longest line to shortest - is
> > a thing for local variable declarations in ovn.
> >
> 
> I don't think it's strictly held, but it shouldn't hurt.

Yeah, that's what I was thinking too.

> > > +
> > >  int error = unixctl_client_create(socket_name, );
> > >  if (error) {
> > > -ctl_fatal("%s: could not connect to %s daemon (%s); "
> > > -  "unset %s to avoid using daemon",
> > > -  socket_name, program_name, ovs_strerror(error),
> > > -  dbctl_options->daemon_env_var_name);
> > > +err = xasprintf("%s: could not connect to %s daemon (%s); "
> > > +"unset %s to avoid using daemon",
> > > +socket_name, program_name, ovs_strerror(error),
> > > +dbctl_options->daemon_env_var_name);
> > > +goto cleanup;
> > >  }
> > >
> > > -char *cmd_result;
> > > -char *cmd_error;
> > > +
> > >  error = unixctl_client_transact(client, "run",
> > >  args.n, args.names,
> > >  _result, 

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

[Faicker Mo]

It's not easy to add a fail test without the changed code.
But I test it failed with the old code manually following these steps,
1. Apply this patch(with test in it)
2. Revert the changed code in netdev-offload-tc.c
3. Run the test


Yes, the fail-test above sometimes may pass because of the env and the 
chance.Maybe run the fail-test several times.






From: Simon Horman 
Date: 2023-02-22 17:07:18
To:  Faicker Mo 
Cc:  d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if device 
not exist>On Wed, Feb 22, 2023 at 10:03:07AM +0800, Faicker Mo wrote:
>> Sorry. 
>> The commit message and code are not changed. 
>> Resended when I met a bug of intel-ovs-compilation test fail and add version 
>> descriptions.
>
>Thanks, I think I understand now.
>But please don't top-post on this mailing list.
>
>And could you please look at my comment regarding
>the test you have added in this patch.
>
>Thanks!
>
>> From: Simon Horman 
>> Date: 2023-02-21 23:09:05
>> To:  Faicker Mo 
>> Cc:  d...@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if 
>> device not exist>On Wed, Feb 01, 2023 at 10:49:22AM +0800, 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 
>> >> ---
>> >> v2:
>> >> - Add tc offload test case
>> >> v3:
>> >> - No change
>> >> v4:
>> >> - No change
>> >> v5:
>> >> - No change
>> >> v6:
>> >> - No change
>> >
>> >I am confused.
>> >Why are there 4 versions (v3 - v6) with no change?
>> >What does that mean?
>
>...
>
>> >> diff --git a/tests/system-offloads-traffic.at 
>> >> b/tests/system-offloads-traffic.at
>> >> index 16a4c1a00..15ea549a6 100644
>> >> --- a/tests/system-offloads-traffic.at
>> >> +++ b/tests/system-offloads-traffic.at
>> >
>> >The test seems to pass both with and without the change to
>> >del_filter_and_ufid_mapping(). I think it would be better to construct a
>> >test that fails without the code change and succeeds with it.
>> >
>> >I ran:
>> >
>> >TESTSUITEFLAGS="-k ufid" make check-offloads
>
>
>This comment, here.
>
>...




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

[ovs-dev] [PATCH 1/1] tc: Fix cleaning chains

[Roi Dayan via dev]

Sometimes there is a need to clean empty chains as done in
delete_chains_from_netdev().  The cited commit doesn't remove
the chain completely which cause adding ingress_block later to fail.
This can be reproduced with adding bond as ovs port which makes ovs
use ingress_block for it.
While at it add the netdev name that fails to the log.

Fixes: e1e5eac5b016 ("tc: Add TCA_KIND flower to delete and get operation to 
avoid rtnl_lock().")
Signed-off-by: Roi Dayan 
---
 lib/netdev-offload-tc.c | 7 ---
 lib/tc.c| 4 +++-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 4fb9d9f2127a..9dd0aa2e2a85 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -524,7 +524,7 @@ delete_chains_from_netdev(struct netdev *netdev, struct 
tcf_id *id)
  */
 HMAP_FOR_EACH_POP (chain_node, node, ) {
 id->chain = chain_node->chain;
-tc_del_flower_filter(id);
+tc_del_filter(id, NULL);
 free(chain_node);
 }
 }
@@ -2860,8 +2860,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
 error = tc_add_del_qdisc(ifindex, true, block_id, hook);
 
 if (error && error != EEXIST) {
-VLOG_INFO("failed adding ingress qdisc required for offloading: %s",
-  ovs_strerror(error));
+VLOG_INFO("failed adding ingress qdisc required for offloading "
+  "on %s: %s",
+  netdev_get_name(netdev), ovs_strerror(error));
 return error;
 }
 
diff --git a/lib/tc.c b/lib/tc.c
index 4c07e22162e7..5c32c6f971da 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -2354,7 +2354,9 @@ tc_del_filter(struct tcf_id *id, const char *kind)
 struct ofpbuf request;
 
 request_from_tcf_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, );
-nl_msg_put_string(, TCA_KIND, kind);
+if (kind) {
+nl_msg_put_string(, TCA_KIND, kind);
+}
 return tc_transact(, NULL);
 }
 
-- 
2.38.0

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

[ovs-dev] [PATCH v5 5/5] route-table: Retrieving the preferred source address from Netlink.

[Nobuhiro MIKI]

We can use the "ip route add ... src ..." command to set the preferred
source address for each entry in the kernel FIB. OVS has a mechanism to
cache the FIB, but the preferred source address is ignored and
calculated with its own logic. This patch resolves the difference
between kernel FIB and OVS route table cache by retrieving the
RTA_PREFSRC attribute of Netlink messages.

Reviewed-by: Simon Horman 
Signed-off-by: Nobuhiro MIKI 
---
 lib/ovs-router.c  |  6 +++---
 lib/ovs-router.h  |  3 ++-
 lib/route-table.c | 16 +++-
 tests/system-route.at | 39 +++
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index dca1a406cb44..71e713068310 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -319,13 +319,13 @@ ovs_router_insert__(uint32_t mark, uint8_t priority, bool 
local,
 
 void
 ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
-  bool local, const char output_bridge[], 
-  const struct in6_addr *gw)
+  bool local, const char output_bridge[],
+  const struct in6_addr *gw, const struct in6_addr *prefsrc)
 {
 if (use_system_routing_table) {
 uint8_t priority = local ? plen + 64 : plen;
 ovs_router_insert__(mark, priority, local, ip_dst, plen,
-output_bridge, gw, _any);
+output_bridge, gw, prefsrc);
 }
 }
 
diff --git a/lib/ovs-router.h b/lib/ovs-router.h
index d8ce3c00ded5..eb4ff85d9e63 100644
--- a/lib/ovs-router.h
+++ b/lib/ovs-router.h
@@ -32,7 +32,8 @@ bool ovs_router_lookup(uint32_t mark, const struct in6_addr 
*ip_dst,
 void ovs_router_init(void);
 void ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst,
uint8_t plen, bool local,
-   const char output_bridge[], const struct in6_addr *gw);
+   const char output_bridge[], const struct in6_addr *gw,
+   const struct in6_addr *prefsrc);
 void ovs_router_flush(void);
 
 void ovs_router_disable_system_routing_table(void);
diff --git a/lib/route-table.c b/lib/route-table.c
index ac82cf262f86..9927dcc1854b 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -51,6 +51,7 @@ struct route_data {
 
 /* Extracted from Netlink attributes. */
 struct in6_addr rta_dst; /* 0 if missing. */
+struct in6_addr rta_prefsrc; /* 0 if missing. */
 struct in6_addr rta_gw;
 char ifname[IFNAMSIZ]; /* Interface name. */
 uint32_t mark;
@@ -201,6 +202,7 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 [RTA_OIF] = { .type = NL_A_U32, .optional = true },
 [RTA_GATEWAY] = { .type = NL_A_U32, .optional = true },
 [RTA_MARK] = { .type = NL_A_U32, .optional = true },
+[RTA_PREFSRC] = { .type = NL_A_U32, .optional = true },
 };
 
 static const struct nl_policy policy6[] = {
@@ -208,6 +210,7 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 [RTA_OIF] = { .type = NL_A_U32, .optional = true },
 [RTA_MARK] = { .type = NL_A_U32, .optional = true },
 [RTA_GATEWAY] = { .type = NL_A_IPV6, .optional = true },
+[RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true },
 };
 
 struct nlattr *attrs[ARRAY_SIZE(policy)];
@@ -274,6 +277,16 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 } else if (ipv4) {
 in6_addr_set_mapped_ipv4(>rd.rta_dst, 0);
 }
+if (attrs[RTA_PREFSRC]) {
+if (ipv4) {
+ovs_be32 prefsrc;
+prefsrc = nl_attr_get_be32(attrs[RTA_PREFSRC]);
+in6_addr_set_mapped_ipv4(>rd.rta_prefsrc, prefsrc);
+} else {
+change->rd.rta_prefsrc =
+nl_attr_get_in6_addr(attrs[RTA_PREFSRC]);
+}
+}
 if (attrs[RTA_GATEWAY]) {
 if (ipv4) {
 ovs_be32 gw;
@@ -309,7 +322,8 @@ route_table_handle_msg(const struct route_table_msg *change)
 const struct route_data *rd = >rd;
 
 ovs_router_insert(rd->mark, >rta_dst, rd->rtm_dst_len,
-  rd->local, rd->ifname, >rta_gw);
+  rd->local, rd->ifname, >rta_gw,
+  >rta_prefsrc);
 }
 }
 
diff --git a/tests/system-route.at b/tests/system-route.at
index 270956d13f6f..114aaebc77f3 100644
--- a/tests/system-route.at
+++ b/tests/system-route.at
@@ -25,3 +25,42 @@ OVS_WAIT_UNTIL([test `ovs-appctl ovs/route/show | grep -c 
'p1-route'` -eq 0 ])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ovs-route - add system route with src - ipv4])
+AT_KEYWORDS([route])
+OVS_TRAFFIC_VSWITCHD_START()
+AT_CHECK([ip link set br0 up])
+
+AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout])
+AT_CHECK([ip addr add 192.168.9.3/24 dev br0], [0], 

[ovs-dev] [PATCH v5 2/5] ovs-router: Cleanup parser for ovs/route/add command.

[Nobuhiro MIKI]

This patch cleans up the parser to accept pkt_mark and gw in any order.

pkt_mark and gw are normally expected to be specified exactly once.
However, as with other tools, if specified multiple times, the last
specification is used. Also, pkt_mark and gw have separate prefix
strings so they can be parsed in any order.

Reviewed-by: Simon Horman 
Signed-off-by: Nobuhiro MIKI 
---
 lib/ovs-router.c| 50 -
 tests/ovs-router.at | 27 
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 5d0fbd503e9e..6c5faf46ea15 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -345,41 +345,45 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
 struct in6_addr ip6;
 uint32_t mark = 0;
 unsigned int plen;
+ovs_be32 gw = 0;
+bool is_ipv6;
 ovs_be32 ip;
 int err;
+int i;
 
 if (scan_ipv4_route(argv[1], , )) {
-ovs_be32 gw = 0;
-
-if (argc > 3) {
-if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, ) &&
-!ip_parse(argv[3], )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark or 
gateway");
-return;
-}
-}
 in6_addr_set_mapped_ipv4(, ip);
-if (gw) {
-in6_addr_set_mapped_ipv4(, gw);
-}
 plen += 96;
+is_ipv6 = false;
 } else if (scan_ipv6_route(argv[1], , )) {
-if (argc > 3) {
-if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, ) &&
-!ipv6_parse(argv[3], )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6 
gateway");
-return;
-}
-}
+is_ipv6 = true;
 } else {
 unixctl_command_reply_error(conn, "Invalid parameters");
 return;
 }
-if (argc > 4) {
-if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark");
-return;
+
+/* Parse optional parameters. */
+for (i = 3; i < argc; i++) {
+if (ovs_scan(argv[i], "pkt_mark=%"SCNi32, )) {
+continue;
 }
+
+if (is_ipv6) {
+if (ipv6_parse(argv[i], )) {
+continue;
+}
+} else {
+if (ip_parse(argv[i], )) {
+continue;
+}
+}
+
+unixctl_command_reply_error(conn, "Invalid parameters");
+return;
+}
+
+if (gw) {
+in6_addr_set_mapped_ipv4(, gw);
 }
 
 err = ovs_router_insert__(mark, plen + 32, false, , plen, argv[2], 
);
diff --git a/tests/ovs-router.at b/tests/ovs-router.at
index 6dacc2954bc6..96fb6e188eef 100644
--- a/tests/ovs-router.at
+++ b/tests/ovs-router.at
@@ -1,14 +1,33 @@
 AT_BANNER([ovs-router])
 
-AT_SETUP([appctl - route/add with gateway])
+AT_SETUP([appctl - route/add with gateway and pkt_mark])
 AT_KEYWORDS([ovs_router])
-OVS_VSWITCHD_START([add-port br0 p2 -- set Interface p2 type=gre \
-options:local_ip=2.2.2.2 options:remote_ip=1.1.1.1 \
--- add-port br0 p1  -- set interface p1 type=dummy])
+OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
 AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 2.2.2.2/24], [0], [OK
 ])
+AT_CHECK([ovs-appctl ovs/route/add 2.2.2.3/32 br0 pkt_mark=1], [0], [OK
+])
 AT_CHECK([ovs-appctl ovs/route/add 1.1.1.0/24 br0 2.2.2.10], [0], [OK
 ])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.2.0/24 br0 2.2.2.10 pkt_mark=2], [0], 
[OK
+])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.3.0/24 br0 pkt_mark=3], [2], [], [dnl
+Error while inserting route.
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.foo.bar/24 br0 2.2.2.10], [2], [], [dnl
+Invalid parameters
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/add 2.2.2.4/24 br0 pkt_mark=baz], [2], [], [dnl
+Invalid parameters
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [dnl
+User: 1.1.1.0/24 dev br0 GW 2.2.2.10 SRC 2.2.2.2
+User: 1.1.2.0/24 MARK 2 dev br0 GW 2.2.2.10 SRC 2.2.2.2
+User: 2.2.2.3/32 MARK 1 dev br0 SRC 2.2.2.2
+])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
-- 
2.31.1

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

[ovs-dev] [PATCH v5 4/5] ovs-router: Introduce src option in ovs/route/add command.

[Nobuhiro MIKI]

When adding a route with ovs/route/add command, the source address
in "ovs_router_entry" structure is always the FIRST address that the
interface has. See "ovs_router_get_netdev_source_address"
function for more information.

If an interface has multiple ipv4 and/or ipv6 addresses, there are use
cases where the user wants to control the source address. This patch
therefore addresses this issue by adding a src parameter.

Note that same constraints also exist when caching routes from
Kernel FIB with Netlink, but are not dealt with in this patch.

Signed-off-by: Nobuhiro MIKI 
---
 NEWS|  3 ++
 lib/ovs-router.c| 83 +
 ofproto/ofproto-tnl-unixctl.man |  5 +-
 tests/ovs-router.at | 78 +++
 4 files changed, 158 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 85b34962145e..d062ca4fac1a 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,9 @@ Post-v3.1.0
in order to create OVSDB sockets with access mode of 0770.
- QoS:
  * Added new configuration option 'jitter' for a linux-netem QoS type.
+   - ovs-appctl:
+ * Add support for selecting the source address with the
+   “ovs-appctl ovs/route/add" command.
 
 
 v3.1.0 - 16 Feb 2023
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 6c5faf46ea15..dca1a406cb44 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -164,6 +164,46 @@ static void rt_init_match(struct match *match, uint32_t 
mark,
 match->flow.pkt_mark = mark;
 }
 
+static int
+verify_prefsrc(const struct in6_addr *ip6_dst,
+   const char output_bridge[],
+   struct in6_addr *prefsrc)
+{
+struct in6_addr *mask, *addr6;
+struct netdev *dev;
+int err, n_in6, i;
+
+err = netdev_open(output_bridge, NULL, );
+if (err) {
+return err;
+}
+
+err = netdev_get_addr_list(dev, , , _in6);
+if (err) {
+goto out;
+}
+
+for (i = 0; i < n_in6; i++) {
+struct in6_addr a1, a2;
+a1 = ipv6_addr_bitand(ip6_dst, [i]);
+a2 = ipv6_addr_bitand(prefsrc, [i]);
+
+/* Check that the intarface has "prefsrc" and
+ * it is same broadcast domain with "ip6_dst". */
+if (IN6_ARE_ADDR_EQUAL(prefsrc, [i]) &&
+IN6_ARE_ADDR_EQUAL(, )) {
+goto out;
+}
+}
+err = ENOENT;
+
+out:
+free(addr6);
+free(mask);
+netdev_close(dev);
+return err;
+}
+
 int
 ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst,
  const char output_bridge[],
@@ -217,8 +257,12 @@ static int
 ovs_router_insert__(uint32_t mark, uint8_t priority, bool local,
 const struct in6_addr *ip6_dst,
 uint8_t plen, const char output_bridge[],
-const struct in6_addr *gw)
+const struct in6_addr *gw,
+const struct in6_addr *ip6_src)
 {
+int (*f)(const struct in6_addr *ip6_dst,
+ const char output_bridge[],
+ struct in6_addr *prefsrc);
 const struct cls_rule *cr;
 struct ovs_router_entry *p;
 struct match match;
@@ -236,11 +280,17 @@ ovs_router_insert__(uint32_t mark, uint8_t priority, bool 
local,
 p->plen = plen;
 p->local = local;
 p->priority = priority;
-err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
-   >src_addr);
+
+if (ipv6_addr_is_set(ip6_src)) {
+p->src_addr = *ip6_src;
+f = verify_prefsrc;
+} else {
+f = ovs_router_get_netdev_source_address;
+}
+
+err = f(ip6_dst, output_bridge, >src_addr);
 if (err && ipv6_addr_is_set(gw)) {
-err = ovs_router_get_netdev_source_address(gw, output_bridge,
-   >src_addr);
+err = f(gw, output_bridge, >src_addr);
 }
 if (err) {
 struct ds ds = DS_EMPTY_INITIALIZER;
@@ -274,7 +324,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr 
*ip_dst, uint8_t plen,
 {
 if (use_system_routing_table) {
 uint8_t priority = local ? plen + 64 : plen;
-ovs_router_insert__(mark, priority, local, ip_dst, plen, 
output_bridge, gw);
+ovs_router_insert__(mark, priority, local, ip_dst, plen,
+output_bridge, gw, _any);
 }
 }
 
@@ -341,10 +392,13 @@ static void
 ovs_router_add(struct unixctl_conn *conn, int argc,
   const char *argv[], void *aux OVS_UNUSED)
 {
+struct in6_addr src6 = in6addr_any;
 struct in6_addr gw6 = in6addr_any;
+char src6_s[IPV6_SCAN_LEN + 1];
 struct in6_addr ip6;
 uint32_t mark = 0;
 unsigned int plen;
+ovs_be32 src = 0;
 ovs_be32 gw = 0;
 bool is_ipv6;
 ovs_be32 ip;
@@ -369,10 +423,17 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
 }
 
 if (is_ipv6) {
+if (ovs_scan(argv[i], 

[ovs-dev] [PATCH v5 1/5] netdev-dummy: Support multiple IP addresses.

[Nobuhiro MIKI]

This is useful in test cases where multiple IPv4/IPv6 addresses
are assigned together.

Reviewed-by: Simon Horman 
Signed-off-by: Nobuhiro MIKI 
---
 lib/netdev-dummy.c | 67 +-
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 5d59c9c0312b..7d3d2aa23ba1 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -136,8 +136,7 @@ struct netdev_dummy {
 
 struct pcap_file *tx_pcap, *rxq_pcap OVS_GUARDED;
 
-struct in_addr address, netmask;
-struct in6_addr ipv6, ipv6_mask;
+struct ovs_list addrs;
 struct ovs_list rxes OVS_GUARDED; /* List of child "netdev_rxq_dummy"s. */
 
 struct hmap offloaded_flows OVS_GUARDED;
@@ -161,6 +160,12 @@ struct netdev_rxq_dummy {
 struct seq *seq;/* Reports newly queued packets. */
 };
 
+struct netdev_addr_dummy {
+struct in6_addr address;
+struct in6_addr netmask;
+struct ovs_list node;   /* In netdev_dummy's "addrs" list. */
+};
+
 static unixctl_cb_func netdev_dummy_set_admin_state;
 static int netdev_dummy_construct(struct netdev *);
 static void netdev_dummy_queue_packet(struct netdev_dummy *,
@@ -169,6 +174,7 @@ static void netdev_dummy_queue_packet(struct netdev_dummy *,
 static void dummy_packet_stream_close(struct dummy_packet_stream *);
 
 static void pkt_list_delete(struct ovs_list *);
+static void addr_list_delete(struct ovs_list *);
 
 static bool
 is_dummy_class(const struct netdev_class *class)
@@ -720,6 +726,7 @@ netdev_dummy_construct(struct netdev *netdev_)
 dummy_packet_conn_init(>conn);
 
 ovs_list_init(>rxes);
+ovs_list_init(>addrs);
 hmap_init(>offloaded_flows);
 ovs_mutex_unlock(>mutex);
 
@@ -756,6 +763,7 @@ netdev_dummy_destruct(struct netdev *netdev_)
 free(off_flow);
 }
 hmap_destroy(>offloaded_flows);
+addr_list_delete(>addrs);
 
 ovs_mutex_unlock(>mutex);
 ovs_mutex_destroy(>mutex);
@@ -803,32 +811,26 @@ netdev_dummy_get_addr_list(const struct netdev *netdev_, 
struct in6_addr **paddr
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
 int cnt = 0, i = 0, err = 0;
 struct in6_addr *addr, *mask;
+struct netdev_addr_dummy *addr_dummy;
 
 ovs_mutex_lock(>mutex);
-if (netdev->address.s_addr != INADDR_ANY) {
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
 cnt++;
 }
 
-if (ipv6_addr_is_set(>ipv6)) {
-cnt++;
-}
 if (!cnt) {
 err = EADDRNOTAVAIL;
 goto out;
 }
 addr = xmalloc(sizeof *addr * cnt);
 mask = xmalloc(sizeof *mask * cnt);
-if (netdev->address.s_addr != INADDR_ANY) {
-in6_addr_set_mapped_ipv4([i], netdev->address.s_addr);
-in6_addr_set_mapped_ipv4([i], netdev->netmask.s_addr);
+
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
+memcpy([i], _dummy->address, sizeof *addr);
+memcpy([i], _dummy->netmask, sizeof *mask);
 i++;
 }
 
-if (ipv6_addr_is_set(>ipv6)) {
-memcpy([i], >ipv6, sizeof *addr);
-memcpy([i], >ipv6_mask, sizeof *mask);
-i++;
-}
 if (paddr) {
 *paddr = addr;
 *pmask = mask;
@@ -844,14 +846,16 @@ out:
 }
 
 static int
-netdev_dummy_set_in4(struct netdev *netdev_, struct in_addr address,
+netdev_dummy_add_in4(struct netdev *netdev_, struct in_addr address,
  struct in_addr netmask)
 {
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+struct netdev_addr_dummy *addr_dummy = xmalloc(sizeof *addr_dummy);
 
 ovs_mutex_lock(>mutex);
-netdev->address = address;
-netdev->netmask = netmask;
+in6_addr_set_mapped_ipv4(_dummy->address, address.s_addr);
+in6_addr_set_mapped_ipv4(_dummy->netmask, netmask.s_addr);
+ovs_list_push_back(>addrs, _dummy->node);
 netdev_change_seq_changed(netdev_);
 ovs_mutex_unlock(>mutex);
 
@@ -859,14 +863,16 @@ netdev_dummy_set_in4(struct netdev *netdev_, struct 
in_addr address,
 }
 
 static int
-netdev_dummy_set_in6(struct netdev *netdev_, struct in6_addr *in6,
+netdev_dummy_add_in6(struct netdev *netdev_, struct in6_addr *in6,
  struct in6_addr *mask)
 {
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+struct netdev_addr_dummy *addr_dummy = xmalloc(sizeof *addr_dummy);
 
 ovs_mutex_lock(>mutex);
-netdev->ipv6 = *in6;
-netdev->ipv6_mask = *mask;
+addr_dummy->address = *in6;
+addr_dummy->netmask = *mask;
+ovs_list_push_back(>addrs, _dummy->node);
 netdev_change_seq_changed(netdev_);
 ovs_mutex_unlock(>mutex);
 
@@ -1178,7 +1184,10 @@ netdev_dummy_send(struct netdev *netdev, int qid,
 dummy_packet_conn_send(>conn, buffer, size);
 
 /* Reply to ARP requests for 'dev''s assigned IP address. */
-if (dev->address.s_addr) {
+struct netdev_addr_dummy *addr_dummy;
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
+ovs_be32 address = 

[ovs-dev] [PATCH v5 0/5] Add support for preffered src address in ovs-router

[Nobuhiro MIKI]

With this series, the preferred source address in ovs-router is obtained
from both ovs/route/add command and kernel FIB.

v5:
- Add patch to fix man page
v4:
- Add cleanup patch for ovs/route/add
- Remove unrelated code
v3:
- Fix netdev-dummy to support multiple IP addresses
- Add validation and unit tests for ovs/route/add
- Refactor parsing for optional parameters in ovs/route/add command
v2:
- Add NEWS

Nobuhiro MIKI (5):
  netdev-dummy: Support multiple IP addresses.
  ovs-router: Cleanup parser for ovs/route/add command.
  ofproto: Fix mam page for tunnel related commands.
  ovs-router: Introduce src option in ovs/route/add command.
  route-table: Retrieving the preferred source address from Netlink.

 NEWS|   3 +
 lib/netdev-dummy.c  |  67 ++--
 lib/ovs-router.c| 137 
 lib/ovs-router.h|   3 +-
 lib/route-table.c   |  16 +++-
 ofproto/ofproto-tnl-unixctl.man |   9 ++-
 tests/ovs-router.at | 105 +++-
 tests/system-route.at   |  39 +
 8 files changed, 311 insertions(+), 68 deletions(-)

-- 
2.31.1

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

[ovs-dev] [PATCH v5 3/5] ofproto: Fix mam page for tunnel related commands.

[Nobuhiro MIKI]

These commands already support both IPv4 and IPv6.

Signed-off-by: Nobuhiro MIKI 
---
 ofproto/ofproto-tnl-unixctl.man | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ofproto/ofproto-tnl-unixctl.man b/ofproto/ofproto-tnl-unixctl.man
index 13a465119a90..0c0a392820fb 100644
--- a/ofproto/ofproto-tnl-unixctl.man
+++ b/ofproto/ofproto-tnl-unixctl.man
@@ -1,8 +1,8 @@
 .SS "OPENVSWITCH TUNNELING COMMANDS"
 These commands query and modify OVS tunnel components.
 .
-.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
-Adds ipv4_address/plen route to vswitchd routing table. output_bridge
+.IP "\fBovs/route/add ip/plen output_bridge [GW]\fR"
+Adds ip/plen route to vswitchd routing table. output_bridge
 needs to be OVS bridge name.  This command is useful if OVS cached
 routes does not look right.
 .
@@ -10,8 +10,8 @@ routes does not look right.
 Print all routes in OVS routing table, This includes routes cached
 from system routing table and user configured routes.
 .
-.IP "\fBovs/route/del ipv4_address/plen\fR"
-Delete ipv4_address/plen route from OVS routing table.
+.IP "\fBovs/route/del ip/plen\fR"
+Delete ip/plen route from OVS routing table.
 .
 .IP "\fBtnl/neigh/show\fR"
 .IP "\fBtnl/arp/show\fR"
-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn] dbctl: Fix a couple of memory leaks

[Ales Musil]

On Wed, Feb 22, 2023 at 10:59 AM Simon Horman 
wrote:

> On Wed, Feb 22, 2023 at 09:02:10AM +0100, Ales Musil wrote:
> > Nothing is being freed wherever we are calling
> > ctl_fatal which is fine because the program is
> > about to shutdown anyway however one of the
> > leaks was caught by address sanitizer.
> > Fix most of the leaks that are happening before
> > call to ctl_fatal.
> >
> > Direct leak of 64 byte(s) in 1 object(s) allocated from:
> > #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o
> > #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9
> > #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12
> > #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12
> > #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23
> > #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5
> > #6 0x82c117 in ovs_cmdl_env_parse_all
> /workspace/ovn/ovs/lib/command-line.c:98:5
> > #7 0x5ad70d in ovn_dbctl_main
> /workspace/ovn/utilities/ovn-dbctl.c:132:20
> > #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> >
> > Indirect leak of 10 byte(s) in 1 object(s) allocated from:
> > #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
> (BuildId: 3a287416f70de43f52382f0336980c876f655f90)
> > #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> > #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> > #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> > #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> > #5 0x82c041 in ovs_cmdl_env_parse_all
> /workspace/ovn/ovs/lib/command-line.c:91:5
> > #6 0x5ad70d in ovn_dbctl_main
> /workspace/ovn/utilities/ovn-dbctl.c:132:20
> > #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> >
> > Indirect leak of 8 byte(s) in 2 object(s) allocated from:
> > #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
> > #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> > #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> > #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> > #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> > #5 0x82c0b6 in ovs_cmdl_env_parse_all
> /workspace/ovn/ovs/lib/command-line.c:96:9
> > #6 0x5ad70d in ovn_dbctl_main
> /workspace/ovn/utilities/ovn-dbctl.c:132:20
> > #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> >
> > Signed-off-by: Ales Musil 
>

Hi Simon,
thank you for your review.


> > ---
> >  utilities/ovn-dbctl.c | 76 +++
> >  1 file changed, 48 insertions(+), 28 deletions(-)
> >
> > diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
> > index f0ee0ba05..f96cab64e 100644
> > --- a/utilities/ovn-dbctl.c
> > +++ b/utilities/ovn-dbctl.c
> > @@ -109,6 +109,15 @@ static void server_loop(const struct
> ovn_dbctl_options *dbctl_options,
> >  struct ovsdb_idl *idl, int argc, char *argv[]);
> >  static void ovn_dbctl_exit(int status);
> >
> > +static void
> > +destroy_argv(int argc, char **argv)
> > +{
> > +for (int i = 0; i < argc; i++) {
> > +free(argv[i]);
> > +}
> > +free(argv);
> > +}
> > +
>
> This destroy_argv seems to be the dual of ovs_cmdl_env_parse_all(),
> does it belong in lib/command-line.c ?
>

It might fit more into command-line.c however getting something into ovs
and then
reflect that in ovn is way longer process.


>
> Also, do all other users of ovs_cmdl_env_parse_all() - in both OVS and OVN
> -
> unwind correctly on error?
>

I can see only single usage and that's here. So before this patch the
answer would be no.


>
>
> >  int
> >  ovn_dbctl_main(int argc, char *argv[],
> > const struct ovn_dbctl_options *dbctl_options)
> > @@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
> >  char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
> > _options,
> _parsed_options);
> >  if (error_s) {
> > +destroy_argv(argc, argv_);
> >  ctl_fatal("%s", error_s);
> >  }
> >
> > @@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
> >  bool daemon_mode = false;
> >  if (get_detach()) {
> >  if (argc != optind) {
> > +destroy_argv(argc, argv_);
> >  ctl_fatal("non-option arguments not supported with --detach
> "
> >"(use --help for help)");
> >  }
> > @@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
> >  if (error) {
> >  ovsdb_idl_destroy(idl);
> >  idl = the_idl = NULL;
> > +destroy_argv(argc, argv_);
> >
> > -for (int i = 0; i < argc; i++) {
> > -free(argv_[i]);
> > -}
> > -free(argv_);
> >  ctl_fatal("%s", error);
> >  }
> >
> > @@ -239,21 

Re: [ovs-dev] [PATCH v4 3/4] ovs-router: Introduce src option in ovs/route/add command.

[Simon Horman]

On Wed, Feb 22, 2023 at 06:31:55PM +0900, Nobuhiro MIKI wrote:
> On 2023/02/22 18:04, Simon Horman wrote:
> > On Wed, Feb 22, 2023 at 05:12:36PM +0900, Nobuhiro MIKI wrote:

...

> >> diff --git a/ofproto/ofproto-tnl-unixctl.man 
> >> b/ofproto/ofproto-tnl-unixctl.man
> >> index 13a465119a90..0af9246dab2e 100644
> >> --- a/ofproto/ofproto-tnl-unixctl.man
> >> +++ b/ofproto/ofproto-tnl-unixctl.man
> >> @@ -1,8 +1,9 @@
> >>  .SS "OPENVSWITCH TUNNELING COMMANDS"
> >>  These commands query and modify OVS tunnel components.
> >>  .
> >> -.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
> >> -Adds ipv4_address/plen route to vswitchd routing table. output_bridge
> >> +.IP "\fBovs/route/add \fIip\fB/\fIplen\fB \fIoutput_bridge\fB \
> >> +[\fIgateway\fB] [pkt_mark=\fImark\fB] [src=\fIsrc_ip\fB]\fR"
> >> +Adds \fIip\fR/\fIplen\fR route to vswitchd routing table. 
> >> \fIoutput_bridge\fR
> >>  needs to be OVS bridge name.  This command is useful if OVS cached
> >>  routes does not look right.
> >>  .
> > 
> > nit: I think the ipv4_address -> ip change should be a separate patch,
> >  which also fixes any other instances of this.
> > 
> > ...
> 
> OK. Before this patch, perform these replacements in another patch.

Yes, I think that would be best.
Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn] dbctl: Fix a couple of memory leaks

[Simon Horman]

On Wed, Feb 22, 2023 at 09:02:10AM +0100, Ales Musil wrote:
> Nothing is being freed wherever we are calling
> ctl_fatal which is fine because the program is
> about to shutdown anyway however one of the
> leaks was caught by address sanitizer.
> Fix most of the leaks that are happening before
> call to ctl_fatal.
> 
> Direct leak of 64 byte(s) in 1 object(s) allocated from:
> #0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o
> #1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9
> #2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12
> #3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12
> #4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23
> #5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5
> #6 0x82c117 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:98:5
> #7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Indirect leak of 10 byte(s) in 1 object(s) allocated from:
> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) 
> (BuildId: 3a287416f70de43f52382f0336980c876f655f90)
> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> #5 0x82c041 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:91:5
> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Indirect leak of 8 byte(s) in 2 object(s) allocated from:
> #0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
> #1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
> #2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
> #3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
> #4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
> #5 0x82c0b6 in ovs_cmdl_env_parse_all 
> /workspace/ovn/ovs/lib/command-line.c:96:9
> #6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
> #7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12
> 
> Signed-off-by: Ales Musil 
> ---
>  utilities/ovn-dbctl.c | 76 +++
>  1 file changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
> index f0ee0ba05..f96cab64e 100644
> --- a/utilities/ovn-dbctl.c
> +++ b/utilities/ovn-dbctl.c
> @@ -109,6 +109,15 @@ static void server_loop(const struct ovn_dbctl_options 
> *dbctl_options,
>  struct ovsdb_idl *idl, int argc, char *argv[]);
>  static void ovn_dbctl_exit(int status);
>  
> +static void
> +destroy_argv(int argc, char **argv)
> +{
> +for (int i = 0; i < argc; i++) {
> +free(argv[i]);
> +}
> +free(argv);
> +}
> +

This destroy_argv seems to be the dual of ovs_cmdl_env_parse_all(),
does it belong in lib/command-line.c ?

Also, do all other users of ovs_cmdl_env_parse_all() - in both OVS and OVN -
unwind correctly on error?


>  int
>  ovn_dbctl_main(int argc, char *argv[],
> const struct ovn_dbctl_options *dbctl_options)
> @@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
>  char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
> _options, _parsed_options);
>  if (error_s) {
> +destroy_argv(argc, argv_);
>  ctl_fatal("%s", error_s);
>  }
>  
> @@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
>  bool daemon_mode = false;
>  if (get_detach()) {
>  if (argc != optind) {
> +destroy_argv(argc, argv_);
>  ctl_fatal("non-option arguments not supported with --detach "
>"(use --help for help)");
>  }
> @@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
>  if (error) {
>  ovsdb_idl_destroy(idl);
>  idl = the_idl = NULL;
> +destroy_argv(argc, argv_);
>  
> -for (int i = 0; i < argc; i++) {
> -free(argv_[i]);
> -}
> -free(argv_);
>  ctl_fatal("%s", error);
>  }
>  
> @@ -239,21 +247,15 @@ cleanup:
>  }
>  free(commands);
>  if (error) {
> -for (int i = 0; i < argc; i++) {
> -free(argv_[i]);
> -}
> -free(argv_);
> +destroy_argv(argc, argv_);
>  ctl_fatal("%s", error);
>  }
>  }
>  
>  ovsdb_idl_destroy(idl);
>  idl = the_idl = NULL;
> +destroy_argv(argc, argv_);
>  
> -for (int i = 0; i < argc; i++) {
> -free(argv_[i]);
> -}
> -free(argv_);
>  

Re: [ovs-dev] [PATCH v4 3/4] ovs-router: Introduce src option in ovs/route/add command.

[Nobuhiro MIKI]

On 2023/02/22 18:04, Simon Horman wrote:
> On Wed, Feb 22, 2023 at 05:12:36PM +0900, Nobuhiro MIKI wrote:
>> When adding a route with ovs/route/add command, the source address
>> in "ovs_router_entry" structure is always the FIRST address that the
>> interface has. See "ovs_router_get_netdev_source_address"
>> function for more information.
>>
>> If an interface has multiple ipv4 and/or ipv6 addresses, there are use
>> cases where the user wants to control the source address. This patch
>> therefore addresses this issue by adding a src parameter.
>>
>> Note that same constraints also exist when caching routes from
>> Kernel FIB with Netlink, but are not dealt with in this patch.
>>
>> Signed-off-by: Nobuhiro MIKI 
> 
> ...
> 
>> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
>> index 6c5faf46ea15..3c86d3e54874 100644
>> --- a/lib/ovs-router.c
>> +++ b/lib/ovs-router.c
>> @@ -164,6 +164,46 @@ static void rt_init_match(struct match *match, uint32_t 
>> mark,
>>  match->flow.pkt_mark = mark;
>>  }
>>  
>> +static int
>> +verify_prefsrc(const struct in6_addr *ip6_dst,
>> +   const char output_bridge[],
>> +   struct in6_addr *prefsrc)
>> +{
>> +struct in6_addr *mask, *addr6;
>> +int err, n_in6, i;
>> +struct netdev *dev;
> 
> nit: reverse xmas tree - longest line to shortest - for local
>  variable declarations.

oh I missed the fix. Sorry.

> ...
> 
>> diff --git a/ofproto/ofproto-tnl-unixctl.man 
>> b/ofproto/ofproto-tnl-unixctl.man
>> index 13a465119a90..0af9246dab2e 100644
>> --- a/ofproto/ofproto-tnl-unixctl.man
>> +++ b/ofproto/ofproto-tnl-unixctl.man
>> @@ -1,8 +1,9 @@
>>  .SS "OPENVSWITCH TUNNELING COMMANDS"
>>  These commands query and modify OVS tunnel components.
>>  .
>> -.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
>> -Adds ipv4_address/plen route to vswitchd routing table. output_bridge
>> +.IP "\fBovs/route/add \fIip\fB/\fIplen\fB \fIoutput_bridge\fB \
>> +[\fIgateway\fB] [pkt_mark=\fImark\fB] [src=\fIsrc_ip\fB]\fR"
>> +Adds \fIip\fR/\fIplen\fR route to vswitchd routing table. 
>> \fIoutput_bridge\fR
>>  needs to be OVS bridge name.  This command is useful if OVS cached
>>  routes does not look right.
>>  .
> 
> nit: I think the ipv4_address -> ip change should be a separate patch,
>  which also fixes any other instances of this.
> 
> ...

OK. Before this patch, perform these replacements in another patch.
Thank you again for your review.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 2/4] ovs-router: Cleanup parser for ovs/route/add command.

[Nobuhiro MIKI]

On 2023/02/22 18:01, Simon Horman wrote:
> On Wed, Feb 22, 2023 at 05:12:35PM +0900, Nobuhiro MIKI wrote:
>> This patch cleans up the parser to accept pkt_mark and gw in any order.
>>
>> pkt_mark and gw are normally expected to be specified exactly once.
>> However, as with other tools, if specified multiple times, the last
>> specification is used. Also, pkt_mark and gw have separate prefix
>> strings so they can be parsed in any order.
>>
>> Signed-off-by: Nobuhiro MIKI 
> 
> Thanks, looks good.
> 
> Reviewed-by: Simon Horman 

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

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

[Simon Horman]

On Wed, Feb 22, 2023 at 10:03:07AM +0800, Faicker Mo wrote:
> Sorry. 
> The commit message and code are not changed. 
> Resended when I met a bug of intel-ovs-compilation test fail and add version 
> descriptions.

Thanks, I think I understand now.
But please don't top-post on this mailing list.

And could you please look at my comment regarding
the test you have added in this patch.

Thanks!

> From: Simon Horman 
> Date: 2023-02-21 23:09:05
> To:  Faicker Mo 
> Cc:  d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v6] netdev-offload-tc: del ufid mapping if 
> device not exist>On Wed, Feb 01, 2023 at 10:49:22AM +0800, 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 
> >> ---
> >> v2:
> >> - Add tc offload test case
> >> v3:
> >> - No change
> >> v4:
> >> - No change
> >> v5:
> >> - No change
> >> v6:
> >> - No change
> >
> >I am confused.
> >Why are there 4 versions (v3 - v6) with no change?
> >What does that mean?

...

> >> diff --git a/tests/system-offloads-traffic.at 
> >> b/tests/system-offloads-traffic.at
> >> index 16a4c1a00..15ea549a6 100644
> >> --- a/tests/system-offloads-traffic.at
> >> +++ b/tests/system-offloads-traffic.at
> >
> >The test seems to pass both with and without the change to
> >del_filter_and_ufid_mapping(). I think it would be better to construct a
> >test that fails without the code change and succeeds with it.
> >
> >I ran:
> >
> >TESTSUITEFLAGS="-k ufid" make check-offloads


This comment, here.

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

Re: [ovs-dev] [PATCH v4 3/4] ovs-router: Introduce src option in ovs/route/add command.

[Simon Horman]

On Wed, Feb 22, 2023 at 05:12:36PM +0900, Nobuhiro MIKI wrote:
> When adding a route with ovs/route/add command, the source address
> in "ovs_router_entry" structure is always the FIRST address that the
> interface has. See "ovs_router_get_netdev_source_address"
> function for more information.
> 
> If an interface has multiple ipv4 and/or ipv6 addresses, there are use
> cases where the user wants to control the source address. This patch
> therefore addresses this issue by adding a src parameter.
> 
> Note that same constraints also exist when caching routes from
> Kernel FIB with Netlink, but are not dealt with in this patch.
> 
> Signed-off-by: Nobuhiro MIKI 

...

> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 6c5faf46ea15..3c86d3e54874 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -164,6 +164,46 @@ static void rt_init_match(struct match *match, uint32_t 
> mark,
>  match->flow.pkt_mark = mark;
>  }
>  
> +static int
> +verify_prefsrc(const struct in6_addr *ip6_dst,
> +   const char output_bridge[],
> +   struct in6_addr *prefsrc)
> +{
> +struct in6_addr *mask, *addr6;
> +int err, n_in6, i;
> +struct netdev *dev;

nit: reverse xmas tree - longest line to shortest - for local
 variable declarations.

...

> diff --git a/ofproto/ofproto-tnl-unixctl.man b/ofproto/ofproto-tnl-unixctl.man
> index 13a465119a90..0af9246dab2e 100644
> --- a/ofproto/ofproto-tnl-unixctl.man
> +++ b/ofproto/ofproto-tnl-unixctl.man
> @@ -1,8 +1,9 @@
>  .SS "OPENVSWITCH TUNNELING COMMANDS"
>  These commands query and modify OVS tunnel components.
>  .
> -.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
> -Adds ipv4_address/plen route to vswitchd routing table. output_bridge
> +.IP "\fBovs/route/add \fIip\fB/\fIplen\fB \fIoutput_bridge\fB \
> +[\fIgateway\fB] [pkt_mark=\fImark\fB] [src=\fIsrc_ip\fB]\fR"
> +Adds \fIip\fR/\fIplen\fR route to vswitchd routing table. \fIoutput_bridge\fR
>  needs to be OVS bridge name.  This command is useful if OVS cached
>  routes does not look right.
>  .

nit: I think the ipv4_address -> ip change should be a separate patch,
 which also fixes any other instances of this.

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

Re: [ovs-dev] [PATCH v4 2/4] ovs-router: Cleanup parser for ovs/route/add command.

[Simon Horman]

On Wed, Feb 22, 2023 at 05:12:35PM +0900, Nobuhiro MIKI wrote:
> This patch cleans up the parser to accept pkt_mark and gw in any order.
> 
> pkt_mark and gw are normally expected to be specified exactly once.
> However, as with other tools, if specified multiple times, the last
> specification is used. Also, pkt_mark and gw have separate prefix
> strings so they can be parsed in any order.
> 
> Signed-off-by: Nobuhiro MIKI 

Thanks, looks good.

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

[ovs-dev] [PATCH v4 1/4] netdev-dummy: Support multiple IP addresses.

[Nobuhiro MIKI]

This is useful in test cases where multiple IPv4/IPv6 addresses
are assigned together.

Reviewed-by: Simon Horman 
Signed-off-by: Nobuhiro MIKI 
---
 lib/netdev-dummy.c | 67 +-
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 5d59c9c0312b..7d3d2aa23ba1 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -136,8 +136,7 @@ struct netdev_dummy {
 
 struct pcap_file *tx_pcap, *rxq_pcap OVS_GUARDED;
 
-struct in_addr address, netmask;
-struct in6_addr ipv6, ipv6_mask;
+struct ovs_list addrs;
 struct ovs_list rxes OVS_GUARDED; /* List of child "netdev_rxq_dummy"s. */
 
 struct hmap offloaded_flows OVS_GUARDED;
@@ -161,6 +160,12 @@ struct netdev_rxq_dummy {
 struct seq *seq;/* Reports newly queued packets. */
 };
 
+struct netdev_addr_dummy {
+struct in6_addr address;
+struct in6_addr netmask;
+struct ovs_list node;   /* In netdev_dummy's "addrs" list. */
+};
+
 static unixctl_cb_func netdev_dummy_set_admin_state;
 static int netdev_dummy_construct(struct netdev *);
 static void netdev_dummy_queue_packet(struct netdev_dummy *,
@@ -169,6 +174,7 @@ static void netdev_dummy_queue_packet(struct netdev_dummy *,
 static void dummy_packet_stream_close(struct dummy_packet_stream *);
 
 static void pkt_list_delete(struct ovs_list *);
+static void addr_list_delete(struct ovs_list *);
 
 static bool
 is_dummy_class(const struct netdev_class *class)
@@ -720,6 +726,7 @@ netdev_dummy_construct(struct netdev *netdev_)
 dummy_packet_conn_init(>conn);
 
 ovs_list_init(>rxes);
+ovs_list_init(>addrs);
 hmap_init(>offloaded_flows);
 ovs_mutex_unlock(>mutex);
 
@@ -756,6 +763,7 @@ netdev_dummy_destruct(struct netdev *netdev_)
 free(off_flow);
 }
 hmap_destroy(>offloaded_flows);
+addr_list_delete(>addrs);
 
 ovs_mutex_unlock(>mutex);
 ovs_mutex_destroy(>mutex);
@@ -803,32 +811,26 @@ netdev_dummy_get_addr_list(const struct netdev *netdev_, 
struct in6_addr **paddr
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
 int cnt = 0, i = 0, err = 0;
 struct in6_addr *addr, *mask;
+struct netdev_addr_dummy *addr_dummy;
 
 ovs_mutex_lock(>mutex);
-if (netdev->address.s_addr != INADDR_ANY) {
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
 cnt++;
 }
 
-if (ipv6_addr_is_set(>ipv6)) {
-cnt++;
-}
 if (!cnt) {
 err = EADDRNOTAVAIL;
 goto out;
 }
 addr = xmalloc(sizeof *addr * cnt);
 mask = xmalloc(sizeof *mask * cnt);
-if (netdev->address.s_addr != INADDR_ANY) {
-in6_addr_set_mapped_ipv4([i], netdev->address.s_addr);
-in6_addr_set_mapped_ipv4([i], netdev->netmask.s_addr);
+
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
+memcpy([i], _dummy->address, sizeof *addr);
+memcpy([i], _dummy->netmask, sizeof *mask);
 i++;
 }
 
-if (ipv6_addr_is_set(>ipv6)) {
-memcpy([i], >ipv6, sizeof *addr);
-memcpy([i], >ipv6_mask, sizeof *mask);
-i++;
-}
 if (paddr) {
 *paddr = addr;
 *pmask = mask;
@@ -844,14 +846,16 @@ out:
 }
 
 static int
-netdev_dummy_set_in4(struct netdev *netdev_, struct in_addr address,
+netdev_dummy_add_in4(struct netdev *netdev_, struct in_addr address,
  struct in_addr netmask)
 {
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+struct netdev_addr_dummy *addr_dummy = xmalloc(sizeof *addr_dummy);
 
 ovs_mutex_lock(>mutex);
-netdev->address = address;
-netdev->netmask = netmask;
+in6_addr_set_mapped_ipv4(_dummy->address, address.s_addr);
+in6_addr_set_mapped_ipv4(_dummy->netmask, netmask.s_addr);
+ovs_list_push_back(>addrs, _dummy->node);
 netdev_change_seq_changed(netdev_);
 ovs_mutex_unlock(>mutex);
 
@@ -859,14 +863,16 @@ netdev_dummy_set_in4(struct netdev *netdev_, struct 
in_addr address,
 }
 
 static int
-netdev_dummy_set_in6(struct netdev *netdev_, struct in6_addr *in6,
+netdev_dummy_add_in6(struct netdev *netdev_, struct in6_addr *in6,
  struct in6_addr *mask)
 {
 struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+struct netdev_addr_dummy *addr_dummy = xmalloc(sizeof *addr_dummy);
 
 ovs_mutex_lock(>mutex);
-netdev->ipv6 = *in6;
-netdev->ipv6_mask = *mask;
+addr_dummy->address = *in6;
+addr_dummy->netmask = *mask;
+ovs_list_push_back(>addrs, _dummy->node);
 netdev_change_seq_changed(netdev_);
 ovs_mutex_unlock(>mutex);
 
@@ -1178,7 +1184,10 @@ netdev_dummy_send(struct netdev *netdev, int qid,
 dummy_packet_conn_send(>conn, buffer, size);
 
 /* Reply to ARP requests for 'dev''s assigned IP address. */
-if (dev->address.s_addr) {
+struct netdev_addr_dummy *addr_dummy;
+LIST_FOR_EACH (addr_dummy, node, >addrs) {
+ovs_be32 address = 

[ovs-dev] [PATCH v4 3/4] ovs-router: Introduce src option in ovs/route/add command.

[Nobuhiro MIKI]

When adding a route with ovs/route/add command, the source address
in "ovs_router_entry" structure is always the FIRST address that the
interface has. See "ovs_router_get_netdev_source_address"
function for more information.

If an interface has multiple ipv4 and/or ipv6 addresses, there are use
cases where the user wants to control the source address. This patch
therefore addresses this issue by adding a src parameter.

Note that same constraints also exist when caching routes from
Kernel FIB with Netlink, but are not dealt with in this patch.

Signed-off-by: Nobuhiro MIKI 
---
 NEWS|  3 ++
 lib/ovs-router.c| 83 +
 ofproto/ofproto-tnl-unixctl.man |  5 +-
 tests/ovs-router.at | 78 +++
 4 files changed, 158 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 85b34962145e..d062ca4fac1a 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,9 @@ Post-v3.1.0
in order to create OVSDB sockets with access mode of 0770.
- QoS:
  * Added new configuration option 'jitter' for a linux-netem QoS type.
+   - ovs-appctl:
+ * Add support for selecting the source address with the
+   “ovs-appctl ovs/route/add" command.
 
 
 v3.1.0 - 16 Feb 2023
diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 6c5faf46ea15..3c86d3e54874 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -164,6 +164,46 @@ static void rt_init_match(struct match *match, uint32_t 
mark,
 match->flow.pkt_mark = mark;
 }
 
+static int
+verify_prefsrc(const struct in6_addr *ip6_dst,
+   const char output_bridge[],
+   struct in6_addr *prefsrc)
+{
+struct in6_addr *mask, *addr6;
+int err, n_in6, i;
+struct netdev *dev;
+
+err = netdev_open(output_bridge, NULL, );
+if (err) {
+return err;
+}
+
+err = netdev_get_addr_list(dev, , , _in6);
+if (err) {
+goto out;
+}
+
+for (i = 0; i < n_in6; i++) {
+struct in6_addr a1, a2;
+a1 = ipv6_addr_bitand(ip6_dst, [i]);
+a2 = ipv6_addr_bitand(prefsrc, [i]);
+
+/* Check that the intarface has "prefsrc" and
+ * it is same broadcast domain with "ip6_dst". */
+if (IN6_ARE_ADDR_EQUAL(prefsrc, [i]) &&
+IN6_ARE_ADDR_EQUAL(, )) {
+goto out;
+}
+}
+err = ENOENT;
+
+out:
+free(addr6);
+free(mask);
+netdev_close(dev);
+return err;
+}
+
 int
 ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst,
  const char output_bridge[],
@@ -217,8 +257,12 @@ static int
 ovs_router_insert__(uint32_t mark, uint8_t priority, bool local,
 const struct in6_addr *ip6_dst,
 uint8_t plen, const char output_bridge[],
-const struct in6_addr *gw)
+const struct in6_addr *gw,
+const struct in6_addr *ip6_src)
 {
+int (*f)(const struct in6_addr *ip6_dst,
+ const char output_bridge[],
+ struct in6_addr *prefsrc);
 const struct cls_rule *cr;
 struct ovs_router_entry *p;
 struct match match;
@@ -236,11 +280,17 @@ ovs_router_insert__(uint32_t mark, uint8_t priority, bool 
local,
 p->plen = plen;
 p->local = local;
 p->priority = priority;
-err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
-   >src_addr);
+
+if (ipv6_addr_is_set(ip6_src)) {
+p->src_addr = *ip6_src;
+f = verify_prefsrc;
+} else {
+f = ovs_router_get_netdev_source_address;
+}
+
+err = f(ip6_dst, output_bridge, >src_addr);
 if (err && ipv6_addr_is_set(gw)) {
-err = ovs_router_get_netdev_source_address(gw, output_bridge,
-   >src_addr);
+err = f(gw, output_bridge, >src_addr);
 }
 if (err) {
 struct ds ds = DS_EMPTY_INITIALIZER;
@@ -274,7 +324,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr 
*ip_dst, uint8_t plen,
 {
 if (use_system_routing_table) {
 uint8_t priority = local ? plen + 64 : plen;
-ovs_router_insert__(mark, priority, local, ip_dst, plen, 
output_bridge, gw);
+ovs_router_insert__(mark, priority, local, ip_dst, plen,
+output_bridge, gw, _any);
 }
 }
 
@@ -341,10 +392,13 @@ static void
 ovs_router_add(struct unixctl_conn *conn, int argc,
   const char *argv[], void *aux OVS_UNUSED)
 {
+struct in6_addr src6 = in6addr_any;
 struct in6_addr gw6 = in6addr_any;
+char src6_s[IPV6_SCAN_LEN + 1];
 struct in6_addr ip6;
 uint32_t mark = 0;
 unsigned int plen;
+ovs_be32 src = 0;
 ovs_be32 gw = 0;
 bool is_ipv6;
 ovs_be32 ip;
@@ -369,10 +423,17 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
 }
 
 if (is_ipv6) {
+if (ovs_scan(argv[i], 

[ovs-dev] [PATCH v4 4/4] route-table: Retrieving the preferred source address from Netlink.

[Nobuhiro MIKI]

We can use the "ip route add ... src ..." command to set the preferred
source address for each entry in the kernel FIB. OVS has a mechanism to
cache the FIB, but the preferred source address is ignored and
calculated with its own logic. This patch resolves the difference
between kernel FIB and OVS route table cache by retrieving the
RTA_PREFSRC attribute of Netlink messages.

Reviewed-by: Simon Horman 
Signed-off-by: Nobuhiro MIKI 
---
 lib/ovs-router.c  |  6 +++---
 lib/ovs-router.h  |  3 ++-
 lib/route-table.c | 16 +++-
 tests/system-route.at | 39 +++
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 3c86d3e54874..854caa6f17a9 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -319,13 +319,13 @@ ovs_router_insert__(uint32_t mark, uint8_t priority, bool 
local,
 
 void
 ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen,
-  bool local, const char output_bridge[], 
-  const struct in6_addr *gw)
+  bool local, const char output_bridge[],
+  const struct in6_addr *gw, const struct in6_addr *prefsrc)
 {
 if (use_system_routing_table) {
 uint8_t priority = local ? plen + 64 : plen;
 ovs_router_insert__(mark, priority, local, ip_dst, plen,
-output_bridge, gw, _any);
+output_bridge, gw, prefsrc);
 }
 }
 
diff --git a/lib/ovs-router.h b/lib/ovs-router.h
index d8ce3c00ded5..eb4ff85d9e63 100644
--- a/lib/ovs-router.h
+++ b/lib/ovs-router.h
@@ -32,7 +32,8 @@ bool ovs_router_lookup(uint32_t mark, const struct in6_addr 
*ip_dst,
 void ovs_router_init(void);
 void ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst,
uint8_t plen, bool local,
-   const char output_bridge[], const struct in6_addr *gw);
+   const char output_bridge[], const struct in6_addr *gw,
+   const struct in6_addr *prefsrc);
 void ovs_router_flush(void);
 
 void ovs_router_disable_system_routing_table(void);
diff --git a/lib/route-table.c b/lib/route-table.c
index ac82cf262f86..9927dcc1854b 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -51,6 +51,7 @@ struct route_data {
 
 /* Extracted from Netlink attributes. */
 struct in6_addr rta_dst; /* 0 if missing. */
+struct in6_addr rta_prefsrc; /* 0 if missing. */
 struct in6_addr rta_gw;
 char ifname[IFNAMSIZ]; /* Interface name. */
 uint32_t mark;
@@ -201,6 +202,7 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 [RTA_OIF] = { .type = NL_A_U32, .optional = true },
 [RTA_GATEWAY] = { .type = NL_A_U32, .optional = true },
 [RTA_MARK] = { .type = NL_A_U32, .optional = true },
+[RTA_PREFSRC] = { .type = NL_A_U32, .optional = true },
 };
 
 static const struct nl_policy policy6[] = {
@@ -208,6 +210,7 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 [RTA_OIF] = { .type = NL_A_U32, .optional = true },
 [RTA_MARK] = { .type = NL_A_U32, .optional = true },
 [RTA_GATEWAY] = { .type = NL_A_IPV6, .optional = true },
+[RTA_PREFSRC] = { .type = NL_A_IPV6, .optional = true },
 };
 
 struct nlattr *attrs[ARRAY_SIZE(policy)];
@@ -274,6 +277,16 @@ route_table_parse(struct ofpbuf *buf, struct 
route_table_msg *change)
 } else if (ipv4) {
 in6_addr_set_mapped_ipv4(>rd.rta_dst, 0);
 }
+if (attrs[RTA_PREFSRC]) {
+if (ipv4) {
+ovs_be32 prefsrc;
+prefsrc = nl_attr_get_be32(attrs[RTA_PREFSRC]);
+in6_addr_set_mapped_ipv4(>rd.rta_prefsrc, prefsrc);
+} else {
+change->rd.rta_prefsrc =
+nl_attr_get_in6_addr(attrs[RTA_PREFSRC]);
+}
+}
 if (attrs[RTA_GATEWAY]) {
 if (ipv4) {
 ovs_be32 gw;
@@ -309,7 +322,8 @@ route_table_handle_msg(const struct route_table_msg *change)
 const struct route_data *rd = >rd;
 
 ovs_router_insert(rd->mark, >rta_dst, rd->rtm_dst_len,
-  rd->local, rd->ifname, >rta_gw);
+  rd->local, rd->ifname, >rta_gw,
+  >rta_prefsrc);
 }
 }
 
diff --git a/tests/system-route.at b/tests/system-route.at
index 270956d13f6f..114aaebc77f3 100644
--- a/tests/system-route.at
+++ b/tests/system-route.at
@@ -25,3 +25,42 @@ OVS_WAIT_UNTIL([test `ovs-appctl ovs/route/show | grep -c 
'p1-route'` -eq 0 ])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ovs-route - add system route with src - ipv4])
+AT_KEYWORDS([route])
+OVS_TRAFFIC_VSWITCHD_START()
+AT_CHECK([ip link set br0 up])
+
+AT_CHECK([ip addr add 192.168.9.2/24 dev br0], [0], [stdout])
+AT_CHECK([ip addr add 192.168.9.3/24 dev br0], [0], 

[ovs-dev] [PATCH v4 0/4] Add support for preffered src address in ovs-router

[Nobuhiro MIKI]

With this series, the preferred source address in ovs-router is obtained
from both ovs/route/add command and kernel FIB.

v4:
- Add cleanup patch for ovs/route/add
- Remove unrelated code
v3:
- Fix netdev-dummy to support multiple IP addresses
- Add validation and unit tests for ovs/route/add
- Refactor parsing for optional parameters in ovs/route/add command
v2:
- Add NEWS

Nobuhiro MIKI (4):
  netdev-dummy: Support multiple IP addresses.
  ovs-router: Cleanup parser for ovs/route/add command.
  ovs-router: Introduce src option in ovs/route/add command.
  route-table: Retrieving the preferred source address from Netlink.

 NEWS|   3 +
 lib/netdev-dummy.c  |  67 ++--
 lib/ovs-router.c| 137 
 lib/ovs-router.h|   3 +-
 lib/route-table.c   |  16 +++-
 ofproto/ofproto-tnl-unixctl.man |   5 +-
 tests/ovs-router.at | 105 +++-
 tests/system-route.at   |  39 +
 8 files changed, 309 insertions(+), 66 deletions(-)

-- 
2.31.1

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

[ovs-dev] [PATCH v4 2/4] ovs-router: Cleanup parser for ovs/route/add command.

[Nobuhiro MIKI]

This patch cleans up the parser to accept pkt_mark and gw in any order.

pkt_mark and gw are normally expected to be specified exactly once.
However, as with other tools, if specified multiple times, the last
specification is used. Also, pkt_mark and gw have separate prefix
strings so they can be parsed in any order.

Signed-off-by: Nobuhiro MIKI 
---
 lib/ovs-router.c| 50 -
 tests/ovs-router.at | 27 
 2 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 5d0fbd503e9e..6c5faf46ea15 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -345,41 +345,45 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
 struct in6_addr ip6;
 uint32_t mark = 0;
 unsigned int plen;
+ovs_be32 gw = 0;
+bool is_ipv6;
 ovs_be32 ip;
 int err;
+int i;
 
 if (scan_ipv4_route(argv[1], , )) {
-ovs_be32 gw = 0;
-
-if (argc > 3) {
-if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, ) &&
-!ip_parse(argv[3], )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark or 
gateway");
-return;
-}
-}
 in6_addr_set_mapped_ipv4(, ip);
-if (gw) {
-in6_addr_set_mapped_ipv4(, gw);
-}
 plen += 96;
+is_ipv6 = false;
 } else if (scan_ipv6_route(argv[1], , )) {
-if (argc > 3) {
-if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, ) &&
-!ipv6_parse(argv[3], )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6 
gateway");
-return;
-}
-}
+is_ipv6 = true;
 } else {
 unixctl_command_reply_error(conn, "Invalid parameters");
 return;
 }
-if (argc > 4) {
-if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, )) {
-unixctl_command_reply_error(conn, "Invalid pkt_mark");
-return;
+
+/* Parse optional parameters. */
+for (i = 3; i < argc; i++) {
+if (ovs_scan(argv[i], "pkt_mark=%"SCNi32, )) {
+continue;
 }
+
+if (is_ipv6) {
+if (ipv6_parse(argv[i], )) {
+continue;
+}
+} else {
+if (ip_parse(argv[i], )) {
+continue;
+}
+}
+
+unixctl_command_reply_error(conn, "Invalid parameters");
+return;
+}
+
+if (gw) {
+in6_addr_set_mapped_ipv4(, gw);
 }
 
 err = ovs_router_insert__(mark, plen + 32, false, , plen, argv[2], 
);
diff --git a/tests/ovs-router.at b/tests/ovs-router.at
index 6dacc2954bc6..96fb6e188eef 100644
--- a/tests/ovs-router.at
+++ b/tests/ovs-router.at
@@ -1,14 +1,33 @@
 AT_BANNER([ovs-router])
 
-AT_SETUP([appctl - route/add with gateway])
+AT_SETUP([appctl - route/add with gateway and pkt_mark])
 AT_KEYWORDS([ovs_router])
-OVS_VSWITCHD_START([add-port br0 p2 -- set Interface p2 type=gre \
-options:local_ip=2.2.2.2 options:remote_ip=1.1.1.1 \
--- add-port br0 p1  -- set interface p1 type=dummy])
+OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
 AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 2.2.2.2/24], [0], [OK
 ])
+AT_CHECK([ovs-appctl ovs/route/add 2.2.2.3/32 br0 pkt_mark=1], [0], [OK
+])
 AT_CHECK([ovs-appctl ovs/route/add 1.1.1.0/24 br0 2.2.2.10], [0], [OK
 ])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.2.0/24 br0 2.2.2.10 pkt_mark=2], [0], 
[OK
+])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.3.0/24 br0 pkt_mark=3], [2], [], [dnl
+Error while inserting route.
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/add 1.1.foo.bar/24 br0 2.2.2.10], [2], [], [dnl
+Invalid parameters
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/add 2.2.2.4/24 br0 pkt_mark=baz], [2], [], [dnl
+Invalid parameters
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [dnl
+User: 1.1.1.0/24 dev br0 GW 2.2.2.10 SRC 2.2.2.2
+User: 1.1.2.0/24 MARK 2 dev br0 GW 2.2.2.10 SRC 2.2.2.2
+User: 2.2.2.3/32 MARK 1 dev br0 SRC 2.2.2.2
+])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
-- 
2.31.1

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

[ovs-dev] [PATCH ovn] dbctl: Fix a couple of memory leaks

[Ales Musil]

Nothing is being freed wherever we are calling
ctl_fatal which is fine because the program is
about to shutdown anyway however one of the
leaks was caught by address sanitizer.
Fix most of the leaks that are happening before
call to ctl_fatal.

Direct leak of 64 byte(s) in 1 object(s) allocated from:
#0 0x568d70 in __interceptor_realloc.part.0 asan_malloc_linux.cpp.o
#1 0xa530a5 in xrealloc__ /workspace/ovn/ovs/lib/util.c:147:9
#2 0xa530a5 in xrealloc /workspace/ovn/ovs/lib/util.c:179:12
#3 0xa530a5 in x2nrealloc /workspace/ovn/ovs/lib/util.c:239:12
#4 0xa2ee57 in svec_expand /workspace/ovn/ovs/lib/svec.c:92:23
#5 0xa2ef6e in svec_terminate /workspace/ovn/ovs/lib/svec.c:116:5
#6 0x82c117 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:98:5
#7 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#8 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Indirect leak of 10 byte(s) in 1 object(s) allocated from:
#0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07) 
(BuildId: 3a287416f70de43f52382f0336980c876f655f90)
#1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
#2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
#3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
#4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
#5 0x82c041 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:91:5
#6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Indirect leak of 8 byte(s) in 2 object(s) allocated from:
#0 0x569c07 in malloc (/workspace/ovn/utilities/ovn-nbctl+0x569c07)
#1 0xa52d4c in xmalloc__ /workspace/ovn/ovs/lib/util.c:137:15
#2 0xa52d4c in xmalloc /workspace/ovn/ovs/lib/util.c:172:12
#3 0xa52d4c in xmemdup0 /workspace/ovn/ovs/lib/util.c:193:15
#4 0xa2e580 in svec_add /workspace/ovn/ovs/lib/svec.c:71:27
#5 0x82c0b6 in ovs_cmdl_env_parse_all 
/workspace/ovn/ovs/lib/command-line.c:96:9
#6 0x5ad70d in ovn_dbctl_main /workspace/ovn/utilities/ovn-dbctl.c:132:20
#7 0x5b58c7 in main /workspace/ovn/utilities/ovn-nbctl.c:7943:12

Signed-off-by: Ales Musil 
---
 utilities/ovn-dbctl.c | 76 +++
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c
index f0ee0ba05..f96cab64e 100644
--- a/utilities/ovn-dbctl.c
+++ b/utilities/ovn-dbctl.c
@@ -109,6 +109,15 @@ static void server_loop(const struct ovn_dbctl_options 
*dbctl_options,
 struct ovsdb_idl *idl, int argc, char *argv[]);
 static void ovn_dbctl_exit(int status);
 
+static void
+destroy_argv(int argc, char **argv)
+{
+for (int i = 0; i < argc; i++) {
+free(argv[i]);
+}
+free(argv);
+}
+
 int
 ovn_dbctl_main(int argc, char *argv[],
const struct ovn_dbctl_options *dbctl_options)
@@ -151,6 +160,7 @@ ovn_dbctl_main(int argc, char *argv[],
 char *error_s = ovs_cmdl_parse_all(argc, argv_, get_all_options(),
_options, _parsed_options);
 if (error_s) {
+destroy_argv(argc, argv_);
 ctl_fatal("%s", error_s);
 }
 
@@ -179,6 +189,7 @@ ovn_dbctl_main(int argc, char *argv[],
 bool daemon_mode = false;
 if (get_detach()) {
 if (argc != optind) {
+destroy_argv(argc, argv_);
 ctl_fatal("non-option arguments not supported with --detach "
   "(use --help for help)");
 }
@@ -206,11 +217,8 @@ ovn_dbctl_main(int argc, char *argv[],
 if (error) {
 ovsdb_idl_destroy(idl);
 idl = the_idl = NULL;
+destroy_argv(argc, argv_);
 
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
 ctl_fatal("%s", error);
 }
 
@@ -239,21 +247,15 @@ cleanup:
 }
 free(commands);
 if (error) {
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
+destroy_argv(argc, argv_);
 ctl_fatal("%s", error);
 }
 }
 
 ovsdb_idl_destroy(idl);
 idl = the_idl = NULL;
+destroy_argv(argc, argv_);
 
-for (int i = 0; i < argc; i++) {
-free(argv_[i]);
-}
-free(argv_);
 exit(EXIT_SUCCESS);
 }
 
@@ -1240,27 +1242,31 @@ dbctl_client(const struct ovn_dbctl_options 
*dbctl_options,
 
 ctl_timeout_setup(timeout);
 
+char *err = NULL;
+char *cmd_error = NULL;
+char *cmd_result = NULL;
+int exit_status = EXIT_SUCCESS;
 struct jsonrpc *client;
+
 int error = unixctl_client_create(socket_name, );
 if (error) {
-ctl_fatal("%s: could not connect to %s daemon (%s); "
-  "unset %s to avoid using daemon",
-