Re: [ovs-dev] [PATCH ovn v3] controller: add ovn-set-local-ip option

[Han Zhou]

On Fri, Feb 18, 2022 at 10:38 AM Vladislav Odintsov 
wrote:
>
> When transport node has multiple interfaces (vlans) and
> ovn-encap-ip on different hosts need to be configured
> from different VLANs source IP for encapsulated packet
> can be not the same, which is expected by remote system.
>
> Explicitely setting local_ip resolves such problem.
>
> Signed-off-by: Vladislav Odintsov 
> ---
>  controller/encaps.c | 43 +
>  controller/ovn-controller.8.xml |  7 ++
>  tests/ovn-controller.at |  9 +++
>  3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index 66e0cd8cd..8e6d290c1 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -23,6 +23,7 @@
>  #include "openvswitch/vlog.h"
>  #include "lib/ovn-sb-idl.h"
>  #include "ovn-controller.h"
> +#include "smap.h"
>
>  VLOG_DEFINE_THIS_MODULE(encaps);
>
> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
>  smap_add(, "dst_port", dst_port);
>  }
>
> +const struct ovsrec_open_vswitch *cfg =
> +ovsrec_open_vswitch_table_first(ovs_table);
> +
> +bool set_local_ip = false;
> +if (cfg) {
> +/* If the tos option is configured, get it */
> +const char *encap_tos = smap_get_def(>external_ids,
> +   "ovn-encap-tos", "none");
> +
> +if (encap_tos && strcmp(encap_tos, "none")) {
> +smap_add(, "tos", encap_tos);
> +}
> +
> +/* If ovn-set-local-ip option is configured, get it */
> +set_local_ip = smap_get_bool(>external_ids,
"ovn-set-local-ip",
> + false);
> +}
> +
>  /* Add auth info if ipsec is enabled. */
>  if (sbg->ipsec) {
> +set_local_ip = true;
> +smap_add(, "remote_name", new_chassis_id);
> +}
> +
> +if (set_local_ip) {
>  const struct sbrec_chassis *this_chassis = tc->this_chassis;
>  const char *local_ip = NULL;
>
> @@ -187,8 +211,10 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
>   */
>  for (int i = 0; i < this_chassis->n_encaps; i++) {
>  if (local_ip && strcmp(local_ip,
this_chassis->encaps[i]->ip)) {
> -VLOG_ERR("ovn-encap-ip has been configured as a list.
This "
> - "is unsupported for IPsec.");
> +static struct vlog_rate_limit rl =
VLOG_RATE_LIMIT_INIT(5, 1);
> +VLOG_ERR_RL(, "ovn-encap-ip has been configured as a
list. "
> +"This is unsupported for IPsec and explicit "
> +"local_ip configuration.");
>  /* No need to loop further as we know this condition has
been
>   * hit */
>  break;
> @@ -200,19 +226,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct
sbrec_sb_global *sbg,
>  if (local_ip) {
>  smap_add(, "local_ip", local_ip);
>  }
> -smap_add(, "remote_name", new_chassis_id);
> -}
> -
> -const struct ovsrec_open_vswitch *cfg =
> -ovsrec_open_vswitch_table_first(ovs_table);
> -/* If the tos option is configured, get it */
> -if (cfg) {
> -const char *encap_tos = smap_get_def(>external_ids,
> -   "ovn-encap-tos", "none");
> -
> -if (encap_tos && strcmp(encap_tos, "none")) {
> -smap_add(, "tos", encap_tos);
> -}
>  }
>
>  /* If there's an existing chassis record that does not need any
change,
> diff --git a/controller/ovn-controller.8.xml
b/controller/ovn-controller.8.xml
> index e9708fe64..cc9a7d1c2 100644
> --- a/controller/ovn-controller.8.xml
> +++ b/controller/ovn-controller.8.xml
> @@ -304,6 +304,13 @@
>  of how many entries there are in the cache.  By default this is
set to
>  3 (30 seconds).
>
> +  external_ids:ovn-set-local-ip
> +  
> +The boolean flag indicates if ovn-controller when
create
> +tunnel ports should set local_ip parameter.  Can be
> +heplful to pin source outer IP for the tunnel when multiple
interfaces
> +are used on the host for overlay traffic.
> +  
>  
>
>  
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index e99eec1d6..89ae2c9e1 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve])
>  ovs-vsctl del-port ovn-fakech-0
>  OVS_WAIT_UNTIL([check_tunnel_property type geneve])
>
> +# set `ovn-set-local-ip` option to true and check if tunnel parameters
> +OVS_WAIT_WHILE([check_tunnel_property options:local_ip
"\"192.168.0.1\""])
> +ovs-vsctl set open . external_ids:ovn-set-local-ip=true
> +OVS_WAIT_UNTIL([check_tunnel_property options:local_ip
"\"192.168.0.1\""])
> +
> +# Change the local_ip on the OVS side and check than OVN fixes it
> +ovs-vsctl set 

Re: [ovs-dev] [ovs-dev v7] ofproto: add refcount to ofproto to fix ofproto use-after-free

[0-day Robot]

Bleep bloop.  Greetings Peng He, 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: Author Peng He  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Peng He , guohongzhi 

Lines checked: 373, Warnings: 1, 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] [ovs-dev v7] ofproto: add refcount to ofproto to fix ofproto use-after-free

[Peng He]

From hepeng:
https://patchwork.ozlabs.org/project/openvswitch/patch/20200717015041.82746-1-hepeng.0...@bytedance.com/#2487473

also from guohongzhi :
http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.19884-1-guohongz...@huawei.com/

also from a discussion about the mixing use of RCU and refcount in the mail
list with Ilya Maximets, William Tu, Ben Pfaf, and Gaëtan Rivet.

A summary, as quoted from Ilya:

"
RCU for ofproto was introduced for one
and only one reason - to avoid freeing ofproto while rules are still
alive.  This was done in commit f416c8d61601 ("ofproto: RCU postpone
rule destruction.").  The goal was to allow using rules without
refcounting them within a single grace period.  And that forced us
to postpone destruction of the ofproto for a single grace period.
Later commit 39c9459355b6 ("Use classifier versioning.") made it
possible for rules to be alive for more than one grace period, so
the commit made ofproto wait for 2 grace periods by double postponing.
As we can see now, that wasn't enough and we have to wait for more
than 2 grace periods in certain cases.
"

In a short, the ofproto should have a longer life time than rule, if
the rule lasts for more than 2 grace periods, the ofproto should live
longer to ensure rule->ofproto is valid. It's hard to predict how long
a ofproto should live, thus we need to use refcount on ofproto to make
things easy. The controversial part is that we have already used RCU postpone
to delay ofproto destrution, if we have to add refcount, is it simpler to
use just refcount without RCU postpone?

IMO, I think going back to the pure refcount solution is more
complicated than mixing using both.

Gaëtan Rive asks some questions on guohongzhi's v2 patch:

during ofproto_rule_create, should we use ofproto_ref
or ofproto_try_ref? how can we make sure the ofproto is alive?

By using RCU, ofproto has three states:

state 1: alive, with refcount >= 1
state 2: dying, with refcount == 0, however pointer is valid
state 3: died, memory freed, pointer might be dangling.

Without using RCU, there is no state 2, thus, we have to be very careful
every time we see a ofproto pointer. In contrast, with RCU, we can be sure
that it's alive at least in this grace peroid, so we can just check if
it is dying by ofproto_try_ref.

This shows that by mixing use of RCU and refcount we can save a lot of work
worrying if ofproto is dangling.

In short, the RCU part makes sure the ofproto is alive when we use it,
and the refcount part makes sure it lives longer enough.

In this patch, I have merged guohongzhi's patch and mine, and fixes
accoring to the previous comments.

v4->v5:
* fix the commits, remove the ref to wangyunjian's patch and
remove the comments describing the previous ofproto destruction code.
* fix group alloc leak issues.

v5->v6:
* fix ofproto unref leak
* fix comments

v6->v7:
* fix comments and typo

Signed-off-by: Peng He 
Signed-off-by: guohongzhi 
Acked-by: Mike Pattrick 
Acked-by: Gaetan Rivet 
---
 ofproto/ofproto-dpif-xlate-cache.c |  2 +
 ofproto/ofproto-dpif-xlate-cache.h |  5 +--
 ofproto/ofproto-dpif-xlate.c   | 14 +++---
 ofproto/ofproto-dpif.c | 24 +-
 ofproto/ofproto-provider.h |  2 +
 ofproto/ofproto.c  | 71 +++---
 ofproto/ofproto.h  |  4 ++
 7 files changed, 98 insertions(+), 24 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate-cache.c 
b/ofproto/ofproto-dpif-xlate-cache.c
index dcc91cb38..9224ee2e6 100644
--- a/ofproto/ofproto-dpif-xlate-cache.c
+++ b/ofproto/ofproto-dpif-xlate-cache.c
@@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
 {
 switch (entry->type) {
 case XC_TABLE:
+ofproto_unref(&(entry->table.ofproto->up));
 break;
 case XC_RULE:
 ofproto_rule_unref(>rule->up);
@@ -231,6 +232,7 @@ xlate_cache_clear_entry(struct xc_entry *entry)
 free(entry->learn.ofm);
 break;
 case XC_NORMAL:
+ofproto_unref(&(entry->normal.ofproto->up));
 break;
 case XC_FIN_TIMEOUT:
 /* 'u.fin.rule' is always already held as a XC_RULE, which
diff --git a/ofproto/ofproto-dpif-xlate-cache.h 
b/ofproto/ofproto-dpif-xlate-cache.h
index 114aff8ea..0fc6d2ea6 100644
--- a/ofproto/ofproto-dpif-xlate-cache.h
+++ b/ofproto/ofproto-dpif-xlate-cache.h
@@ -61,9 +61,8 @@ enum xc_type {
  * that a flow relates to, although they may be used for other effects as well
  * (for instance, refreshing hard timeouts for learned flows).
  *
- * An explicit reference is taken to all pointers other than the ones for
- * struct ofproto_dpif.  ofproto_dpif pointers are explicitly protected by
- * destroying all xlate caches before the ofproto is destroyed. */
+ * An explicit reference is taken to all pointers.
+ */
 struct xc_entry {
 enum xc_type type;
 union {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 6fb59e170..129cdf714 100644
--- 

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

Hi Numan, Han,

please check this out (v3):
https://patchwork.ozlabs.org/project/ovn/patch/20220218183814.2976667-1-odiv...@gmail.com/

Regards,
Vladislav Odintsov

> On 18 Feb 2022, at 20:16, Han Zhou  wrote:
> 
> On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique  wrote:
>> 
>> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou  wrote:
>>> 
>>> On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
 
 On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
>>> wrote:
> 
> Hi Han,
> 
> thanks for the note about log message, I’ll fix this in v3 right
> after
>>> the question with other_config is resolved.
> Frankly speaking I also don’t understand why to sync
> ovn-set-local-ip
>>> option to other_config —
> I did this only because Numan asked to do :)
> 
> Regards,
> Vladislav Odintsov
> 
>> On 17 Feb 2022, at 09:39, Han Zhou  wrote:
>> 
>> On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique >> > wrote:
>>> 
>>> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <
> odiv...@gmail.com>
>> wrote:
 
 When transport node has multiple interfaces (vlans) and
 ovn-encap-ip on different hosts need to be configured
 from different VLANs source IP for encapsulated packet
 can be not the same, which is expected by remote system.
 
 Explicitely setting local_ip resolves such problem.
 
 Signed-off-by: Vladislav Odintsov 
>>> 
>>> Hi Vladislav,
>>> 
>>> Can you please add the code to copy the new added config from OVS
>>> database to the
>>> other_config column of Chassis table in Southbound db ?
>>> 
>>> Please take a look at "struct ovs_chassis_cfg" in
>>> controller/chassis.c
>>> 
>>> Thanks
>>> Numan
>> 
>> Hi Numan,
>> 
>> May I ask what's the purpose of adding this to other_config in
> SB? I
>> understand that there are fields in other_config that is of
>>> importance for
>> other chassises, so need to be added to SB, but for this one, how
>>> would it
>> be used in SB DB?
 
 My understanding is that we just clone all the local ovs configs into
 chassis's other_config and my suggestion was to
 make sure we are consistent with the present behaviour.  Have we
 missed copying some of the ovs configs ?
 
 I'm actually fine either way.  If you think it's better not to copy to
 the sb db, then I am fine with it.
 
>>> 
>>> Thanks Numan for the confirmation. I was wondering if I missed any
>>> important use case of the OVS configs being stored in SB, now it seems
> it
>>> is fine not adding them. There are in fact many OVS configs not in SB,
> such
>>> as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval,
>>> ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally
>>> important, so I don't think it is necessary to sync them to SB. Probably
>>> there were historical reasons why some of the configs were synced to SB
>>> while they were useful only locally, and I can't recall all the details.
>>> Maybe we can remove the unnecessary ones from SB in a separate patch to
>>> reduce the noise of the SB chassis table, not urgent though.
>> 
>> Sounds good to me.
>> 
>> Does @Vladislav Odintsov  need to submit another version reverting the
>> change I requested ?
>> 
> I think Vladislav mentioned that he will submit v3 to fix the error log,
> maybe he could revert the other-config change in v3, too.
> 
>> Numan
>> 
>>> 
>>> Thanks,
>>> Han
>>> 
 Numan
 
>> Hi Vladislav,
>> 
>> Regarding this:
>>   VLOG_ERR("ovn-encap-ip has been configured as a
> list.
>>> This "
>>"is unsupported for IPsec.");
>> 
>> Before your change it applies to IPsec only, but with your change
> this
>> would apply to non-IPsec as well, if ovn-set-local-ip is true.
> Could
>>> you
>> update the error log as well? (it is better to be ratelimited,
> but it
>>> is
>> not the fault of your patch)
>> 
>> Thanks,
>> Han
>> 
>>> 
 ---
 controller/encaps.c | 37
>>> +
 controller/ovn-controller.8.xml |  7 +++
 tests/ovn-controller.at |  9 
 3 files changed, 40 insertions(+), 13 deletions(-)
 
 diff --git a/controller/encaps.c b/controller/encaps.c
 index 66e0cd8cd..3b0c92931 100644
 --- a/controller/encaps.c
 +++ b/controller/encaps.c
 @@ -23,6 +23,7 @@
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
 #include "ovn-controller.h"
 +#include "smap.h"
 
 VLOG_DEFINE_THIS_MODULE(encaps);
 
 @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const
> struct
>> sbrec_sb_global *sbg,
smap_add(, "dst_port", dst_port);
}
 

[ovs-dev] [PATCH ovn v3] controller: add ovn-set-local-ip option

[Vladislav Odintsov]

When transport node has multiple interfaces (vlans) and
ovn-encap-ip on different hosts need to be configured
from different VLANs source IP for encapsulated packet
can be not the same, which is expected by remote system.

Explicitely setting local_ip resolves such problem.

Signed-off-by: Vladislav Odintsov 
---
 controller/encaps.c | 43 +
 controller/ovn-controller.8.xml |  7 ++
 tests/ovn-controller.at |  9 +++
 3 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index 66e0cd8cd..8e6d290c1 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -23,6 +23,7 @@
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
 #include "ovn-controller.h"
+#include "smap.h"
 
 VLOG_DEFINE_THIS_MODULE(encaps);
 
@@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 smap_add(, "dst_port", dst_port);
 }
 
+const struct ovsrec_open_vswitch *cfg =
+ovsrec_open_vswitch_table_first(ovs_table);
+
+bool set_local_ip = false;
+if (cfg) {
+/* If the tos option is configured, get it */
+const char *encap_tos = smap_get_def(>external_ids,
+   "ovn-encap-tos", "none");
+
+if (encap_tos && strcmp(encap_tos, "none")) {
+smap_add(, "tos", encap_tos);
+}
+
+/* If ovn-set-local-ip option is configured, get it */
+set_local_ip = smap_get_bool(>external_ids, "ovn-set-local-ip",
+ false);
+}
+
 /* Add auth info if ipsec is enabled. */
 if (sbg->ipsec) {
+set_local_ip = true;
+smap_add(, "remote_name", new_chassis_id);
+}
+
+if (set_local_ip) {
 const struct sbrec_chassis *this_chassis = tc->this_chassis;
 const char *local_ip = NULL;
 
@@ -187,8 +211,10 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
  */
 for (int i = 0; i < this_chassis->n_encaps; i++) {
 if (local_ip && strcmp(local_ip, this_chassis->encaps[i]->ip)) {
-VLOG_ERR("ovn-encap-ip has been configured as a list. This "
- "is unsupported for IPsec.");
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_ERR_RL(, "ovn-encap-ip has been configured as a list. "
+"This is unsupported for IPsec and explicit "
+"local_ip configuration.");
 /* No need to loop further as we know this condition has been
  * hit */
 break;
@@ -200,19 +226,6 @@ tunnel_add(struct tunnel_ctx *tc, const struct 
sbrec_sb_global *sbg,
 if (local_ip) {
 smap_add(, "local_ip", local_ip);
 }
-smap_add(, "remote_name", new_chassis_id);
-}
-
-const struct ovsrec_open_vswitch *cfg =
-ovsrec_open_vswitch_table_first(ovs_table);
-/* If the tos option is configured, get it */
-if (cfg) {
-const char *encap_tos = smap_get_def(>external_ids,
-   "ovn-encap-tos", "none");
-
-if (encap_tos && strcmp(encap_tos, "none")) {
-smap_add(, "tos", encap_tos);
-}
 }
 
 /* If there's an existing chassis record that does not need any change,
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index e9708fe64..cc9a7d1c2 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -304,6 +304,13 @@
 of how many entries there are in the cache.  By default this is set to
 3 (30 seconds).
   
+  external_ids:ovn-set-local-ip
+  
+The boolean flag indicates if ovn-controller when create
+tunnel ports should set local_ip parameter.  Can be
+heplful to pin source outer IP for the tunnel when multiple interfaces
+are used on the host for overlay traffic.
+  
 
 
 
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index e99eec1d6..89ae2c9e1 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -298,6 +298,15 @@ OVS_WAIT_UNTIL([check_tunnel_property type geneve])
 ovs-vsctl del-port ovn-fakech-0
 OVS_WAIT_UNTIL([check_tunnel_property type geneve])
 
+# set `ovn-set-local-ip` option to true and check if tunnel parameters
+OVS_WAIT_WHILE([check_tunnel_property options:local_ip "\"192.168.0.1\""])
+ovs-vsctl set open . external_ids:ovn-set-local-ip=true
+OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""])
+
+# Change the local_ip on the OVS side and check than OVN fixes it
+ovs-vsctl set interface ovn-fakech-0 options:local_ip="1.1.1.1"
+OVS_WAIT_UNTIL([check_tunnel_property options:local_ip "\"192.168.0.1\""])
+
 # Gracefully terminate daemons
 OVN_CLEANUP_SBOX([hv])
 OVN_CLEANUP_VSWITCH([main])
-- 
2.26.3

___
dev mailing list

Re: [ovs-dev] [PATCH ovn v2] controller: add ovn-set-local-ip option

[Han Zhou]

On Thu, Feb 17, 2022 at 3:57 PM Numan Siddique  wrote:
>
> On Thu, Feb 17, 2022 at 5:50 PM Han Zhou  wrote:
> >
> > On Thu, Feb 17, 2022 at 2:02 PM Numan Siddique  wrote:
> > >
> > > On Thu, Feb 17, 2022 at 8:58 AM Vladislav Odintsov 
> > wrote:
> > > >
> > > > Hi Han,
> > > >
> > > > thanks for the note about log message, I’ll fix this in v3 right
after
> > the question with other_config is resolved.
> > > > Frankly speaking I also don’t understand why to sync
ovn-set-local-ip
> > option to other_config —
> > > > I did this only because Numan asked to do :)
> > > >
> > > > Regards,
> > > > Vladislav Odintsov
> > > >
> > > > > On 17 Feb 2022, at 09:39, Han Zhou  wrote:
> > > > >
> > > > > On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  > > wrote:
> > > > >>
> > > > >> On Wed, Feb 9, 2022 at 6:33 PM Vladislav Odintsov <
odiv...@gmail.com>
> > > > > wrote:
> > > > >>>
> > > > >>> When transport node has multiple interfaces (vlans) and
> > > > >>> ovn-encap-ip on different hosts need to be configured
> > > > >>> from different VLANs source IP for encapsulated packet
> > > > >>> can be not the same, which is expected by remote system.
> > > > >>>
> > > > >>> Explicitely setting local_ip resolves such problem.
> > > > >>>
> > > > >>> Signed-off-by: Vladislav Odintsov 
> > > > >>
> > > > >> Hi Vladislav,
> > > > >>
> > > > >> Can you please add the code to copy the new added config from OVS
> > > > >> database to the
> > > > >> other_config column of Chassis table in Southbound db ?
> > > > >>
> > > > >> Please take a look at "struct ovs_chassis_cfg" in
> > controller/chassis.c
> > > > >>
> > > > >> Thanks
> > > > >> Numan
> > > > >
> > > > > Hi Numan,
> > > > >
> > > > > May I ask what's the purpose of adding this to other_config in
SB? I
> > > > > understand that there are fields in other_config that is of
> > importance for
> > > > > other chassises, so need to be added to SB, but for this one, how
> > would it
> > > > > be used in SB DB?
> > >
> > > My understanding is that we just clone all the local ovs configs into
> > > chassis's other_config and my suggestion was to
> > > make sure we are consistent with the present behaviour.  Have we
> > > missed copying some of the ovs configs ?
> > >
> > > I'm actually fine either way.  If you think it's better not to copy to
> > > the sb db, then I am fine with it.
> > >
> >
> > Thanks Numan for the confirmation. I was wondering if I missed any
> > important use case of the OVS configs being stored in SB, now it seems
it
> > is fine not adding them. There are in fact many OVS configs not in SB,
such
> > as ovn-remote, ovn-remote-probe-interval, ovn-openflow-probe-interval,
> > ovn-encap-tos, ovn-match-northd-version, etc. All these are only locally
> > important, so I don't think it is necessary to sync them to SB. Probably
> > there were historical reasons why some of the configs were synced to SB
> > while they were useful only locally, and I can't recall all the details.
> > Maybe we can remove the unnecessary ones from SB in a separate patch to
> > reduce the noise of the SB chassis table, not urgent though.
>
> Sounds good to me.
>
> Does @Vladislav Odintsov  need to submit another version reverting the
> change I requested ?
>
I think Vladislav mentioned that he will submit v3 to fix the error log,
maybe he could revert the other-config change in v3, too.

> Numan
>
> >
> > Thanks,
> > Han
> >
> > > Numan
> > >
> > > > > Hi Vladislav,
> > > > >
> > > > > Regarding this:
> > > > >VLOG_ERR("ovn-encap-ip has been configured as a
list.
> > This "
> > > > > "is unsupported for IPsec.");
> > > > >
> > > > > Before your change it applies to IPsec only, but with your change
this
> > > > > would apply to non-IPsec as well, if ovn-set-local-ip is true.
Could
> > you
> > > > > update the error log as well? (it is better to be ratelimited,
but it
> > is
> > > > > not the fault of your patch)
> > > > >
> > > > > Thanks,
> > > > > Han
> > > > >
> > > > >>
> > > > >>> ---
> > > > >>> controller/encaps.c | 37
> > +
> > > > >>> controller/ovn-controller.8.xml |  7 +++
> > > > >>> tests/ovn-controller.at |  9 
> > > > >>> 3 files changed, 40 insertions(+), 13 deletions(-)
> > > > >>>
> > > > >>> diff --git a/controller/encaps.c b/controller/encaps.c
> > > > >>> index 66e0cd8cd..3b0c92931 100644
> > > > >>> --- a/controller/encaps.c
> > > > >>> +++ b/controller/encaps.c
> > > > >>> @@ -23,6 +23,7 @@
> > > > >>> #include "openvswitch/vlog.h"
> > > > >>> #include "lib/ovn-sb-idl.h"
> > > > >>> #include "ovn-controller.h"
> > > > >>> +#include "smap.h"
> > > > >>>
> > > > >>> VLOG_DEFINE_THIS_MODULE(encaps);
> > > > >>>
> > > > >>> @@ -176,8 +177,31 @@ tunnel_add(struct tunnel_ctx *tc, const
struct
> > > > > sbrec_sb_global *sbg,
> > > > >>> smap_add(, "dst_port", dst_port);
> > > > >>> }
> > > > >>>
> > > > >>> +const struct 

Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".

[Gaëtan Rivet]

On Thu, Feb 17, 2022, at 16:40, Adrian Moreno wrote:
> On 2/17/22 16:15, Gaëtan Rivet wrote:
>> On Thu, Feb 17, 2022, at 15:08, wangyunjian wrote:
 -Original Message-
 From: Gaëtan Rivet [mailto:gr...@u256.net]
 Sent: Thursday, February 17, 2022 9:31 PM
 To: wangyunjian ; 
 ; Ilya Maximets ; 贺鹏
 ; amore...@redhat.com
 Cc: dingxiaoxiong 
 Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".

 On Thu, Feb 17, 2022, at 08:29, wangyunjian wrote:
>> -Original Message-
>> From: dev [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of
>> wangyunjian via dev
>> Sent: Thursday, February 17, 2022 11:27 AM
>> To: Gaëtan Rivet ; 
>> ; Ilya Maximets 
>> Cc: dingxiaoxiong 
>> Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for "ofproto".
>>
>>
>>
>>> -Original Message-
>>> From: Gaëtan Rivet [mailto:gr...@u256.net]
>>> Sent: Thursday, February 17, 2022 1:29 AM
>>> To: wangyunjian ; 
>>> ; Ilya Maximets 
>>> Cc: amore...@redhat.com; dingxiaoxiong ;
>>> 贺
>> 鹏
>>> 
>>> Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for 
>>> "ofproto".
>>>
>>> On Wed, Feb 16, 2022, at 14:24, wangyunjian wrote:
> -Original Message-
> From: Gaëtan Rivet [mailto:gr...@u256.net]
> Sent: Wednesday, February 16, 2022 7:34 PM
> To: wangyunjian ; 
> ; Ilya Maximets 
> Cc: dingxiaoxiong 
> Subject: Re: [ovs-dev] [PATCH] ofproto: fix use-after-free for 
> "ofproto".
>
> On Fri, Dec 3, 2021, at 12:25, Yunjian Wang via dev wrote:
>> When handler threads lookup a "ofproto" and use it, main
>> thread maybe remove and free the "ofproto" at the same time. The
 "ofproto"
>> has not been protected well, which can lead to an OVS crash.
>>
>> This patch fixes this by making the "ofproto" lookup RCU-safe
>> by using cmap instead of hmap and moving remove "ofproto" call
>> before xlate_txn_commit().
>>
>
> I don't understand the point of moving the cmap_remove() call
> before xlate_txn_commit().

 To use of the rcu_synchronize in the xlate_txn_commit to avoid
 access to the ofproto from other thread through uuid map.

>>>
>>> Yes the reason is clear.
>>>
>>> But my question is why is it needed? It seems that the ofproto
>>> lifecycle was written with the assumption that it would still be
>>> used while being
>> destroyed.
>>>
>>> Can you explain why it needs to be changed?
>>
>> I didn't describe the problem clearly before. The main problem is
>> that hmap variable is not thread safe. The all_ofproto_dpifs_by_uuid
>> variable uses the hmap structure, which maybe be accessed by main thread
 and handler threads.

 I'm ok on the part switching to using CMAP to allow concurrent reads.
 That I see the reason and it is fine.

 The part that I don't understand is moving the cmap_remove() call before 
 the
 RCU sync.

 As far as I know, the CMAP type does not require that to safely operate.
 The writer thread is allowed to call cmap_remove() while a reader is 
 iterating on
 the CMAP to find a node. The only precaution needed is that actual 
 destruction
 of the node (freeing) is deferred, which it is.

 So I don't see the reason to move cmap_remove() before the RCU
 synchronization, instead of keeping it as it is now. Could you please 
 explain your
 reasoning?
>>>
>>> I consider it is more reasonable for the upcall thread cannot find the 
>>> ofproto
>>> when the ofproto will be deleted, no other special consideration.
>>>
>>> Thanks,
>>> Yunjian
>> 
>> That might be an interesting change to clean up the 'execution context' of 
>> the ofproto destruction.
>> 
>> But I think it is out of scope for this fix. It means changing the xlate 
>> object,
>> introduce coupling between the ofproto map and the xlate layer. This goes 
>> beyond
>> fixing undefined behavior due to concurrent accesses on the map.
>> 
>
> Hi Gaëtan,
>
> I think the root cause of the crash this fix pretends to fix is not the 
> concurrent access to the map (though that is also an issue, of course) but 
> the 
> upcall using an ofproto-dpif object after the main thread has run its 
> destruct(). So I think we need to fix both.
>
> With regards to the way of fixing it, what do you think about removing 
> all_ofproto_dpifs_by_uuid altogether and using xbridge_lookup_by_uuid() 
> intead?
>
> Thanks
> -- 
> Adrián Moreno

Hi Adrián,

Thanks for explaining, it's clear now.
Your suggestion is interesting, it seems correct.

However, it means changing xcfg->xbridges into a cmap. It requires
xlate_xbridge_remove() itself to be RCU compatible. Although there is the
RCU 

Re: [ovs-dev] [PATCH net 1/1] net/sched: act_ct: Fix flow table lookup after ct clear or switching zones

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller :

On Thu, 17 Feb 2022 11:30:48 +0200 you wrote:
> Flow table lookup is skipped if packet either went through ct clear
> action (which set the IP_CT_UNTRACKED flag on the packet), or while
> switching zones and there is already a connection associated with
> the packet. This will result in no SW offload of the connection,
> and the and connection not being removed from flow table with
> TCP teardown (fin/rst packet).
> 
> [...]

Here is the summary with links:
  - [net,1/1] net/sched: act_ct: Fix flow table lookup after ct clear or 
switching zones
https://git.kernel.org/netdev/net/c/2f131de361f6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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