Re: [ovs-dev] [PATCH ovn branch-22.12] northd: Fix missig "); " from LB flows

[0-day Robot]

Bleep bloop.  Greetings Ales Musil, 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:
ERROR: "Fixes" tag is malformed.
Use the following format:
  git log -1 --pretty=format:"Fixes: %h (\"%s\")" --abbrev=12 COMMIT_REF

11: Fixes: cd600de6 ("northd: Add flag for CT related.")

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


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] tests: Extends tests for feature flag compatibility

[Ales Musil]

Extend the tests for "ct-no-masked-label" and
"ovn-ct-lb-related" with check for LB lflows
when LB is configured with "skip_snat" or "force_snat".

Signed-off-by: Ales Musil 
---
This patch should be backported only to 23.03.
---
 tests/ovn-northd.at | 32 
 1 file changed, 32 insertions(+)

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 3fa02d2b3..96a783420 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -7906,6 +7906,22 @@ AT_CHECK([ovn-sbctl lflow-list | grep -e natted -e 
ct_lb], [0], [dnl
   table=2 (ls_out_pre_stateful), priority=110  , match=(reg0[[2]] == 1), 
action=(ct_lb;)
 ])
 
+check ovn-nbctl --wait=sb set logical_router lr 
options:lb_force_snat_ip="42.42.42.1"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 66.66.66.66 && ct_label.natted == 1), 
action=(flags.force_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 66.66.66.66), action=(flags.force_snat_for_lb = 1; 
ct_lb(backends=42.42.42.2);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove logical_router lr options lb_force_snat_ip
+
+check ovn-nbctl --wait=sb set load_balancer lb-test options:skip_snat="true"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 66.66.66.66 && ct_label.natted == 1), 
action=(flags.skip_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 66.66.66.66), action=(flags.skip_snat_for_lb = 1; 
ct_lb(backends=42.42.42.2);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove load_balancer lb-test options skip_snat
+
 AS_BOX([Chassis upgrades and supports ct_lb_mark - use ct_lb_mark and 
ct_mark.natted])
 check ovn-sbctl set chassis hv other_config:ct-no-masked-label=true
 check ovn-nbctl --wait=sb sync
@@ -8605,6 +8621,22 @@ AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" 
lflows1], [0], [dnl
   table=? (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 192.168.0.1), action=(ct_lb(backends=192.168.1.10);)
 ])
 
+check ovn-nbctl --wait=sb set logical_router lr 
options:lb_force_snat_ip="192.168.1.1"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), 
action=(flags.force_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 192.168.0.1), action=(flags.force_snat_for_lb = 1; 
ct_lb(backends=192.168.1.10);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove logical_router lr options lb_force_snat_ip
+
+check ovn-nbctl --wait=sb set load_balancer lb-test options:skip_snat="true"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), 
action=(flags.skip_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 192.168.0.1), action=(flags.skip_snat_for_lb = 1; 
ct_lb(backends=192.168.1.10);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove load_balancer lb-test options skip_snat
+
 AT_CHECK([grep -e "ls_in_acl" -e "ls_out_acl" lflows1 | grep 
"priority=65532"], [0], [dnl
   table=? (ls_in_acl  ), priority=65532, match=(!ct.est && ct.rel && 
!ct.new && !ct.inv && ct_label.blocked == 0), action=(reg0[[17]] = 1; next;)
   table=? (ls_in_acl  ), priority=65532, match=(ct.est && !ct.rel && 
!ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(reg0[[9]] = 0; 
reg0[[10]] = 0; reg0[[17]] = 1; next;)
-- 
2.39.2

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

[ovs-dev] [PATCH ovn branch-22.12] northd: Fix missig "); " from LB flows

[Ales Musil]

Fix missing enclose for LB lflows when both
"ct-no-masked-label" and "ovn-ct-lb-related"
features are set false and the LB is configured
with either "skip_snat" or "force_snat".
Add missing test case for those.

Fixes: cd600de6 ("northd: Add flag for CT related.")
Signed-off-by: Ales Musil 
---
This patch should be backported all the way down to 21.12.
---
 northd/northd.c |  8 
 tests/ovn-northd.at | 32 
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index b3aa3a26b..b8126e05f 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10535,9 +10535,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
 
 if (lb->skip_snat) {
 const char *skip_snat = features->ct_lb_related && !drop
-? "; skip_snat);"
+? "; skip_snat"
 : "";
-skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s%s",
+skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s%s);",
  ds_cstr(action), skip_snat);
 skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; "
  "next;");
@@ -10672,9 +10672,9 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip 
*lb_vip,
 skip_snat_est_action, lflows, prio, meter_groups);
 
 const char *force_snat = features->ct_lb_related && !drop
- ? "; force_snat);"
+ ? "; force_snat"
  : "";
-char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s",
+char *new_actions = xasprintf("flags.force_snat_for_lb = 1; %s%s);",
   ds_cstr(action), force_snat);
 build_gw_lrouter_nat_flows_for_lb(lb, gw_router_force_snat,
 n_gw_router_force_snat, reject, new_match,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index ab06d0406..52f4757b4 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -7840,6 +7840,22 @@ AT_CHECK([ovn-sbctl lflow-list | grep -e natted -e 
ct_lb], [0], [dnl
   table=2 (ls_out_pre_stateful), priority=110  , match=(reg0[[2]] == 1), 
action=(ct_lb;)
 ])
 
+check ovn-nbctl --wait=sb set logical_router lr 
options:lb_force_snat_ip="42.42.42.1"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 66.66.66.66 && ct_label.natted == 1), 
action=(flags.force_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 66.66.66.66), action=(flags.force_snat_for_lb = 1; 
ct_lb(backends=42.42.42.2);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove logical_router lr options lb_force_snat_ip
+
+check ovn-nbctl --wait=sb set load_balancer lb-test options:skip_snat="true"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 66.66.66.66 && ct_label.natted == 1), 
action=(flags.skip_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 66.66.66.66), action=(flags.skip_snat_for_lb = 1; 
ct_lb(backends=42.42.42.2);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove load_balancer lb-test options skip_snat
+
 AS_BOX([Chassis upgrades and supports ct_lb_mark - use ct_lb_mark and 
ct_mark.natted])
 check ovn-sbctl set chassis hv other_config:ct-no-masked-label=true
 check ovn-nbctl --wait=sb sync
@@ -8502,6 +8518,22 @@ AT_CHECK([grep -e "lr_in_defrag" -e "lr_in_dnat" 
lflows1], [0], [dnl
   table=? (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 192.168.0.1), action=(ct_lb(backends=192.168.1.10);)
 ])
 
+check ovn-nbctl --wait=sb set logical_router lr 
options:lb_force_snat_ip="192.168.1.1"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), 
action=(flags.force_snat_for_lb = 1; next;)
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.new && !ct.rel && 
ip4 && reg0 == 192.168.0.1), action=(flags.force_snat_for_lb = 1; 
ct_lb(backends=192.168.1.10);)
+  table=7 (lr_in_dnat ), priority=0, match=(1), action=(next;)
+])
+check ovn-nbctl remove logical_router lr options lb_force_snat_ip
+
+check ovn-nbctl --wait=sb set load_balancer lb-test options:skip_snat="true"
+AT_CHECK([ovn-sbctl lflow-list | grep lr_in_dnat], [0], [dnl
+  table=7 (lr_in_dnat ), priority=110  , match=(ct.est && !ct.rel && 
ip4 && reg0 == 192.168.0.1 && ct_label.natted == 1), 
action=(flags.skip_snat_for_lb = 1; 

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

[Eelco Chaudron]

On 8 Mar 2023, at 3:34, Faicker Mo wrote:

> Updated,
> Under my env, the sleep time before revalidator/purge can't be shorten to 
> 0.2s or 0.5s.
> It's not reasonable there exists a dp flow after revalidator/purge.

What about my suggested change of removing the sleeps completely? Does it work 
for you, or is it not testing correctly?

>
> From: Faicker Mo 
>  Date: 2023-03-06 12:22:48
> To:Eelco Chaudron 
>  cc: d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
> Subject: Re:Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not 
> exist
> It failed at system-offloads-traffic.at:737 without the sleep before 
> revalidator/purge. There was a flow,
>> 2023-03-06T02:17:13.245Z|00079|unixctl|DBG|received request 
>> dpctl/dump-flows[], id=0
>> 2023-03-06T02:17:13.245Z|00080|unixctl|DBG|replying with success, id=0: 
>> "recirc_id(0),in_port(4),eth(src=f6:2d:a8:f7:d2:f7,dst=aa:1a:54:e9:c5:56),eth_type(0x0800),ipv4(frag=no),
>>  packe
> ts:1, bytes:84, used:0.040s, actions:2
>> "
>
> Tested,
> The first sleep can be removed with msg filters.
> The sleep before revalidator/purge can be shorten to 0.2s.
>
>
>
>
>
>
> From: Eelco Chaudron 
> Date: 2023-03-03 19:11:25
> To:  Faicker Mo 
> Cc:  d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
> Subject: Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not 
> exist>
>>
>> On 2 Mar 2023, at 8:57, 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
>>> v7:
>>> - Minor fix for test case and rebased
>>> v8:
>>> - Shorten the time of the test case
>>> ---
>>>  lib/netdev-offload-tc.c  |  3 +-
>>>  tests/system-offloads-traffic.at | 54 
>>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>>
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index 4fb9d9f21..dd2020cad 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -276,8 +276,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const 
>>> ovs_u128 *ufid,
>>>  }
>>>
>>>  err = tc_del_flower_filter(id);
>>> -if (!err) {
>>> +if (!err || err == ENODEV) {
>>>  del_ufid_tc_mapping(ufid);
>>> +return 0;
>>>  }
>>>  return err;
>>>  }
>>> diff --git a/tests/system-offloads-traffic.at 
>>> b/tests/system-offloads-traffic.at
>>> index 7558812eb..ba18711cb 100644
>>> --- a/tests/system-offloads-traffic.at
>>> +++ b/tests/system-offloads-traffic.at
>>> @@ -690,3 +690,57 @@ 
>>> OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(5),le(check_pkt_len(size=100,gt(5),
>>>
>>>  OVS_TRAFFIC_VSWITCHD_STOP
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([offloads - delete ufid mapping if device not exist - offloads 
>>> enabled])
>>> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
>>> other_config:hw-offload=true])
>>> +
>>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>>> +
>>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
>>> +
>>> +dnl Disable IPv6 to skip unexpected flow
>>> +AT_CHECK([sysctl -w net.ipv6.conf.br0.disable_ipv6=1], [0], [ignore])
>>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>>> [ignore])
>>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>>> [ignore])
>>> +NS_CHECK_EXEC([at_ns2], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>>> [ignore])
>>> +
>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "aa:1a:54:e9:c5:56")
>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>>> +
>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], 
>>> [dnl
>>> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
>>> +])
>>> +sleep 1
>>> +
>>> +dnl Delete and add interface ovs-p0/p0
>>> +AT_CHECK([ip link del dev ovs-p0])
>>> +AT_CHECK([ip link add p0 type veth peer name ovs-p0 || return 77])
>>> +AT_CHECK([ip link set p0 netns at_ns0])
>>> +AT_CHECK([ip link set dev ovs-p0 up])
>>> +NS_CHECK_EXEC([at_ns0], [ip addr add dev p0 "10.1.1.1/24"])
>>> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 up])
>>> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address "aa:1a:54:e9:c5:56"])
>>> +
>>> +sleep 1
>>> +AT_CHECK([ovs-appctl revalidator/purge])
>>> +
>>> +dnl Generate flows to trigger the hmap expand once
>>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
>>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], 
>>> 

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

[Faicker Mo]

Updated,
Under my env, the sleep time before revalidator/purge can't be shorten to 0.2s 
or 0.5s.
It's not reasonable there exists a dp flow after revalidator/purge.


From: Faicker Mo 
 Date: 2023-03-06 12:22:48
To:Eelco Chaudron 
 cc: d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
Subject: Re:Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not 
exist
It failed at system-offloads-traffic.at:737 without the sleep before 
revalidator/purge. There was a flow,
> 2023-03-06T02:17:13.245Z|00079|unixctl|DBG|received request 
> dpctl/dump-flows[], id=0
> 2023-03-06T02:17:13.245Z|00080|unixctl|DBG|replying with success, id=0: 
> "recirc_id(0),in_port(4),eth(src=f6:2d:a8:f7:d2:f7,dst=aa:1a:54:e9:c5:56),eth_type(0x0800),ipv4(frag=no),
>  packe
ts:1, bytes:84, used:0.040s, actions:2
> "

Tested,
The first sleep can be removed with msg filters.
The sleep before revalidator/purge can be shorten to 0.2s.






From: Eelco Chaudron 
Date: 2023-03-03 19:11:25
To:  Faicker Mo 
Cc:  d...@openvswitch.org,i.maxim...@ovn.org,simon.hor...@corigine.com
Subject: Re: [PATCH v8] netdev-offload-tc: del ufid mapping if device not exist>
>
>On 2 Mar 2023, at 8:57, 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
>> v7:
>> - Minor fix for test case and rebased
>> v8:
>> - Shorten the time of the test case
>> ---
>>  lib/netdev-offload-tc.c  |  3 +-
>>  tests/system-offloads-traffic.at | 54 
>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>
>>
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 4fb9d9f21..dd2020cad 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -276,8 +276,9 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const 
>> ovs_u128 *ufid,
>>  }
>>
>>  err = tc_del_flower_filter(id);
>> -if (!err) {
>> +if (!err || err == ENODEV) {
>>  del_ufid_tc_mapping(ufid);
>> +return 0;
>>  }
>>  return err;
>>  }
>> diff --git a/tests/system-offloads-traffic.at 
>> b/tests/system-offloads-traffic.at
>> index 7558812eb..ba18711cb 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -690,3 +690,57 @@ 
>> OVS_CHECK_ACTIONS([check_pkt_len(size=200,gt(5),le(check_pkt_len(size=100,gt(5),
>>
>>  OVS_TRAFFIC_VSWITCHD_STOP
>>  AT_CLEANUP
>> +
>> +AT_SETUP([offloads - delete ufid mapping if device not exist - offloads 
>> enabled])
>> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
>> other_config:hw-offload=true])
>> +
>> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>> +
>> +ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
>> +
>> +dnl Disable IPv6 to skip unexpected flow
>> +AT_CHECK([sysctl -w net.ipv6.conf.br0.disable_ipv6=1], [0], [ignore])
>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>> [ignore])
>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>> [ignore])
>> +NS_CHECK_EXEC([at_ns2], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], [0], 
>> [ignore])
>> +
>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "aa:1a:54:e9:c5:56")
>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
>> +
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], 
>> [dnl
>> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
>> +])
>> +sleep 1
>> +
>> +dnl Delete and add interface ovs-p0/p0
>> +AT_CHECK([ip link del dev ovs-p0])
>> +AT_CHECK([ip link add p0 type veth peer name ovs-p0 || return 77])
>> +AT_CHECK([ip link set p0 netns at_ns0])
>> +AT_CHECK([ip link set dev ovs-p0 up])
>> +NS_CHECK_EXEC([at_ns0], [ip addr add dev p0 "10.1.1.1/24"])
>> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 up])
>> +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address "aa:1a:54:e9:c5:56"])
>> +
>> +sleep 1
>> +AT_CHECK([ovs-appctl revalidator/purge])
>> +
>> +dnl Generate flows to trigger the hmap expand once
>> +ADD_VETH(p2, at_ns2, br0, "10.1.1.3/24")
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.2 | FORMAT_PING], [0], 
>> [dnl
>> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
>> +])
>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 2 -i 0.2 10.1.1.3 | FORMAT_PING], [0], 
>> [dnl
>> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
>> +])
>> +
>> +sleep 1
>> +AT_CHECK([ovs-appctl revalidator/purge])
>> +
>> +AT_CHECK([test $(ovs-appctl 

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

[Robin Jarry]

Hi Aaron,

Thanks for your feedback.

Aaron Conole, Mar 07, 2023 at 19:57:
> I'm concerned about this - this is a negative interference with rte_flow
> offload.  And rte_flow offload would also just alleviate these problems,
> yes?  Maybe we should encourage a user to just turn on flow offloading?

Indeed this feature will not get along with rte_flow offload, but as you
said, if your hardware and flow pipeline are compatible with rte_flow
offload, then, you probably don't need this feature since your PMD
threads will only handle a limited portion of the data plane traffic.

The CP protection flows are very basic (for LACP, only matching the
ether type, no VLAN support needed, etc.) and they should be supported
by a broad range of NICs (even Intel 82599 Niantic from 2013).

This feature is mostly aimed at pure DPDK (without extended rte_flow
offload support) where LACP is mixed in the same queues and processed by
the same CPUs than data plane traffic.

> Additionally, it doesn't seem to have a good solution for kernel side.

For non-DPDK use cases, I see two separate paths. Please do correct me
if I am wrong:

1) pure kernel (not tested, only from ethtool(8))

   ethtool -L $dev rx $((rxqs + 1))
   ethtool -X $dev equal $((rxqs - 1))
   ethtool -U $dev flow-type ether proto 0x8809 m 0x action $rxqs

2) tc flow offload

   Only mlx5 supported hardware is concerned and there should be direct
   support for hardware bond offload (LACP handled in the NIC).

> And I worry about adding flows into the system that the ofproto layer
> doesn't know about but will interact with - but maybe I'm just being
> paranoid.

There should be no overlap with the ofproto layer. The CP protection
flows only route certain packets into certain RXQs. The packets are
still processed by the same pipeline.

I understand your reservations as this involves intrusive elements in
the packet flow and does not play well with RTE flow offload.

However, actual openflow pipelines (like the ones in OVN deployments)
are supported in RTE flow by a limited and recent set of hardware. Users
with existing deployments and older hardware cannot benefit from this.
This feature could allow making these environments more resilient and
less prone to link flapping.

Also, given that cp-protection is disabled by default, there will be no
conflict with DPDK hw-offload when there is hardware support.

What do you think?

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

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

[Aaron Conole]

Robin Jarry  writes:

> 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.

I'm concerned about this - this is a negative interference with rte_flow
offload.  And rte_flow offload would also just alleviate these problems,
yes?  Maybe we should encourage a user to just turn on flow offloading?

Additionally, it doesn't seem to have a good solution for kernel side.
And I worry about adding flows into the system that the ofproto layer
doesn't know about but will interact with - but maybe I'm just being
paranoid.

> 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 

Re: [ovs-dev] [PATCH v2] dpdk: Allow retaining CAP_SYS_RAWIO privileges

[Aaron Conole]

Flavio Leitner  writes:

> On Tue, Mar 07, 2023 at 02:32:04PM +0100, David Marchand wrote:
>> On Tue, Mar 7, 2023 at 2:06 PM Aaron Conole  wrote:
>> >
>> > 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.
>> >
>> > Reviewed-by: Simon Horman 
>> > Signed-off-by: Aaron Conole 
>> > ---
>> >  NEWS   |  4 
>> >  lib/daemon-unix.c  | 29 +
>> >  lib/daemon-windows.c   |  3 ++-
>> >  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 ++-
>> >  14 files changed, 60 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..a080facddc 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;
>> > @@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
>> > OVS_UNUSED)
>> >  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);
>> > +#ifdef DPDK_NETDEV
>> > +if (access_hardware_ports && !ret) {
>> > +ret = capng_update(CAPNG_ADD, 

Re: [ovs-dev] [PATCH v2] dpdk: Allow retaining CAP_SYS_RAWIO privileges

[Aaron Conole]

David Marchand  writes:

> On Tue, Mar 7, 2023 at 2:06 PM Aaron Conole  wrote:
>>
>> 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.
>>
>> Reviewed-by: Simon Horman 
>> Signed-off-by: Aaron Conole 
>> ---
>>  NEWS   |  4 
>>  lib/daemon-unix.c  | 29 +
>>  lib/daemon-windows.c   |  3 ++-
>>  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 ++-
>>  14 files changed, 60 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..a080facddc 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;
>> @@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
>> OVS_UNUSED)
>>  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);
>> +#ifdef DPDK_NETDEV
>> +if (access_hardware_ports && !ret) {
>> +ret = capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO);
>> +VLOG_INFO("CAP_SYS_RAWIO enabled.");
>> +}
>> +#else
>> +if (access_hardware_ports) {
>> +VLOG_WARN("Dropped CAP_SYS_RAWIO request (no 
>> drivers).");
>
> Does it mean we drop 

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

[Han Zhou]

On Tue, Mar 7, 2023 at 12:38 AM Ales Musil  wrote:
>
>
>
> On Thu, Feb 23, 2023 at 7:35 AM Han Zhou  wrote:
>>
>> 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
>>
>
> The whole series looks good to me, thanks.
>
> Acked-by: Ales Musil 
>
Thanks Ales for reviewing.
Also thanks Simon for reviewing and testing.
I incorporated the minor change for patch 2 below (verified by Simon), also
added some description in patch 2 commit message, and pushed the series to
main.

Thanks,
Han
---
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 92c32087ee23..03dd08ad81dc 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -10797,7 +10797,7 @@ ADD_VETH(ln1, ns_ext1, br-ex, "172.0.0.99/24",
"0a:0a:b6:fc:03:01", "172.0.0.1")
 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])
+NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 10.0.0.88])
 wait_for_ports_up vip
 check ovn-nbctl --wait=hv sync

@@ -10818,7 +10818,7 @@ 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])
-NS_EXEC([ns_ls1p2], [arping -U -c 1 -w 2 -I ls1p2 -s 10.0.0.88 10.0.0.88])
+NS_EXEC([ns_ls1p2], [arping -U -c 1 -w 2 -I ls1p2 10.0.0.88])

 wait_column "ls1p2" Port_Binding virtual_parent logical_port=vip
 check ovn-nbctl --wait=hv sync
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ilya Maximets]

On 3/7/23 16:03, Eelco Chaudron wrote:
> 
> 
> On 6 Mar 2023, at 3:49, Nobuhiro MIKI wrote:
> 
>> v7:
>> - Fix error message
>> v6:
>> - Fix notation quirks between man page and in-app help
>> - Fix error message in ovs/route/add command
>> - Fix coding style
>> 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  |  70 +--
>>  lib/ovs-router.c| 145 
>>  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, 317 insertions(+), 73 deletions(-)
> 
> Thanks for this revision, it fixes all my remaining comments.
> 
> So for clarity, I’ll ack the series explicitly.
> 
> Acked-by: Eelco Chaudron 

Thanks, Nobuhiro, Simon and Eelco!

I fixed a couple of typos and added a small NEWS entry for the last patch.
With that, Applied.

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

Re: [ovs-dev] [PATCH v2 2/2] ci: Run tc offload tests in GitHub Actions.

[Ilya Maximets]

On 2/28/23 16:52, Eelco Chaudron wrote:
> Run "make check-offloads" as part of the GitHub actions tests.
> 
> This test was run 25 times using GitHub actions, and the
> failing rerun test cases where excluded. There are quite some
> first-run failures, but unfortunately, there is no other
> more stable kernel available as a GitHub-hosted runner.
> 
> Did not yet include sanitizers in the run, as it's causing
> the test to run too long >30min and there seems to be (timing)
> issues with some of the tests.
> 
> Signed-off-by: Eelco Chaudron 
> ---
> v2: Added a new test keyword to exclude the failing tests.
> Added some documentation around the keyword usage.
> 
>  .ci/linux-build.sh   |6 +-
>  .github/workflows/build-and-test.yml |   10 +-
>  Documentation/topics/testing.rst |   16 
>  tests/system-offloads-traffic.at |3 +++
>  tests/system-traffic.at  |2 ++
>  5 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index 6394a8137..19ed9796d 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -159,7 +159,7 @@ fi
>  
>  OPTS="${EXTRA_OPTS} ${OPTS} $*"
>  
> -if [ "$TESTSUITE" ]; then
> +if [ "$TESTSUITE" = 'test' ]; then
>  # 'distcheck' will reconfigure with required options.
>  # Now we only need to prepare the Makefile without sparse-wrapped CC.
>  configure_ovs
> @@ -169,6 +169,10 @@ if [ "$TESTSUITE" ]; then
>  TESTSUITEFLAGS=-j4 RECHECK=yes
>  else
>  build_ovs
> +if [ -n "$TESTSUITE" ]; then
> +sudo -E PATH="$PATH" make "$TESTSUITE" TESTSUITEFLAGS="$TEST_OPTS" \
> +RECHECK=yes
> +fi
>  fi
>  
>  exit 0
> diff --git a/.github/workflows/build-and-test.yml 
> b/.github/workflows/build-and-test.yml
> index 86e594bf3..45326b659 100644
> --- a/.github/workflows/build-and-test.yml
> +++ b/.github/workflows/build-and-test.yml
> @@ -17,6 +17,7 @@ jobs:
>OPTS:${{ matrix.opts }}
>SANITIZERS:  ${{ matrix.sanitizers }}
>TESTSUITE:   ${{ matrix.testsuite }}
> +  TEST_OPTS:   ${{ matrix.test_opts }}
>  
>  name: linux ${{ join(matrix.*, ' ') }}
>  runs-on: ubuntu-22.04
> @@ -86,6 +87,10 @@ jobs:
>  m32:  m32
>  opts: --disable-ssl
>  
> +  - compiler: gcc
> +testsuite:check-offloads
> +test_opts:"-k !github_offloads_skip"
> +
>  steps:
>  - name: checkout
>uses: actions/checkout@v3
> @@ -147,7 +152,10 @@ jobs:
>  # So, we're just archiving everything here to avoid any issues.
>  mkdir logs
>  cp config.log ./logs/
> -cp -r ./*/_build/sub/tests/testsuite.* ./logs/ || true
> +sudo chown -R $USER ./tests/testsuite.* \
> +./tests/system-offloads-testsuite.* || true
> +cp -r ./tests/testsuite.* ./logs/ || true

This seems suspicious because distcheck builds and runs tests in a different 
directory.

> +cp -r ./tests/system-offloads-testsuite.* ./logs/ || true
>  tar -czvf logs.tgz logs/
>  
>  - name: upload logs on failure
> diff --git a/Documentation/topics/testing.rst 
> b/Documentation/topics/testing.rst
> index 5f6940b84..d06167ee3 100644
> --- a/Documentation/topics/testing.rst
> +++ b/Documentation/topics/testing.rst
> @@ -114,6 +114,22 @@ And from another window, one can execute ovs-xxx 
> commands like::
>  
>  Once done with investigation, press ENTER to perform cleanup operation.
>  
> +GitHub actions
> +++

Empty line should be here.

> +The OVS GitHub repository also runs some of these unit tests through GitHub
> +actions. These tests are defined in the
> +``ovs/.github/workflows/build-and-test.yml`` file.
> +
> +Based on the GitHub runners available, not all tests will work. In these 
> cases,
> +the AT_KEYWORDS() macro can be used. For example, to skip a
> +``make check-offloads`` test, use the ``github_offloads_skip`` keyword.
> +
> +Only use these keywords if no other way to skip the test is available.

s/skip/fix or skip/ maybe?

> +
> +To see a list of currently skipped tests, you can do something like::
> +
> +$ make check-offloads TESTSUITEFLAGS="-l" | grep -B 1 
> github_offloads_skip
> +
>  .. _testing-coverage:
>  
>  Coverage
> diff --git a/tests/system-offloads-traffic.at 
> b/tests/system-offloads-traffic.at
> index 7558812eb..6ee92746b 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -191,6 +191,7 @@ AT_CLEANUP
>  
>  AT_SETUP([offloads - check interface meter offloading -  offloads disabled])
>  AT_KEYWORDS([dp-meter])
> +AT_KEYWORDS([github_offloads_skip])
>  AT_SKIP_IF([test $HAVE_NC = "no"])
>  OVS_TRAFFIC_VSWITCHD_START()
>  
> @@ -240,6 +241,7 @@ AT_CLEANUP
>  
>  AT_SETUP([offloads - check interface meter offloading -  offloads enabled])
>  AT_KEYWORDS([offload-meter])
> 

Re: [ovs-dev] [PATCH v2 2/2] ci: Run tc offload tests in GitHub Actions.

[Ilya Maximets]

On 3/7/23 17:31, Simon Horman wrote:
> On Tue, Mar 07, 2023 at 04:58:47PM +0100, Eelco Chaudron wrote:
>>
>>
>> On 7 Mar 2023, at 16:32, Eelco Chaudron wrote:
>>
>>> On 3 Mar 2023, at 16:15, Simon Horman wrote:
>>>
 On Tue, Feb 28, 2023 at 04:52:15PM +0100, Eelco Chaudron wrote:
> Run "make check-offloads" as part of the GitHub actions tests.
>
> This test was run 25 times using GitHub actions, and the
> failing rerun test cases where excluded. There are quite some
> first-run failures, but unfortunately, there is no other
> more stable kernel available as a GitHub-hosted runner.
>
> Did not yet include sanitizers in the run, as it's causing
> the test to run too long >30min and there seems to be (timing)
> issues with some of the tests.
>
> Signed-off-by: Eelco Chaudron 

 Hi Eelco,

 perhaps I am missing something obvious,
 but I seem to be getting failures with;

 9: offloads - check_pkt_len action - offloads disabled FAILED 
 (system-offloads-traffic.at:342)

 Also, there does not seem to be enough in the logs to determine why.

 https://github.com/horms/ovs/actions/runs/4323873948/jobs/7549439777#step:12:2847
>>
>>
>> Looking at your GitHub actions for this test (I think I was looking at the 
>> right commit :) It seems you only applied the second patch of the series, 
>> but the first patch updates the ubuntu release, this might be the problem.

FWIW, this change belongs to the second patch.  It's not justified
in the first one.

> 
> Yikes!
> Sorry about that.
> I've fixed up my branch and am running the workflow again.
> 
> https://github.com/horms/ovs/actions/runs/4356340517

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

Re: [ovs-dev] [PATCH v2 2/2] ci: Run tc offload tests in GitHub Actions.

[Simon Horman]

On Tue, Mar 07, 2023 at 04:58:47PM +0100, Eelco Chaudron wrote:
> 
> 
> On 7 Mar 2023, at 16:32, Eelco Chaudron wrote:
> 
> > On 3 Mar 2023, at 16:15, Simon Horman wrote:
> >
> >> On Tue, Feb 28, 2023 at 04:52:15PM +0100, Eelco Chaudron wrote:
> >>> Run "make check-offloads" as part of the GitHub actions tests.
> >>>
> >>> This test was run 25 times using GitHub actions, and the
> >>> failing rerun test cases where excluded. There are quite some
> >>> first-run failures, but unfortunately, there is no other
> >>> more stable kernel available as a GitHub-hosted runner.
> >>>
> >>> Did not yet include sanitizers in the run, as it's causing
> >>> the test to run too long >30min and there seems to be (timing)
> >>> issues with some of the tests.
> >>>
> >>> Signed-off-by: Eelco Chaudron 
> >>
> >> Hi Eelco,
> >>
> >> perhaps I am missing something obvious,
> >> but I seem to be getting failures with;
> >>
> >> 9: offloads - check_pkt_len action - offloads disabled FAILED 
> >> (system-offloads-traffic.at:342)
> >>
> >> Also, there does not seem to be enough in the logs to determine why.
> >>
> >> https://github.com/horms/ovs/actions/runs/4323873948/jobs/7549439777#step:12:2847
> 
> 
> Looking at your GitHub actions for this test (I think I was looking at the 
> right commit :) It seems you only applied the second patch of the series, but 
> the first patch updates the ubuntu release, this might be the problem.

Yikes!
Sorry about that.
I've fixed up my branch and am running the workflow again.

https://github.com/horms/ovs/actions/runs/4356340517
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 2/2] ci: Run tc offload tests in GitHub Actions.

[Eelco Chaudron]

On 7 Mar 2023, at 16:32, Eelco Chaudron wrote:

> On 3 Mar 2023, at 16:15, Simon Horman wrote:
>
>> On Tue, Feb 28, 2023 at 04:52:15PM +0100, Eelco Chaudron wrote:
>>> Run "make check-offloads" as part of the GitHub actions tests.
>>>
>>> This test was run 25 times using GitHub actions, and the
>>> failing rerun test cases where excluded. There are quite some
>>> first-run failures, but unfortunately, there is no other
>>> more stable kernel available as a GitHub-hosted runner.
>>>
>>> Did not yet include sanitizers in the run, as it's causing
>>> the test to run too long >30min and there seems to be (timing)
>>> issues with some of the tests.
>>>
>>> Signed-off-by: Eelco Chaudron 
>>
>> Hi Eelco,
>>
>> perhaps I am missing something obvious,
>> but I seem to be getting failures with;
>>
>> 9: offloads - check_pkt_len action - offloads disabled FAILED 
>> (system-offloads-traffic.at:342)
>>
>> Also, there does not seem to be enough in the logs to determine why.
>>
>> https://github.com/horms/ovs/actions/runs/4323873948/jobs/7549439777#step:12:2847


Looking at your GitHub actions for this test (I think I was looking at the 
right commit :) It seems you only applied the second patch of the series, but 
the first patch updates the ubuntu release, this might be the problem.

>
> Looked at the logs captures and they only show that not all packets passed 
> trough:
>
>   --- -   2023-03-03 14:59:53.139186466 +
>   +++ 
> /home/runner/work/ovs/ovs/tests/system-offloads-testsuite.dir/at-groups/9/stdout
> 2023-03-03 14:59:53.135502663 +
>   @@ -1,2 +1,2 @@
>   -10 1032
>   +2 1032
>
>
> Interesting as it was running fine in my setup;
>
> https://github.com/chaudron/ovs/actions/runs/4294641204/jobs/7483953067
>
>
> Can you do a re-run and see if it’s fine a second run? I’ll do another couple 
> of re-runs and see if it fails in any of them.
>

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

Re: [ovs-dev] [PATCH v2] dpdk: Allow retaining CAP_SYS_RAWIO privileges

[Flavio Leitner]

On Tue, Mar 07, 2023 at 12:24:37PM -0300, Flavio Leitner wrote:
> On Tue, Mar 07, 2023 at 02:32:04PM +0100, David Marchand wrote:
> > On Tue, Mar 7, 2023 at 2:06 PM Aaron Conole  wrote:
> > >
> > > 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.
> > >
> > > Reviewed-by: Simon Horman 
> > > Signed-off-by: Aaron Conole 
> > > ---
> > >  NEWS   |  4 
> > >  lib/daemon-unix.c  | 29 +
> > >  lib/daemon-windows.c   |  3 ++-
> > >  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 ++-
> > >  14 files changed, 60 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..a080facddc 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;
> > > @@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
> > > OVS_UNUSED)
> > >  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, 
> > > 

Re: [ovs-dev] [PATCH v2 2/2] ci: Run tc offload tests in GitHub Actions.

[Eelco Chaudron]

On 3 Mar 2023, at 16:15, Simon Horman wrote:

> On Tue, Feb 28, 2023 at 04:52:15PM +0100, Eelco Chaudron wrote:
>> Run "make check-offloads" as part of the GitHub actions tests.
>>
>> This test was run 25 times using GitHub actions, and the
>> failing rerun test cases where excluded. There are quite some
>> first-run failures, but unfortunately, there is no other
>> more stable kernel available as a GitHub-hosted runner.
>>
>> Did not yet include sanitizers in the run, as it's causing
>> the test to run too long >30min and there seems to be (timing)
>> issues with some of the tests.
>>
>> Signed-off-by: Eelco Chaudron 
>
> Hi Eelco,
>
> perhaps I am missing something obvious,
> but I seem to be getting failures with;
>
> 9: offloads - check_pkt_len action - offloads disabled FAILED 
> (system-offloads-traffic.at:342)
>
> Also, there does not seem to be enough in the logs to determine why.
>
> https://github.com/horms/ovs/actions/runs/4323873948/jobs/7549439777#step:12:2847

Looked at the logs captures and they only show that not all packets passed 
trough:

  --- -   2023-03-03 14:59:53.139186466 +
  +++ 
/home/runner/work/ovs/ovs/tests/system-offloads-testsuite.dir/at-groups/9/stdout
2023-03-03 14:59:53.135502663 +
  @@ -1,2 +1,2 @@
  -10 1032
  +2 1032


Interesting as it was running fine in my setup;

https://github.com/chaudron/ovs/actions/runs/4294641204/jobs/7483953067


Can you do a re-run and see if it’s fine a second run? I’ll do another couple 
of re-runs and see if it fails in any of them.

//Eelco

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

Re: [ovs-dev] [PATCH v2] dpdk: Allow retaining CAP_SYS_RAWIO privileges

[Flavio Leitner]

On Tue, Mar 07, 2023 at 02:32:04PM +0100, David Marchand wrote:
> On Tue, Mar 7, 2023 at 2:06 PM Aaron Conole  wrote:
> >
> > 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.
> >
> > Reviewed-by: Simon Horman 
> > Signed-off-by: Aaron Conole 
> > ---
> >  NEWS   |  4 
> >  lib/daemon-unix.c  | 29 +
> >  lib/daemon-windows.c   |  3 ++-
> >  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 ++-
> >  14 files changed, 60 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..a080facddc 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;
> > @@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
> > OVS_UNUSED)
> >  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);
> > +#ifdef DPDK_NETDEV
> > +if (access_hardware_ports && !ret) {
> > +ret = capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO);
> > +VLOG_INFO("CAP_SYS_RAWIO enabled.");

Perhaps "The Linux capability CAP_SYS_RAWIO is 

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

[Eelco Chaudron]

On 6 Mar 2023, at 3:49, Nobuhiro MIKI wrote:

> v7:
> - Fix error message
> v6:
> - Fix notation quirks between man page and in-app help
> - Fix error message in ovs/route/add command
> - Fix coding style
> 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  |  70 +--
>  lib/ovs-router.c| 145 
>  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, 317 insertions(+), 73 deletions(-)

Thanks for this revision, it fixes all my remaining comments.

So for clarity, I’ll ack the series explicitly.

Acked-by: Eelco Chaudron 

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

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

[Eelco Chaudron]

On 6 Mar 2023, at 3:49, 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.
>
> Acked-by: Eelco Chaudron 
> Reviewed-by: Simon Horman 
> Signed-off-by: Nobuhiro MIKI 
> ---
>  NEWS|  3 ++
>  lib/ovs-router.c| 86 +
>  ofproto/ofproto-tnl-unixctl.man |  5 +-
>  tests/ovs-router.at | 80 +-
>  4 files changed, 161 insertions(+), 13 deletions(-)

Changes look good to me! ACKing additional test script changes due to suggested 
message changes.

Acked-by: Eelco Chaudron 

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

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

[Eelco Chaudron]

On 6 Mar 2023, at 3:49, Nobuhiro MIKI wrote:

> Fixed the manual page to indicate that both IPv4/IPv6
> are supported. Also added missing pkt_mark on one side
> and fixed the "gw" and "bridge" notation quirks.
>
> Reviewed-by: Simon Horman 
> Signed-off-by: Nobuhiro MIKI 
> ---
>  lib/ovs-router.c| 6 +++---
>  ofproto/ofproto-tnl-unixctl.man | 8 
>  tests/ovs-router.at | 2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)

Changes look good to me! Thanks for following through!

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2] dpdk: Allow retaining CAP_SYS_RAWIO privileges

[David Marchand]

On Tue, Mar 7, 2023 at 2:06 PM Aaron Conole  wrote:
>
> 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.
>
> Reviewed-by: Simon Horman 
> Signed-off-by: Aaron Conole 
> ---
>  NEWS   |  4 
>  lib/daemon-unix.c  | 29 +
>  lib/daemon-windows.c   |  3 ++-
>  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 ++-
>  14 files changed, 60 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..a080facddc 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;
> @@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
> OVS_UNUSED)
>  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);
> +#ifdef DPDK_NETDEV
> +if (access_hardware_ports && !ret) {
> +ret = capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO);
> +VLOG_INFO("CAP_SYS_RAWIO enabled.");
> +}
> +#else
> +if (access_hardware_ports) {
> +VLOG_WARN("Dropped CAP_SYS_RAWIO request (no drivers).");

Does it mean we drop the capability? or do we drop the drop?
I don't really have a better wording but the current log had me
reading it a few times.


> +   

Re: [ovs-dev] [PATCH v3 ovn] northd: drop ct.inv packets in post snat and lb_aff_learn stages

[Lorenzo Bianconi]

> On Tue, Mar 07, 2023 at 11:57:16AM +0100, Lorenzo Bianconi wrote:
> > Drop ip packets with ct status set to invalid in post snat and
> > lb_aff_learn router stages.
> > Skip ICMPv{4,6} error messages packet in ct.inv rules in order to avoid
> > to introduce too complicated code.
> > 
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160685
> > Signed-off-by: Lorenzo Bianconi 
> 
> ...
> 
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 2eab2c4ae..cdf9373a6 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> 
> ...
> 
> > @@ -5070,7 +5109,7 @@ clone {
> >
> >  
> >  
> > -Egress Table 5: Delivery
> > +Egress Table 6: Delivery
> >  
> >  
> >Packets that reach this table are ready for delivery.  It contains:
> 
> Can I clarify that the change above is intentional?

yep, because we are missing "Post SNAT" egress table here, so we need to shift
the following ones.

Regards,
Lorenzo

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

[ovs-dev] [PATCH v2] 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.

Reviewed-by: Simon Horman 
Signed-off-by: Aaron Conole 
---
 NEWS   |  4 
 lib/daemon-unix.c  | 29 +
 lib/daemon-windows.c   |  3 ++-
 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 ++-
 14 files changed, 60 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..a080facddc 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;
@@ -827,6 +829,16 @@ daemon_become_new_user_linux(bool access_datapath 
OVS_UNUSED)
 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);
+#ifdef DPDK_NETDEV
+if (access_hardware_ports && !ret) {
+ret = capng_update(CAPNG_ADD, cap_sets, CAP_SYS_RAWIO);
+VLOG_INFO("CAP_SYS_RAWIO enabled.");
+}
+#else
+if (access_hardware_ports) {
+VLOG_WARN("Dropped CAP_SYS_RAWIO request (no drivers).");
+}
+#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 non-root user
  * as specifed by the --user option.  

Re: [ovs-dev] [PATCH v3 ovn] northd: drop ct.inv packets in post snat and lb_aff_learn stages

[Simon Horman]

On Tue, Mar 07, 2023 at 11:57:16AM +0100, Lorenzo Bianconi wrote:
> Drop ip packets with ct status set to invalid in post snat and
> lb_aff_learn router stages.
> Skip ICMPv{4,6} error messages packet in ct.inv rules in order to avoid
> to introduce too complicated code.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160685
> Signed-off-by: Lorenzo Bianconi 

...

> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 2eab2c4ae..cdf9373a6 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml

...

> @@ -5070,7 +5109,7 @@ clone {
>
>  
>  
> -Egress Table 5: Delivery
> +Egress Table 6: Delivery
>  
>  
>Packets that reach this table are ready for delivery.  It contains:

Can I clarify that the change above is intentional?

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

[ovs-dev] [PATCH v3 ovn] northd: drop ct.inv packets in post snat and lb_aff_learn stages

[Lorenzo Bianconi]

Drop ip packets with ct status set to invalid in post snat and
lb_aff_learn router stages.
Skip ICMPv{4,6} error messages packet in ct.inv rules in order to avoid
to introduce too complicated code.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2160685
Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- rebase on top of ovn main branch
- cosmetics
Changes since v1:
- skip ICMPv{4,6} error messages packet in ct.inv rules
- this series is based on the following series not yet applied in ovn master:
  https://patchwork.ozlabs.org/project/ovn/list/?series=343841
---
 northd/northd.c | 30 -
 northd/ovn-northd.8.xml | 43 +++--
 tests/ovn-northd.at | 13 +
 tests/ovn.at| 60 +
 tests/system-ovn.at | 16 +--
 5 files changed, 122 insertions(+), 40 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 7ad4cdfad..a8ea04bf8 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13825,6 +13825,31 @@ build_lrouter_out_is_dnat_local(struct hmap *lflows, 
struct ovn_datapath *od,
 >header_);
 }
 
+static void
+build_lrouter_drop_ct_inv_flow(struct ovn_datapath *od, struct hmap *lflows)
+{
+if (!od->nbr) {
+return;
+}
+
+ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 0, "1", "next;");
+
+if (use_ct_inv_match) {
+ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 150,
+  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
+  " (ip6 && icmp6.type == 2 && icmp6.code == 0))",
+  "next;");
+ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 100,
+  "ip && ct.trk && ct.inv", debug_drop_action());
+ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 250,
+  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
+  " (ip6 && icmp6.type == 2 && icmp6.code == 0))",
+  "next;");
+ovn_lflow_add(lflows, od, S_ROUTER_IN_LB_AFF_LEARN, 200,
+  "ip && ct.trk && ct.inv", debug_drop_action());
+}
+}
+
 static void
 build_lrouter_out_snat_flow(struct hmap *lflows, struct ovn_datapath *od,
 const struct nbrec_nat *nat, struct ds *match,
@@ -14213,7 +14238,6 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
 ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 0, "1", "next;");
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 0, "1", "next;");
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 0, "1", "next;");
-ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_SNAT, 0, "1", "next;");
 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;");
 
@@ -14267,6 +14291,9 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath 
*od, struct hmap *lflows,
  * flag set. Some NICs are unable to offload these flows.
  */
 if (od->is_gw_router && (od->nbr->n_nat || od->has_lb_vip)) {
+/* Do not send ND or ICMP packets to connection tracking. */
+ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 100,
+  "nd || nd_rs || nd_ra", "next;");
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_UNDNAT, 50,
   "ip", "flags.loopback = 1; ct_dnat;");
 ovn_lflow_add(lflows, od, S_ROUTER_OUT_POST_UNDNAT, 50,
@@ -14591,6 +14618,7 @@ build_lswitch_and_lrouter_iterate_by_od(struct 
ovn_datapath *od,
 build_lrouter_nat_defrag_and_lb(od, lsi->lflows, lsi->ports, >match,
 >actions, lsi->meter_groups,
 lsi->features);
+build_lrouter_drop_ct_inv_flow(od, lsi->lflows);
 build_lb_affinity_default_flows(od, lsi->lflows);
 }
 
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 2eab2c4ae..cdf9373a6 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -3639,6 +3639,17 @@ icmp6 {
 
 
 
+  
+For ICMPv4/ICMPv6 packet too big traffic, a priority-250 flow with
+action next;.
+  
+
+  
+If use_ct_inv_match is set, a priority-200 flow
+matches ip  ct.trk  ct.inv with
+action drop;.
+  
+
   
 For all the configured load balancing rules for a logical router where
 a positive affinity timeout T is specified in options
@@ -4721,6 +4732,11 @@ nd_ns {
 Egress Table 1: UNDNAT on Gateway Routers
 
 
+  
+For IPv6 Neighbor Discovery or Router Solicitation/Advertisement
+traffic, a priority-100 flow with action next;.
+  
+
   
 For all IP packets, a priority-50 flow with an action
 flags.loopback = 1; ct_dnat;.
@@ -4998,7 +5014,30 @@ nd_ns {
   
 
 
-Egress Table 4: Egress Loopback
+Egress Table 4: Post SNAT
+
+
+  

Re: [ovs-dev] [PATCH ovn v3] northd, lsp: Add additional arp proxy features

[Ales Musil]

On Tue, Feb 28, 2023 at 11:25 AM Enrique Llorente 
wrote:

> Configure mac address
> The mac address returned by ARP/NDP can be configured similar to LSP
> addresses where the mac is the first entry on the list
>
> IPv6
> Support NDP IPv6 protocol
>
> Use CIDRs
> Allow to specify subnets for ipv4 and ipv6, they will match whatever
> address is received from ARP/NDP
>
> Signed-off-by: Enrique Llorente 
> ---
>  northd/northd.c   | 135 ++---
>  northd/ovn-northd.8.xml   |   9 ++
>  ovn-nb.xml|  17 ++-
>  tests/ovn.at  | 179 ++
>  tests/system-common-macros.at |   1 +
>  tests/system-ovn.at   | 270 ++
>  6 files changed, 556 insertions(+), 55 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 770a5b50e..c8118d2ba 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8644,29 +8644,43 @@ build_lswitch_arp_nd_responder_known_ips(struct
> ovn_port *op,
>  }
>  }
>  }
> -
> -if (op->peer) {
> -const char *arp_proxy =
> smap_get(>nbsp->options,"arp_proxy");
> -
> +const char *arp_proxy = smap_get(>nbsp->options,"arp_proxy");
> +if (arp_proxy) {
>  struct lport_addresses proxy_arp_addrs;
> -int i = 0;
> +int i, ofs = 0;
> +/* Either takes "MAC IP1 IP2" or "IP1 IP2" */
> +if (!extract_addresses(arp_proxy, _arp_addrs, ) &&
> +!extract_ip_addresses(arp_proxy, _arp_addrs)) {
> +static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> +VLOG_WARN_RL(, "Invalid arp_proxy option: '%s' at lsp
> '%s'",
> + arp_proxy, op->nbsp->name);
> +return;
> +}
> +
> +/* Select the mac address to answer the proxy ARP/NDP */
> +char *ea_s = NULL;
> +if (!eth_addr_is_zero(proxy_arp_addrs.ea)) {
> +ea_s = proxy_arp_addrs.ea_s;
> +} else if (op->peer) {
> +ea_s = op->peer->lrp_networks.ea_s;
> +} else {
> +return;
> +}
>
> -/* Add responses for ARP proxies. */
> -if (arp_proxy && extract_ip_addresses(arp_proxy,
> -  _arp_addrs) &&
> -proxy_arp_addrs.n_ipv4_addrs) {
> +/* Add IPv4 responses for ARP proxies. */
> +if (proxy_arp_addrs.n_ipv4_addrs) {
>  /* Match rule on all proxy ARP IPs. */
>  ds_clear(match);
>  ds_put_cstr(match, "arp.op == 1 && arp.tpa == {");
>
>  for (i = 0; i < proxy_arp_addrs.n_ipv4_addrs; i++) {
> -ds_put_format(match, "%s,",
> -  proxy_arp_addrs.ipv4_addrs[i].addr_s);
> +ds_put_format(match, "%s/%u,",
> +  proxy_arp_addrs.ipv4_addrs[i].addr_s,
> +  proxy_arp_addrs.ipv4_addrs[i].plen);
>  }
>
>  ds_chomp(match, ',');
>  ds_put_cstr(match, "}");
> -destroy_lport_addresses(_arp_addrs);
>
>  ds_clear(actions);
>  ds_put_format(actions,
> @@ -8679,12 +8693,66 @@ build_lswitch_arp_nd_responder_known_ips(struct
> ovn_port *op,
>  "outport = inport; "
>  "flags.loopback = 1; "
>  "output;",
> -op->peer->lrp_networks.ea_s,
> -op->peer->lrp_networks.ea_s);
> +ea_s,
> +ea_s);
>
>  ovn_lflow_add_with_hint(lflows, op->od,
> S_SWITCH_IN_ARP_ND_RSP,
>  50, ds_cstr(match), ds_cstr(actions),
> >nbsp->header_);
>  }
> +
> +/* Add IPv6 NDP responses.
> + * For ND solicitations, we need to listen for both the
> + * unicast IPv6 address and its all-nodes multicast address,
> + * but always respond with the unicast IPv6 address. */
> +if (proxy_arp_addrs.n_ipv6_addrs) {
> +struct ds ip6_dst_match = DS_EMPTY_INITIALIZER;
> +struct ds nd_target_match = DS_EMPTY_INITIALIZER;
> +for (size_t j = 0; j < proxy_arp_addrs.n_ipv6_addrs; j++)
> {
> +ds_put_format(_dst_match, "%s/%u, %s/%u, ",
> +proxy_arp_addrs.ipv6_addrs[j].addr_s,
> +proxy_arp_addrs.ipv6_addrs[j].plen,
> +proxy_arp_addrs.ipv6_addrs[j].sn_addr_s,
> +proxy_arp_addrs.ipv6_addrs[j].plen);
> +ds_put_format(_target_match, "%s/%u, ",
> +proxy_arp_addrs.ipv6_addrs[j].addr_s,
> +

Re: [ovs-dev] [PATCH ovn] rhel: pass options to stop daemon command in systemd units

[Ales Musil]

On Mon, Feb 27, 2023 at 7:41 PM Vladislav Odintsov 
wrote:

> This patch fixes ovn-northd.service and ovn-db@.service systemd units
> which didn't pass startup options to ovn-ctl script while stop_* call.
>
> For instance if ovn-northd service was started with option
> '--ovn-manage-ovsdb=no' and NB/SB databases are located on the same host
> and started by ovn-db@nb / ovn-db@sb or manually with ovn-ctl, so
> ovsdb-server processes for these  services will be unexpectedly stopped.
>
> Fixes: 497ec3fdfbdb ("rhel: add ovn-db@.service systemd-unit")
> Signed-off-by: Vladislav Odintsov 
> ---
>  rhel/usr_lib_systemd_system_ovn-db@.service| 2 +-
>  rhel/usr_lib_systemd_system_ovn-northd.service | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/rhel/usr_lib_systemd_system_ovn-db@.service
> b/rhel/usr_lib_systemd_system_ovn-db@.service
> index 98556a673..c835e4967 100644
> --- a/rhel/usr_lib_systemd_system_ovn-db@.service
> +++ b/rhel/usr_lib_systemd_system_ovn-db@.service
> @@ -33,7 +33,7 @@ EnvironmentFile=-/etc/sysconfig/ovn-%i
>  ExecStartPre=-/usr/bin/chown -R ${OVN_USER_ID} ${OVN_DBDIR}
>  ExecStart=/usr/share/ovn/scripts/ovn-ctl \
>--ovn-user=${OVN_USER_ID} start_%i_ovsdb $OPTIONS $ovn_%i_opts
> -ExecStop=/usr/share/ovn/scripts/ovn-ctl stop_%i_ovsdb
> +ExecStop=/usr/share/ovn/scripts/ovn-ctl stop_%i_ovsdb $OPTIONS
> $ovn_%i_opts
>
>  [Install]
>  WantedBy=multi-user.target
> diff --git a/rhel/usr_lib_systemd_system_ovn-northd.service
> b/rhel/usr_lib_systemd_system_ovn-northd.service
> index d281f861c..6c4c6621c 100644
> --- a/rhel/usr_lib_systemd_system_ovn-northd.service
> +++ b/rhel/usr_lib_systemd_system_ovn-northd.service
> @@ -26,7 +26,7 @@ EnvironmentFile=-/etc/sysconfig/ovn-northd
>  ExecStartPre=-/usr/bin/chown -R ${OVN_USER_ID} ${OVN_DBDIR}
>  ExecStart=/usr/share/ovn/scripts/ovn-ctl \
>--ovn-user=${OVN_USER_ID} start_northd $OVN_NORTHD_OPTS
> -ExecStop=/usr/share/ovn/scripts/ovn-ctl stop_northd
> +ExecStop=/usr/share/ovn/scripts/ovn-ctl stop_northd $OVN_NORTHD_OPTS
>
>  [Install]
>  WantedBy=multi-user.target
> --
> 2.36.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [External] Re: [PATCH v4] utilities/ofctl: add-meters for save and restore

[Simon Horman]

On Tue, Mar 07, 2023 at 12:24:55PM +0800, Wan Junjie wrote:
> Hi Simon Horman,
> 
> Thanks for your review.
> 
> 
> On Mon, Mar 6, 2023 at 11:23 PM Simon Horman  
> wrote:
> > On Sat, Mar 04, 2023 at 11:23:50AM +0800, Wan Junjie wrote:
...
> > > On Fri, Mar 3, 2023 at 11:06 PM Simon Horman  
> > > wrote:
> > > > On Wed, Mar 01, 2023 at 04:05:17PM +0800, Wan Junjie wrote:

...

> > > > > +struct ofputil_meter_config *mcs;
> > > > > +size_t n_mtrs;
> > > > > +run(vconn_dump_meters(vconn, request, , _mtrs),
> > > > > +"dump meters");
> > > > > +
> > > > > +struct ds s = DS_EMPTY_INITIALIZER;
> > > > > +for (size_t i = 0; i < n_mtrs; i ++) {
> > > > > +ds_clear();
> > > > > +ofputil_format_meter_config(, [i], 1);
> > > > > +printf("%s\n", ds_cstr());
> > > > > +}
> > > > > +ds_destroy();
> > > > > +
> > > > > +for (size_t i = 0; i < n_mtrs; i ++) {
> > > > > +free(CONST_CAST(struct ofputil_meter_band *, 
> > > > > mcs[i].bands));
> > > > > +}
> > > > > +free(mcs);
> > > >
> > > > The above, ofputil_format_meter_config(), and recv_meter_stats_reply()
> > > > seems to be a lot of code to customise what was otherwise a call to
> > > > dump_transaction().
> > > >
> > > > Perhaps it would be cleaner to parameterise ofp_print() over 'oneline'.
> > > >
> > > That was my first thought, then I realize that I would have to add
> > > this parameter to several
> > > functions recursively, like dump_transaction(), ofp_print(),
> > > ofp_to_string__(), ofp_to_string().
> > > I don't think these functions really need that parameter since only
> > > meter has compatibility issues.
> >
> > I understand your point. But I think it would be better to teach the
> > core/common code how to handle this and avoid open coding the special case.
> >
> > Perhaps a 'oneline' parameter isn't the best approach.
> > But really one bit of information is needed in order
> > for core/common code to implement a (slightly) different behaviour.
> >
> > And it is conceivable that other dump functions will need
> > alternate behaviour in future.
> >
> OK, I will see if I can merge it to 'verbosity'.

Thanks. I agree that overloading/reusing the verbosity parameter
is worth a shot.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ales Musil]

On Thu, Feb 23, 2023 at 7:35 AM Han Zhou  wrote:

> 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
>
>
The whole series looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH ovn] controller: Use ofctrl_add_flow for CT SNAT hairpin flows.

[Ales Musil]

On Sat, Mar 4, 2023 at 12:03 AM Ilya Maximets  wrote:

> We no longer use conjunctions for these flows, so no need to
> try to append them.
>
> Fixes: c2f968235241 ("controller: Fix hairpin SNAT flow explosion if
> hairpin_snat_ip is set.")
> Signed-off-by: Ilya Maximets 
> ---
>  controller/lflow.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 6a98b19e1..003195ae4 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1746,11 +1746,9 @@ add_lb_ct_snat_hairpin_for_dp(const struct
> ovn_controller_lb *lb,
>   * datapath match, but it will also match on the less restrictive
>   * general case.  Therefore, we set the priority in the
>   * "hairpin_snat_ip" case to be higher than the general case. */
> -ofctrl_add_or_append_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN,
> -  datapath ? 200 : 100,
> -  lb->slb->header_.uuid.parts[0],
> -  dp_match, dp_acts, >slb->header_.uuid,
> -  NX_CTLR_NO_METER, NULL);
> +ofctrl_add_flow(flow_table, OFTABLE_CT_SNAT_HAIRPIN,
> +datapath ? 200 : 100, lb->slb->header_.uuid.parts[0],
> +dp_match, dp_acts, >slb->header_.uuid);
>  }
>
>  /* Add a ct_snat flow for each VIP of the LB.  If this LB does not use
> --
> 2.39.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

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

Re: [ovs-dev] [PATCH ovn v2] northd: prevents sending packet to conntrack for router ports

[Ales Musil]

On Mon, Mar 6, 2023 at 12:07 PM Xavier Simonart  wrote:

> As commented in northd.c, we should not use ct() for router ports.
> When there are no stateful_acl, this patch prevents sending packet to
> conntrack
> for router ports.
> The patch does this by issuing ct_clear in ls_out_pre_lb stage so that
> hints
> are not set in ls_out_acl_hint and ls_out_acl stages.
>
> Note that ct_clear is not added for ingress for router ports as already
> done
> for patch ports (no change by this patch on this aspect).
>
> Also, this patch does not change the behavior for ACLs such as
> allow-related:
> packets are still sent to conntrack, even for router ports. While this does
> not work if router ports are distributed, allow-related ACLs work today on
> router ports when those ports are handled on the same chassis for ingress
> and
> egress traffic. This patch does not change that behavior.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431
>
> Signed-off-by: Xavier Simonart 
>
> ---
> v2: - handled Dumitru's comments
> - handled Ales' comments
> - added change to xml documentation
> - do not do ct_clear for ingress as already done
> ---
>  northd/northd.c |  24 +++---
>  northd/ovn-northd.8.xml |  11 +++
>  tests/ovn-northd.at |  11 +--
>  tests/system-ovn.at | 166 
>  4 files changed, 197 insertions(+), 15 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 7ad4cdfad..b356faf64 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5779,20 +5779,24 @@ skip_port_from_conntrack(struct ovn_datapath *od,
> struct ovn_port *op,
>   * know about the connection, as the icmp request went through the
> logical
>   * router on hostA, not hostB. This would only work with distributed
>   * conntrack state across all chassis. */
> -struct ds match_in = DS_EMPTY_INITIALIZER;
> -struct ds match_out = DS_EMPTY_INITIALIZER;
>
> -ds_put_format(_in, "ip && inport == %s", op->json_key);
> -ds_put_format(_out, "ip && outport == %s", op->json_key);
> +const char *ingress_action = "next;";
> +const char *egress_action = od->has_stateful_acl
> +? "next;"
> +: "ct_clear; next;";
> +
> +char *ingress_match = xasprintf("ip && inport == %s", op->json_key);
> +char *egress_match = xasprintf("ip && outport == %s", op->json_key);
> +
>  ovn_lflow_add_with_lport_and_hint(lflows, od, in_stage, priority,
> -  ds_cstr(_in), "next;",
> op->key,
> -  >nbsp->header_);
> +  ingress_match, ingress_action,
> +  op->key, >nbsp->header_);
>  ovn_lflow_add_with_lport_and_hint(lflows, od, out_stage, priority,
> -  ds_cstr(_out), "next;",
> op->key,
> -  >nbsp->header_);
> +  egress_match, egress_action,
> +  op->key, >nbsp->header_);
>
> -ds_destroy(_in);
> -ds_destroy(_out);
> +free(ingress_match);
> +free(egress_match);
>  }
>
>  static void
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 2eab2c4ae..efadfe808 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2056,6 +2056,17 @@ output;
>db="OVN_Northbound"/> table.
>  
>
> +
> +  This table also has a priority-110 flow with the match
> +  outport == I for all logical switch
> +  datapaths to move traffic to the next table, and, if there are no
> +  stateful_acl, clear the ct_state. Where I
> +  is the peer of a logical router port. This flow is added to
> +  skip the connection tracking of packets which will be entering
> +  logical router datapath from logical switch datapath for routing.
> +
> +
> +
>  Egress Table 2: Pre-stateful
>
>  
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 3fa02d2b3..d0f6893e9 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -4111,6 +4111,7 @@ check ovn-nbctl lsp-set-options sw0-lr0
> router-port=lr0-sw0
>  check ovn-nbctl --wait=sb sync
>
>  check_stateful_flows() {
> +action=$1
>  ovn-sbctl dump-flows sw0 > sw0flows
>  AT_CAPTURE_FILE([sw0flows])
>
> @@ -4144,12 +4145,12 @@ check_stateful_flows() {
>table=??(ls_in_stateful ), priority=100  , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
>  ])
>
> -AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
> +AT_CHECK_UNQUOTED([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
>table=1 (ls_out_pre_lb  ), priority=0, match=(1), action=(next;)
>table=1 (ls_out_pre_lb  ), priority=100  , match=(ip),
> action=(reg0[[2]] = 1; next;)
>table=1