Re: [ovs-dev] [PATCH v2] tc: Keep header rewrite actions order

[Roi Dayan via dev]

On 2022-02-16 12:15 PM, Eelco Chaudron wrote:



On 16 Feb 2022, at 10:48, Roi Dayan wrote:


On 2022-02-16 10:37 AM, Eelco Chaudron wrote:



On 15 Feb 2022, at 10:52, Roi Dayan wrote:


From: Chris Mi 

Currently, tc merges all header rewrite actions into one tc pedit
action. So the header rewrite actions order is lost. Save each header
rewrite action into one tc pedit action to keep the order. And only
append one tc csum action to the last pedit action of a series.


I’m on training all of this week, so will try to look at this later.
But in the meantime, do you think it’s worth adding a test case for this?

//Eelco


maybe. but the fix is already ready for some time while it looks like we
could add many potential cases related to tc.
We should and will find the time later to look at adding more unit tests
into the repo.
thanks


The reason for asking for a test case is that when reviewing TC-related patches 
I no longer just do a visual review, I test them. This is because a lot of 
times a small change results in the introduction of problems, for example with 
the dpctl/dump-flows output. So without any test cases, I need to reverse 
engineer how to replicate the problem, and then test it.

So my request would be if you are not supplying a test case, at least supply a 
simple replicator in the commit description.



I understand. I thought in this case it's clear.

before this commit all header rewrite actions are saved into
flower->rewrite and from there translated to single tc pedit action.

This patch saves header rewrite in action->rewrite per action instance
so translating actions to tc ends up with multiple/separated tc pedit
actions.

as we already agreed we will try to improve the testsuite
in the ovs repo in near future.

We can reproduce the issue like this:
Config ovs with hw-offload=true.
Create bridge with 2 ports.

execute:

ovs-appctl dpctl/add-flow 
"ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(),ipv4()" 
"set(eth(dst=76:59:99:fb:aa:aa)),3,set(eth(dst=76:59:99:fb:ff:ff))" ; 
tc filter show dev enp8s0f0 ingress;



before this commit you will see in TC pedit,mirred and after the commit
you will see pedit,mirred,pedit.

do you want me to add the steps to the commit msg?


Signed-off-by: Chris Mi 
Reviewed-by: Roi Dayan 
---

v2:
- rebased to fix conflict.

   lib/netdev-offload-tc.c | 22 
   lib/tc.c| 54 
+
   lib/tc.h| 25 +++
   3 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 9845e8d3feae..ab2a678c6923 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -481,10 +481,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)

   static void
   parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
-   struct tc_flower *flower)
+   struct tc_action *action)
   {
-char *mask = (char *) >rewrite.mask;
-char *data = (char *) >rewrite.key;
+char *mask = (char *) >rewrite.mask;
+char *data = (char *) >rewrite.key;

   for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
   char *put = NULL;
@@ -877,7 +877,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
   }
   break;
   case TC_ACT_PEDIT: {
-parse_flower_rewrite_to_netlink_action(buf, flower);
+parse_flower_rewrite_to_netlink_action(buf, action);
   }
   break;
   case TC_ACT_ENCAP: {
@@ -1222,8 +1222,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
   uint64_t set_stub[1024 / 8];
   struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
   char *set_data, *set_mask;
-char *key = (char *) >rewrite.key;
-char *mask = (char *) >rewrite.mask;
+char *key = (char *) >rewrite.key;
+char *mask = (char *) >rewrite.mask;
   const struct nlattr *attr;
   int i, j, type;
   size_t size;
@@ -1265,14 +1265,6 @@ parse_put_flow_set_masked_action(struct tc_flower 
*flower,
   }
   }

-if (!is_all_zeros(>rewrite, sizeof flower->rewrite)) {
-if (flower->rewrite.rewrite == false) {
-flower->rewrite.rewrite = true;
-action->type = TC_ACT_PEDIT;
-flower->action_count++;
-}
-}
-
   if (hasmask && !is_all_zeros(set_mask, size)) {
   VLOG_DBG_RL(, "unsupported sub attribute of set action type %d",
   type);
@@ -1281,6 +1273,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
   }

   ofpbuf_uninit(_buf);
+action->type = TC_ACT_PEDIT;
+flower->action_count++;
   return 0;
   }

diff --git a/lib/tc.c b/lib/tc.c
index adb2d3182ad4..b637f4c17ed9 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1006,14 

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

[wangyunjian via dev]

> -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 change the description to "ofproto: fix concurrent when looking up an ofproto 
by UUID.".
And it maybe be clearly understood. How about this?

Thanks,
Yunjian

> 
> Thanks,
> Yunjian
> 
> >
> > --
> > Gaetan Rivet
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v9 4/6] northd, utils: support for RouteTables in LRs

[Han Zhou]

On Mon, Feb 14, 2022 at 10:32 AM Vladislav Odintsov 
wrote:
>
> Hi Dumitru,
>
> I’ve got a general question about what the logical topology you’d like to
implement.
>
> Also, please see inline comments.
>
>
> Regards,
> Vladislav Odintsov
>
> > On 14 Feb 2022, at 15:51, Dumitru Ceara  wrote:
> >
> > Hi Vladislav,
> >
> > I was looking at what options we have in OVN to implement a VRF-like
> > behavior and then remembered you had worked on the multiple routing
> > table feature.
> >

Hi Dumitru, during the code review it has been clarified that this
RouteTable feature is NOT for VRF-like use cases. VRF-like use cases can be
directly implemented with multiple LRs in OVN.

Thanks,
Han

> > On 11/19/21 17:07, Vladislav Odintsov wrote:
> >> This patch extends Logical Router's routing functionality.
> >> Now user may create multiple routing tables within a Logical Router
> >> and assign them to Logical Router Ports.
> >>
> >> Traffic coming from Logical Router Port with assigned route_table
> >> is checked against Logical_Router_Static_Routes with same route_table
> >> field value and routes to connected networks. If no route_table option
> >> is set to the LRP, routes' lookup is done agains routes with no
> >> route_table field value ("", empty string) and against routes to
> >> connected networks.
> >
> > I was a bit surprised by the fact that if there's no static route with
> > the same route_table value as the ingress router port we go ahead and
> > match against all connected networks, regardless of their ports' route
> > tables.
>
> The current behaviour is expected — earlier while code review I’ve
described it’s similar
> to AWS/GCP VPC lookup logic: the "local" route (route to connected
networks/subnets) is present
> always and can’t be deleted or modified.
> This means that "connected" routes (including learned routes from other
AZs, which
> have "connected" origin) are installed in the routing table regardless of
their `route_table` value.
> But the user may create more specific static route if he/she needs it and
this route doesn’t break
> necessary connectivity.
>
> > Does it make sense to change this and perform longest prefix match in
> > general, always matching on route_table?
>
> I can’t imagine how to easy support current behaviour with such change. I
mean how
> to force a match against all connected routes regardless of routing table.
> The only one option I see is to add an option for LR, which configures a
mode of
> local routes match: with or without route_table value respect.
> >
> > In this case connected routes could inherit the route_table value from
> > the corresponding router ports where they're defined.
> >
> > Would that break your use case?
>
> Please, see previous comment.
>
> >
> > Thanks,
> > Dumitru
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Han Zhou]

On Fri, Feb 11, 2022 at 11:22 AM Numan Siddique  wrote:
>
> On Wed, Feb 9, 2022 at 6:33 PM 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 
>
> 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?

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 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;
> >
> > @@ -200,19 +224,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 2f39e5f3e..9e6302e5a 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])

Re: [ovs-dev] [PATCH ovn v2 1/5] Introduce LSP:options:requested-additional-chassis

[0-day Robot]

Bleep bloop.  Greetings Ihar Hrachyshka, 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.


git-am:
error: sha1 information is lacking or useless (northd/northd.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 Introduce LSP:options:requested-additional-chassis
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


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 v2 4/5] Implement RARP activation strategy for ports

[Ihar Hrachyshka]

When options:activation-strategy is set to "rarp" for LSP, when used in
combination with options:requested-additional-chassis, the additional
chassis will install special flows that would block all ingress and
egress traffic for the port until a special activation event happens.

For "rarp" strategy, an observation of a RARP packet sent from the port
on the additional chassis is such an event. When it occurs, a special
flow passes control to a controller() action handler that removes the
installed blocking flows and also marks the port as
options:additional-chassis-activated in southbound db. Once vswitchd
processes the flow mod request, the port is ready to communicate from
the new location.

This feature is useful in live migration scenarios where it's not
advisable to unlock the destination port location prematurily to avoid
duplicate packets originating from the port.

Signed-off-by: Ihar Hrachyshka 
---
v2: support ddlog
---
 controller/physical.c |  74 +++
 controller/pinctrl.c  | 161 +-
 controller/pinctrl.h  |   2 +
 include/ovn/actions.h |   9 +++
 lib/actions.c |  40 ++-
 northd/northd.c   |   7 ++
 northd/ovn-northd.c   |   5 +-
 northd/ovn_northd.dl  |  27 ++-
 ovn-nb.xml|  10 +++
 ovn-sb.xml|  15 
 tests/ovn.at  | 161 ++
 utilities/ovn-trace.c |   3 +
 12 files changed, 507 insertions(+), 7 deletions(-)

diff --git a/controller/physical.c b/controller/physical.c
index 47ad3da33..f80665573 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -40,7 +40,9 @@
 #include "lib/mcast-group-index.h"
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
+#include "ovn/actions.h"
 #include "physical.h"
+#include "pinctrl.h"
 #include "openvswitch/shash.h"
 #include "simap.h"
 #include "smap.h"
@@ -978,6 +980,59 @@ get_additional_tunnel(const struct sbrec_port_binding 
*binding,
 return tun;
 }
 
+static void
+setup_rarp_activation_strategy(const struct sbrec_port_binding *binding,
+   struct ovn_desired_flow_table *flow_table,
+   struct ofpbuf *ofpacts_p)
+{
+struct match match = MATCH_CATCHALL_INITIALIZER;
+uint32_t dp_key = binding->datapath->tunnel_key;
+uint32_t port_key = binding->tunnel_key;
+
+/* Unblock the port on ingress RARP. */
+match_set_metadata(, htonll(dp_key));
+match_set_dl_type(, htons(ETH_TYPE_RARP));
+match_set_reg(, MFF_LOG_INPORT - MFF_REG0, port_key);
+ofpbuf_clear(ofpacts_p);
+
+size_t ofs = ofpacts_p->size;
+struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts_p);
+oc->max_len = UINT16_MAX;
+oc->reason = OFPR_ACTION;
+oc->pause = true;
+
+struct action_header ah = {
+.opcode = htonl(ACTION_OPCODE_ACTIVATION_STRATEGY_RARP)
+};
+ofpbuf_put(ofpacts_p, , sizeof ah);
+
+ofpacts_p->header = oc;
+oc->userdata_len = ofpacts_p->size - (ofs + sizeof *oc);
+ofpact_finish_CONTROLLER(ofpacts_p, );
+
+ofctrl_add_flow(flow_table, OFTABLE_LOG_INGRESS_PIPELINE, 1010,
+binding->header_.uuid.parts[0],
+, ofpacts_p, >header_.uuid);
+ofpbuf_clear(ofpacts_p);
+
+/* Block all non-RARP traffic for the port, both directions. */
+match_init_catchall();
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_INPORT - MFF_REG0, port_key);
+
+ofctrl_add_flow(flow_table, OFTABLE_LOG_INGRESS_PIPELINE, 1000,
+binding->header_.uuid.parts[0],
+, ofpacts_p, >header_.uuid);
+
+match_init_catchall();
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, port_key);
+
+ofctrl_add_flow(flow_table, OFTABLE_LOG_EGRESS_PIPELINE, 1000,
+binding->header_.uuid.parts[0],
+, ofpacts_p, >header_.uuid);
+}
+
 static void
 consider_port_binding(struct ovsdb_idl_index *sbrec_port_binding_by_name,
   enum mf_field_id mff_ovn_geneve,
@@ -1194,6 +1249,25 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 }
 }
 
+if (binding->additional_chassis == chassis) {
+const char *strategy = smap_get(>options,
+"activation-strategy");
+if (strategy
+&& !smap_get_bool(>options,
+  "additional-chassis-activated", false)
+&& !pinctrl_is_port_activated(dp_key, port_key)) {
+if (!strcmp(strategy, "rarp")) {
+setup_rarp_activation_strategy(binding, flow_table, ofpacts_p);
+} else {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(,
+ "Unknown activation strategy defined for "
+ "port %s: %s", 

[ovs-dev] [PATCH ovn v2 1/5] Introduce LSP:options:requested-additional-chassis

[Ihar Hrachyshka]

If used in conjunction with requested-chassis, OVN will attempt to
bind the port at another location in addition to the main chassis.

This is useful in live migration scenarios where it's important to
prepare the environment for workloads to move to, avoiding costly flow
configuration at the moment of the final port binding location change.

The patch mimics behavior of requested-chassis. Corresponding database
fields (pb->additional_chassis, pb->requested_additional_chassis,
pb->additional_encap) are introduced as part of the patch.

Signed-off-by: Ihar Hrachyshka 
---
v2: support ddlog
v2: fixed sb db version / checksum
---
 controller/binding.c | 178 +--
 controller/lport.c   |  19 -
 northd/northd.c  |  64 +---
 northd/ovn-northd.c  |   4 +-
 northd/ovn-sb.dlopts |   2 +
 northd/ovn_northd.dl | 148 +++
 ovn-nb.xml   |   8 ++
 ovn-sb.ovsschema |  17 -
 ovn-sb.xml   |  58 +-
 tests/ovn.at |  91 ++
 10 files changed, 517 insertions(+), 72 deletions(-)

diff --git a/controller/binding.c b/controller/binding.c
index c7a13d5d5..ec8bff3d8 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -912,6 +912,26 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
 }
 }
 
+typedef void (*set_func)(const struct sbrec_port_binding *pb,
+ const struct sbrec_encap *);
+
+static bool
+update_port_encap_if_needed(const struct sbrec_port_binding *pb,
+const struct sbrec_chassis *chassis_rec,
+const struct ovsrec_interface *iface_rec,
+bool sb_readonly, set_func f)
+{
+const struct sbrec_encap *encap_rec =
+sbrec_get_port_encap(chassis_rec, iface_rec);
+if (encap_rec && pb->encap != encap_rec) {
+if (sb_readonly) {
+return false;
+}
+f(pb, encap_rec);
+}
+return true;
+}
+
 /* Returns false if lport is not claimed due to 'sb_readonly'.
  * Returns true otherwise.
  */
@@ -928,37 +948,68 @@ claim_lport(const struct sbrec_port_binding *pb,
 claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up, if_mgr);
 }
 
-if (pb->chassis != chassis_rec) {
-if (sb_readonly) {
-return false;
-}
+if (!pb->requested_chassis || pb->requested_chassis == chassis_rec) {
+if (pb->chassis != chassis_rec) {
+if (sb_readonly) {
+return false;
+}
 
-if (pb->chassis) {
-VLOG_INFO("Changing chassis for lport %s from %s to %s.",
-pb->logical_port, pb->chassis->name,
-chassis_rec->name);
-} else {
-VLOG_INFO("Claiming lport %s for this chassis.", pb->logical_port);
-}
-for (int i = 0; i < pb->n_mac; i++) {
-VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
+if (pb->chassis) {
+VLOG_INFO("Changing chassis for lport %s from %s to %s.",
+pb->logical_port, pb->chassis->name,
+chassis_rec->name);
+} else {
+VLOG_INFO("Claiming lport %s for this chassis.",
+  pb->logical_port);
+}
+for (int i = 0; i < pb->n_mac; i++) {
+VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
+}
+
+sbrec_port_binding_set_chassis(pb, chassis_rec);
+if (pb->additional_chassis == chassis_rec) {
+sbrec_port_binding_set_additional_chassis(pb, NULL);
+if (pb->additional_encap) {
+sbrec_port_binding_set_additional_encap(pb, NULL);
+}
+}
 }
+} else if (pb->requested_additional_chassis == chassis_rec) {
+if (pb->additional_chassis != chassis_rec) {
+if (sb_readonly) {
+return false;
+}
 
-sbrec_port_binding_set_chassis(pb, chassis_rec);
+if (pb->additional_chassis) {
+VLOG_INFO(
+"Changing additional chassis for lport %s from %s to %s.",
+pb->logical_port, pb->chassis->name, chassis_rec->name);
+} else {
+VLOG_INFO(
+"Claiming lport %s for this additional chassis.",
+pb->logical_port);
+}
+for (int i = 0; i < pb->n_mac; i++) {
+VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
+}
 
-if (tracked_datapaths) {
-update_lport_tracking(pb, tracked_datapaths, true);
+sbrec_port_binding_set_additional_chassis(pb, chassis_rec);
 }
 }
 
+if (tracked_datapaths) {
+update_lport_tracking(pb, tracked_datapaths, true);
+}
+
 /* Check if 

[ovs-dev] [PATCH ovn 5/5] Reinject RARP packet when activation-strategy=rarp

[Ihar Hrachyshka]

It takes some time for vswitchd to remove the blocking flows, so we need
to wait for the flow_mod message handled before reinjecting the received
RARP packet into the pipeline.

Use a barrier to indicate the message processed by vswitchd.

Signed-off-by: Ihar Hrachyshka 
---
 controller/pinctrl.c | 103 ---
 tests/ovn.at |   8 +++-
 2 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index af9279bb4..d95a5d8bf 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -201,7 +201,14 @@ static void send_mac_binding_buffered_pkts(struct rconn 
*swconn)
 OVS_REQUIRES(pinctrl_mutex);
 
 static void pinctrl_rarp_activation_strategy_handler(struct rconn *swconn,
- const struct match *md);
+ const struct match *md,
+ struct dp_packet *pkt_in);
+
+static void init_pending_rarp_packets(void);
+static void destroy_pending_rarp_packets(void);
+static void flush_pending_rarp_packets(struct rconn *swconn, uint32_t xid)
+OVS_REQUIRES(pinctrl_mutex);
+
 static void init_activated_ports(void);
 static void destroy_activated_ports(void);
 static void wait_activated_ports(struct ovsdb_idl_txn *ovnsb_idl_txn);
@@ -536,6 +543,7 @@ pinctrl_init(void)
 init_ipv6_prefixd();
 init_buffered_packets_map();
 init_activated_ports();
+init_pending_rarp_packets();
 init_event_table();
 ip_mcast_snoop_init();
 init_put_vport_bindings();
@@ -3258,7 +3266,8 @@ process_packet_in(struct rconn *swconn, const struct 
ofp_header *msg)
 
 case ACTION_OPCODE_ACTIVATION_STRATEGY_RARP:
 ovs_mutex_lock(_mutex);
-pinctrl_rarp_activation_strategy_handler(swconn, _metadata);
+pinctrl_rarp_activation_strategy_handler(swconn, _metadata,
+ );
 ovs_mutex_unlock(_mutex);
 break;
 
@@ -3319,6 +3328,10 @@ pinctrl_recv(struct rconn *swconn, const struct 
ofp_header *oh,
 } else if (type == OFPTYPE_PACKET_IN) {
 COVERAGE_INC(pinctrl_total_pin_pkts);
 process_packet_in(swconn, oh);
+} else if (type == OFPTYPE_BARRIER_REPLY) {
+ovs_mutex_lock(_mutex);
+flush_pending_rarp_packets(swconn, ntohl(oh->xid));
+ovs_mutex_unlock(_mutex);
 } else {
 if (VLOG_IS_DBG_ENABLED()) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30, 300);
@@ -4072,6 +4085,7 @@ pinctrl_destroy(void)
 destroy_ipv6_prefixd();
 destroy_buffered_packets_map();
 destroy_activated_ports();
+destroy_pending_rarp_packets();
 event_table_destroy();
 destroy_put_mac_bindings();
 destroy_put_vport_bindings();
@@ -7760,6 +7774,70 @@ encode_flow_mod(struct ofputil_flow_mod *fm)
 return ofputil_encode_flow_mod(fm, OFPUTIL_P_OF15_OXM);
 }
 
+struct rarp_packet {
+uint32_t xid;
+int64_t dp_key;
+int64_t port_key;
+struct dp_packet *pkt;
+struct ovs_list list;
+};
+
+static struct ovs_list pending_rarp_packets;
+
+static void
+init_pending_rarp_packets(void)
+{
+ovs_list_init(_rarp_packets);
+}
+
+static void
+destroy_pending_rarp_packets(void)
+{
+struct rarp_packet *rp;
+LIST_FOR_EACH_POP (rp, list, _rarp_packets) {
+free(rp->pkt);
+free(rp);
+}
+}
+
+static void flush_pending_rarp_packets(struct rconn *swconn, uint32_t xid)
+OVS_REQUIRES(pinctrl_mutex)
+{
+struct rarp_packet *rp, *next;
+LIST_FOR_EACH_SAFE (rp, next, list, _rarp_packets) {
+if (rp->xid != xid) {
+continue;
+}
+/* Blocking flows are now gone; re-inject RARP message. */
+uint64_t ofpacts_stub[4096 / 8];
+struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
+enum ofp_version version = rconn_get_version(swconn);
+put_load(rp->dp_key, MFF_LOG_DATAPATH, 0, 64, );
+put_load(rp->port_key, MFF_LOG_INPORT, 0, 32, );
+struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT();
+resubmit->in_port = OFPP_CONTROLLER;
+resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
+
+struct ofputil_packet_out po = {
+.packet = dp_packet_data(rp->pkt),
+.packet_len = dp_packet_size(rp->pkt),
+.buffer_id = UINT32_MAX,
+.ofpacts = ofpacts.data,
+.ofpacts_len = ofpacts.size,
+};
+match_set_in_port(_metadata, OFPP_CONTROLLER);
+
+enum ofputil_protocol proto;
+proto = ofputil_protocol_from_ofp_version(version);
+queue_msg(swconn, ofputil_encode_packet_out(, proto));
+ofpbuf_uninit();
+
+ovs_list_remove(>list);
+free(rp->pkt);
+free(rp);
+}
+}
+
 struct port_pair {
 uint32_t dp_key;
 uint32_t port_key;
@@ -7828,7 +7906,8 @@ bool 

[ovs-dev] [PATCH ovn v2 3/5] Enforce tunneling when additional-chassis is set

[Ihar Hrachyshka]

When additional-chassis is set, we cannot guarantee the upstream
switch to deliver a unicast packet sent through a localnet port to
both port chassis locations (pb->chassis and pb->additional_chassis).
To deliver packets to both locations, switch to tunneling.

Signed-off-by: Ihar Hrachyshka 
---
 controller/physical.c |  11 +-
 tests/ovn.at  | 360 ++
 2 files changed, 365 insertions(+), 6 deletions(-)

diff --git a/controller/physical.c b/controller/physical.c
index ec4e8543a..47ad3da33 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1166,7 +1166,9 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 if (!ofport) {
 /* It is remote port, may be reached by tunnel or localnet port */
 is_remote = true;
-if (localnet_port) {
+/* Enforce tunneling while we clone packets to additional chassis b/c
+ * otherwise upstream switch won't flood the packet to both chassis. */
+if (localnet_port && !binding->additional_chassis) {
 ofport = u16_to_ofp(simap_get(patch_ofports,
   localnet_port->logical_port));
 if (!ofport) {
@@ -1193,11 +1195,8 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 }
 
 /* Clone packets to additional chassis if needed. */
-const struct chassis_tunnel *additional_tun = NULL;
-if (!localnet_port) {
-additional_tun = get_additional_tunnel(binding, chassis,
-   chassis_tunnels);
-}
+const struct chassis_tunnel *additional_tun;
+additional_tun = get_additional_tunnel(binding, chassis, chassis_tunnels);
 
 if (!is_remote) {
 /* Packets that arrive from a vif can belong to a VM or
diff --git a/tests/ovn.at b/tests/ovn.at
index 2c8c706df..1ccec492d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -14023,6 +14023,366 @@ OVN_CLEANUP([hv1],[hv2],[hv3])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([localnet connectivity with options:requested-additional-chassis])
+ovn_start
+
+net_add n1
+for i in 1 2 3; do
+sim_add hv$i
+as hv$i
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.$i
+check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+done
+
+# Disable local ARP responder to pass ARP requests through tunnels
+check ovn-nbctl ls-add ls0 -- add Logical_Switch ls0 other_config 
vlan-passthru=true
+
+check ovn-nbctl lsp-add ls0 first
+check ovn-nbctl lsp-add ls0 second
+check ovn-nbctl lsp-add ls0 third
+check ovn-nbctl lsp-add ls0 migrator
+check ovn-nbctl lsp-set-addresses first "00:00:00:00:00:01 10.0.0.1"
+check ovn-nbctl lsp-set-addresses second "00:00:00:00:00:02 10.0.0.2"
+check ovn-nbctl lsp-set-addresses third "00:00:00:00:00:03 10.0.0.3"
+check ovn-nbctl lsp-set-addresses migrator "00:00:00:00:00:ff 10.0.0.100"
+
+check ovn-nbctl lsp-add ls0 public
+check ovn-nbctl lsp-set-type public localnet
+check ovn-nbctl lsp-set-addresses public unknown
+check ovn-nbctl lsp-set-options public network_name=phys
+
+# The test scenario will migrate Migrator port between hv1 and hv2 and check
+# that connectivity to and from the port is functioning properly for both
+# chassis locations. Connectivity will be checked for resources located at hv1
+# (First) and hv2 (Second) as well as for hv3 (Third) that does not take part
+# in port migration.
+check ovn-nbctl lsp-set-options first requested-chassis=hv1
+check ovn-nbctl lsp-set-options second requested-chassis=hv2
+check ovn-nbctl lsp-set-options third requested-chassis=hv3
+
+as hv1 check ovs-vsctl -- add-port br-int first -- \
+set Interface first external-ids:iface-id=first \
+options:tx_pcap=hv1/first-tx.pcap \
+options:rxq_pcap=hv1/first-rx.pcap
+as hv2 check ovs-vsctl -- add-port br-int second -- \
+set Interface second external-ids:iface-id=second \
+options:tx_pcap=hv2/second-tx.pcap \
+options:rxq_pcap=hv2/second-rx.pcap
+as hv3 check ovs-vsctl -- add-port br-int third -- \
+set Interface third external-ids:iface-id=third \
+options:tx_pcap=hv3/third-tx.pcap \
+options:rxq_pcap=hv3/third-rx.pcap
+
+# Create Migrator interfaces on both hv1 and hv2
+for hv in hv1 hv2; do
+as $hv check ovs-vsctl -- add-port br-int migrator -- \
+set Interface migrator external-ids:iface-id=migrator \
+options:tx_pcap=$hv/migrator-tx.pcap \
+options:rxq_pcap=$hv/migrator-rx.pcap
+done
+
+send_arp() {
+local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
+local 
request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+as ${hv} ovs-appctl netdev-dummy/receive $inport $request
+echo "${request}"
+}
+
+send_garp() {
+local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
+local 
request=${eth_dst}${eth_src}08060001080006040002${eth_src}${spa}${eth_dst}${tpa}
+as ${hv} 

[ovs-dev] [PATCH ovn v2 2/5] Clone packets to both port chassis

[Ihar Hrachyshka]

When requested-additional-chassis is set, port binding is configured
in two cluster locations. In case of live migration scenario, only one
of the locations run a workload at a particular point in time. Yet,
it's expected that the workload may switch to running at the
additional-chassis at any moment during live migration (depends on
libvirt / qemu migration progress). To speed up the switch to near
instant, do the following:

When a port located sends a packet to another port that has two
chassis then, in addition to sending the packet to the main chassis,
also send it to the additional chassis. When the sending port is bound
on either the main or additional chassis, then handle the packet
locally plus send it to the other chassis.

This is achieved with additional flows in tables 37 and 38.

Signed-off-by: Ihar Hrachyshka 
---
 controller/physical.c | 180 +++
 tests/ovn.at  | 335 ++
 2 files changed, 486 insertions(+), 29 deletions(-)

diff --git a/controller/physical.c b/controller/physical.c
index a3eddd100..ec4e8543a 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -287,12 +287,13 @@ match_outport_dp_and_port_keys(struct match *match,
 }
 
 static void
-put_remote_port_redirect_overlay(const struct
- sbrec_port_binding *binding,
+put_remote_port_redirect_overlay(const struct sbrec_port_binding *binding,
  bool is_ha_remote,
  struct ha_chassis_ordered *ha_ch_ordered,
  enum mf_field_id mff_ovn_geneve,
  const struct chassis_tunnel *tun,
+ const struct chassis_tunnel *additional_tun,
+ uint32_t dp_key,
  uint32_t port_key,
  struct match *match,
  struct ofpbuf *ofpacts_p,
@@ -301,14 +302,51 @@ put_remote_port_redirect_overlay(const struct
 {
 if (!is_ha_remote) {
 /* Setup encapsulation */
-if (!tun) {
-return;
+bool is_vtep = !strcmp(binding->type, "vtep");
+if (!additional_tun) {
+/* Output to main chassis tunnel. */
+put_encapsulation(mff_ovn_geneve, tun, binding->datapath, port_key,
+  is_vtep, ofpacts_p);
+ofpact_put_OUTPUT(ofpacts_p)->port = tun->ofport;
+
+ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
+binding->header_.uuid.parts[0],
+match, ofpacts_p, >header_.uuid);
+} else {
+/* For packets arriving from tunnels, don't clone to avoid sending
+ * packets received from another chassis back to it. */
+match_outport_dp_and_port_keys(match, dp_key, port_key);
+match_set_reg_masked(match, MFF_LOG_FLAGS - MFF_REG0,
+ MLF_LOCAL_ONLY, MLF_LOCAL_ONLY);
+
+/* Output to main chassis tunnel. */
+put_encapsulation(mff_ovn_geneve, tun, binding->datapath, port_key,
+  is_vtep, ofpacts_p);
+ofpact_put_OUTPUT(ofpacts_p)->port = tun->ofport;
+
+ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 110,
+binding->header_.uuid.parts[0], match, ofpacts_p,
+>header_.uuid);
+
+/* For packets originating from this chassis, clone in addition to
+ * handling it locally. */
+match_outport_dp_and_port_keys(match, dp_key, port_key);
+ofpbuf_clear(ofpacts_p);
+
+/* Output to main chassis tunnel. */
+put_encapsulation(mff_ovn_geneve, tun, binding->datapath, port_key,
+  is_vtep, ofpacts_p);
+ofpact_put_OUTPUT(ofpacts_p)->port = tun->ofport;
+
+/* Output to additional chassis tunnel. */
+put_encapsulation(mff_ovn_geneve, additional_tun,
+  binding->datapath, port_key, is_vtep, ofpacts_p);
+ofpact_put_OUTPUT(ofpacts_p)->port = additional_tun->ofport;
+
+ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
+binding->header_.uuid.parts[0], match, ofpacts_p,
+>header_.uuid);
 }
-put_encapsulation(mff_ovn_geneve, tun, binding->datapath, port_key,
-  !strcmp(binding->type, "vtep"),
-  ofpacts_p);
-/* Output to tunnel. */
-ofpact_put_OUTPUT(ofpacts_p)->port = tun->ofport;
 } else {
 /* Make sure all tunnel endpoints use the same encapsulation,
  * and set it up */
@@ -376,10 +414,11 @@ put_remote_port_redirect_overlay(const struct
 bundle->basis = 0;
 bundle->fields = 

[ovs-dev] [PATCH ovn v2 0/5] Support additional-chassis for ports

[Ihar Hrachyshka]

This is an update to the initial series. Most patches from the original
series still stand. This new v2 series update the last four patches in
the original series and adds an additional patch to re-inject RARP
packets observed back into pipeline to speed up fabric update about the
new location of the port.

Ihar Hrachyshka (5):
  Introduce LSP:options:requested-additional-chassis
  Clone packets to both port chassis
  Enforce tunneling when additional-chassis is set
  Implement RARP activation strategy for ports
  Reinject RARP packet when activation-strategy=rarp

 controller/binding.c  | 178 ++--
 controller/lport.c|  19 +-
 controller/physical.c | 255 +--
 controller/pinctrl.c  | 254 ++-
 controller/pinctrl.h  |   2 +
 include/ovn/actions.h |   9 +
 lib/actions.c |  40 +-
 northd/northd.c   |  71 +++-
 northd/ovn-northd.c   |   7 +-
 northd/ovn-sb.dlopts  |   2 +
 northd/ovn_northd.dl  | 169 +++-
 ovn-nb.xml|  18 +
 ovn-sb.ovsschema  |  17 +-
 ovn-sb.xml|  73 +++-
 tests/ovn.at  | 953 ++
 utilities/ovn-trace.c |   3 +
 16 files changed, 1965 insertions(+), 105 deletions(-)

-- 
2.34.1

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

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

[wangyunjian via dev]

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

Thanks,
Yunjian

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

Re: [ovs-dev] [PATCH 2/5] ipf: add ipf context

[Peng He]

Hi,

just found I forget to answer other questions.

Ilya Maximets  于2022年2月15日周二 02:11写道：

> On 1/28/22 17:14, Aaron Conole wrote:
> > From: Peng He 
> >
> > ipf_postprocess will emit packets into the datapath pipeline ignoring
> > the conntrack context, this might casuse weird issues when a packet
> > batch has less space to hold all the fragments belonging to single
> > packet.
> >
> > Given the below ruleest and consider sending a 64K ICMP packet which
> > is splitted into 64 fragments.
> >
> > priority=1,action=drop
> > priority=10,arp,action=normal
> > priority=100,in_port=1,ct_state=-trk,icmp,action=ct(zone=9,table=0)
> > priority=100,in_port=1,ct_state=+new+trk,icmp,action=ct(zone=9,commit),2
> > priority=100,in_port=1,ct_state=-new+est+trk,icmp,action=2
> > priority=100,in_port=2,ct_state=-trk,icmp,action=ct(table=0,zone=9)
> > priority=100,in_port=2,ct_state=+trk+est-new,icmp,action=1
> >
> > Batch 1:
> > the first 32 packets will be buffered in the ipf preprocessing, nothing
> > more proceeds.
> >
> > Batch 2:
> > the second 32 packets succeed the fragment reassembly and goes to ct
> > and ipf_post will emits the first 32 packets due to the limit of batch
> > size.
> >
> > the first 32 packets goes to the datapath again due to the
> > recirculation, and again buffered at ipf preprocessing before ct commit,
> > then the ovs tries to call ct commit as well as ipf_postprocessing which
> emits
> > the last 32 packets, in this case the last 32 packets will follow
> > the current actions which will be sent to port 2 directly without
> > recirculation and going to ipf preprocssing and ct commit again.
> >
> > This will cause the first 32 packets never get the chance to
> > reassemble and evevntually this large ICMP packets fail to transmit.
> >
> > this patch fixes this issue by adding firstly ipf context to avoid
> > ipf_postprocessing emits packets in the wrong context. Then by
> > re-executing the action list again to emit the last 32 packets
> > in the right context to correctly transmitting multiple fragments.
> >
> > This might be one of the root cause why the OVS log warns:
> >
> > "
> > skipping output to input port on bridge br-int while processing
> > "
> >
> > As a pkt might enter into different ipf context.
> >
> > One implementation trick of implementing ipf_ctx_eq, is to use ipf_list's
> > key instead of the first frag's metadata, this can reduce quite a lot of
> > cache misses as at the time of calling ipf_ctx_eq, ipf_list is cache-hot.
> > On x86_64, this trick saves 2% of CPU usage of ipf processing.
> >
> > Signed-off-by: Peng He 
> > Co-authored-by: Mike Pattrick 
> > Signed-off-by: Aaron Conole 
> > ---
> >  lib/conntrack.c |  9 ---
> >  lib/conntrack.h | 15 ++--
> >  lib/dpif-netdev.c   | 52 +
> >  lib/ipf.c   | 39 ---
> >  lib/ipf.h   | 11 +++--
> >  tests/system-traffic.at | 33 ++
> >  tests/test-conntrack.c  |  6 ++---
> >  7 files changed, 135 insertions(+), 30 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 33a1a92953..72999f1aed 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -1434,14 +1434,14 @@ process_one(struct conntrack *ct, struct
> dp_packet *pkt,
> >   * connection tables.  'setmark', if not NULL, should point to a two
> >   * elements array containing a value and a mask to set the connection
> mark.
> >   * 'setlabel' behaves similarly for the connection label.*/
> > -int
> > +bool
> >  conntrack_execute(struct conntrack *ct, struct dp_packet_batch
> *pkt_batch,
> >ovs_be16 dl_type, bool force, bool commit, uint16_t
> zone,
> >const uint32_t *setmark,
> >const struct ovs_key_ct_labels *setlabel,
> >ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
> >const struct nat_action_info_t *nat_action_info,
> > -  long long now, uint32_t tp_id)
> > +  long long now, uint32_t tp_id, struct ipf_ctx
> *ipf_ctx)
> >  {
> >  ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> >   ct->hash_basis);
> > @@ -1468,9 +1468,8 @@ conntrack_execute(struct conntrack *ct, struct
> dp_packet_batch *pkt_batch,
> >  }
> >  }
> >
> > -ipf_postprocess_conntrack(ct->ipf, pkt_batch, now, dl_type);
> > -
> > -return 0;
> > +return ipf_postprocess_conntrack(ct->ipf, pkt_batch, ipf_ctx, \
> > + now, dl_type);
> >  }
> >
> >  void
> > diff --git a/lib/conntrack.h b/lib/conntrack.h
> > index 9553b188a4..211efad3f3 100644
> > --- a/lib/conntrack.h
> > +++ b/lib/conntrack.h
> > @@ -64,6 +64,7 @@
> >  struct dp_packet_batch;
> >
> >  struct conntrack;
> > +struct ipf_ctx;
> >
> >  union ct_addr {
> >  ovs_be32 ipv4;
> > @@ -88,13 +89,13 @@ struct nat_action_info_t {

Re: [ovs-dev] [PATCH] Documentation: Update USDT documentation to include systemtap dependency

[Ilya Maximets]

On 2/7/22 12:32, Eelco Chaudron wrote:
> Update the documentation to include details on SystemTap dependency
> when enabling USDT probes.
> 
> Suggested-by: Adrian Moreno 
> Signed-off-by: Eelco Chaudron 
> ---
>  Documentation/topics/usdt-probes.rst |5 +
>  1 file changed, 5 insertions(+)

Thanks!  Applied to master and 2.17.

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

Re: [ovs-dev] [PATCH v2] ovsdb-idl: Fix use-after-free when destroying an IDL loop.

[Ilya Maximets]

On 2/4/22 17:54, Dumitru Ceara wrote:
> Transactions that are still incomplete (waiting for a reply from the
> server) are kept in the IDL's 'outstanding_txns' map.  When a transaction
> is destroyed, ovsdb_idl_txn_destroy() will take care of removing the
> transaction from the 'outstanding_txns' map if the transaction was
> incomplete but also abort it and disassemble it if needed.
> 
> Aborting the transaction first, before ovsdb_idl_txn_destroy(), may
> cause an use-after-free if the transaction was outstanding; that's
> because the transaction would move to state "aborted" without being
> removed from the 'outstanding_txns' map.
> 
> Fixes: 53a540e5311c ("ovsdb-idl: ovsdb_idl_loop_destroy must also destroy the 
> committing txn.")
> Signed-off-by: Dumitru Ceara 
> ---
> v2:
> - Removed unnecessary ovsdb_idl_txn_disassemble() call per Ilya's
>   comment.
> - Rephrased the commit log.
> ---
>  lib/ovsdb-idl.c | 1 -
>  1 file changed, 1 deletion(-)

Thanks!  Applied and backported down to 2.13.

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

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

[Gaëtan Rivet]

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?

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

Re: [ovs-dev] [PATCH v1 0/3] Fix offload rule flush race condition

[Gaëtan Rivet]

On Wed, Feb 16, 2022, at 14:15, Ilya Maximets wrote:
> On 2/4/22 17:12, Gaetan Rivet wrote:
>> A race condition has been identified during datapath destruction,
>> along with the port offload flushes issued.
>> 
>> This series addresses these race conditions, cleaning up the
>> port deletion process.
>> 
>> The last patch also cleans up the offload structure.
>> It is not strictly necessary like the first two fixes,
>> so I put it last. It can wait until after the code-freeze
>> to be integrated.
>> 
>> I tested for a few hours without ASAN enabled without seeing
>> issues. ASAN has been executed as part of the github CI:
>> https://github.com/grivet/ovs/actions/runs/1795624401
>> It is however not too relevant, as no offloads are inserted during CI.
>> 
>> The following patch was used to fix an unrelated CI issue:
>> https://patchwork.ozlabs.org/project/openvswitch/patch/20220204150445.1481457-1-i.maxim...@ovn.org/
>> 
>> I also ran datapath creation + deletion loop with ASAN on an offload
>> test setup, but the execution was excruciatingly slow and could
>> not progress much. It reached datapath deletion without panicking
>> and no crash was seen, even though I had to interrupt the test after
>> a few hours.
>> 
>> Gaetan Rivet (2):
>>   dpif-netdev: Move port flush after datapath reconfiguration
>>   dpif-netdev: Use dp_netdev reference in offload threads
>> 
>> Sriharsha Basavapatna (1):
>>   dpif-netdev: Fix a race condition in deletion of offloaded flows
>> 
>>  lib/dpif-netdev.c | 71 ++-
>>  1 file changed, 40 insertions(+), 31 deletions(-)
>
>
> Thanks!  I applied the series to master and 2.17.
>
> I was thinking about backports and I think that it should be OK to
> backport patch #2 (always enqueue deletions), because it doesn't
> introduce any new issues.
>
> But I'm not sure about the first patch.  On 2.17 flush acts like
> a barrier, so no offload requests are in the offload queue after
> the port removal from PMD thread and flush from the offload thread.
> However, there is no such synchronization mechanism on 2.16.
> Flush is performed from the main thread and some offload requests
> could still be in the offload queue.  And if dp is destroyed,
> processing of those offload requests will cause use-after-free.
> In other words, while patch #1 can reduce the race window for flows
> remaining in the hardware after the port deletion, it doesn't solve
> the use-after-free problem.  So, we need some other solution.
> Backporting the barrier machinery doesn't seem like a good option.
>
> Referencing the dp on creation of the PMD thread or on creation
> of the offload item might be a solution. But this will change the
> time dp will actually be destroyed, so needs a careful consideration.
>
> We may also not fix the UAF problem on 2.16 taking into account that
> issue is happening only during deletion of the datapath, so should not
> be very severe and 2.16 is not an LTS branch.
>
> Thoughts?
>
> Best regards, Ilya Maximets.

I agree that the first patch is only correct as long as flush serves as
a synchronization point. Without the barrier, nothing ensures that no further
offload reqs exists in the queue.

Managing dp reference and implementing an offload barrier are two solutions
that have the potential to impact beyond the datapath destruction, potentially
introducing more severe bugs.

Another potential solution might be offload request cancellation, but
it is more complex than the barrier one, so even less doable on the branch.

I will keep trying to find another simple solution that could be used,
but so far I agree that the safest thing would be to leave it as-is
(or just reduce the race window with patch 1 & 2).

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

Re: [ovs-dev] [PATCH v3 12/17] hindex: remove the next variable in safe loops

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Use ovs_assert() in place of assert()
#124 FILE: tests/test-hindex.c:296:
assert(n == n_remaining);

ERROR: Use ovs_assert() in place of assert()
#133 FILE: tests/test-hindex.c:305:
assert(i < n);

ERROR: Use ovs_assert() in place of assert()
#138 FILE: tests/test-hindex.c:310:
assert(j < n_remaining);

ERROR: Use ovs_assert() in place of assert()
#148 FILE: tests/test-hindex.c:320:
assert(i == n);

Lines checked: 156, Warnings: 0, Errors: 4


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

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

Re: [ovs-dev] [PATCH v3 10/17] cmap: use multi-variable iterators

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Use ovs_assert() in place of assert()
#74 FILE: tests/test-cmap.c:77:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#82 FILE: tests/test-cmap.c:111:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#90 FILE: tests/test-cmap.c:135:
assert(e == NULL);

Lines checked: 97, Warnings: 0, Errors: 3


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

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

Re: [ovs-dev] [PATCH v3 09/17] hmap: use short version of safe loops if possible

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Inappropriate bracing around statement
#87 FILE: include/openvswitch/hmap.h:185:
HMAP_FOR_EACH_SAFE_LONG_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)

ERROR: Inappropriate bracing around statement
#102 FILE: include/openvswitch/hmap.h:198:
HMAP_FOR_EACH_SAFE_SHORT_INIT (NODE, MEMBER, HMAP, (void) 0)

ERROR: Use ovs_assert() in place of assert()
#1545 FILE: tests/test-hmap.c:277:
assert(n == n_remaining);

ERROR: Use ovs_assert() in place of assert()
#1554 FILE: tests/test-hmap.c:286:
assert(i < n);

ERROR: Use ovs_assert() in place of assert()
#1559 FILE: tests/test-hmap.c:291:
assert(j < n_remaining);

ERROR: Use ovs_assert() in place of assert()
#1569 FILE: tests/test-hmap.c:301:
assert(i == n);

ERROR: Use ovs_assert() in place of assert()
#1570 FILE: tests/test-hmap.c:302:
assert(e == NULL);

Lines checked: 1841, Warnings: 0, Errors: 7


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

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

Re: [ovs-dev] [PATCH v3 08/17] hmap: implement UB-safe hmap pop iterator

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Use ovs_assert() in place of assert()
#76 FILE: tests/test-hmap.c:320:
assert(e == NULL);

Lines checked: 83, 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

Re: [ovs-dev] [PATCH v3 07/17] hmap: use multi-variable helpers for hmap loops

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Inappropriate bracing around statement
#80 FILE: include/openvswitch/hmap.h:185:
HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)

ERROR: Inappropriate bracing around statement
#118 FILE: lib/ovs-numa.h:71:
HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->cores)

ERROR: Inappropriate bracing around statement
#122 FILE: lib/ovs-numa.h:74:
HMAP_FOR_EACH (ITER, hmap_node, &(DUMP)->numas)

ERROR: Use ovs_assert() in place of assert()
#133 FILE: tests/test-hmap.c:65:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#141 FILE: tests/test-hmap.c:86:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#150 FILE: tests/test-hmap.c:249:
assert(next == NULL);

ERROR: Use ovs_assert() in place of assert()
#152 FILE: tests/test-hmap.c:251:
assert(>node == hmap_next(, >node));

ERROR: Use ovs_assert() in place of assert()
#161 FILE: tests/test-hmap.c:269:
assert(next == NULL);

ERROR: Use ovs_assert() in place of assert()
#162 FILE: tests/test-hmap.c:270:
assert(e == NULL);

Lines checked: 169, Warnings: 0, Errors: 9


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

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

Re: [ovs-dev] [PATCH v3 06/17] list: use short version of safe loops if possible

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Use ovs_assert() in place of assert()
#1178 FILE: tests/test-list.c:176:
assert(i < n);

ERROR: Use ovs_assert() in place of assert()
#1191 FILE: tests/test-list.c:189:
assert(i == n);

ERROR: Use ovs_assert() in place of assert()
#1192 FILE: tests/test-list.c:190:
assert(e == NULL);

Lines checked: 1366, Warnings: 0, Errors: 3


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

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

Re: [ovs-dev] [PATCH v3 04/17] list: use multi-variable helpers for list loops

[0-day Robot]

Bleep bloop.  Greetings Adrian Moreno, 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: Use ovs_assert() in place of assert()
#130 FILE: tests/test-list.c:64:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#139 FILE: tests/test-list.c:73:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#149 FILE: tests/test-list.c:140:
assert(next == NULL);

ERROR: Use ovs_assert() in place of assert()
#151 FILE: tests/test-list.c:142:
assert(>node == e->node.next);

ERROR: Use ovs_assert() in place of assert()
#167 FILE: tests/test-list.c:159:
assert(e == NULL);

ERROR: Use ovs_assert() in place of assert()
#168 FILE: tests/test-list.c:160:
assert(next == NULL);

Lines checked: 175, Warnings: 0, Errors: 6


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 v3 17/17] sset: add SHORT version of SAFE loop macros

[Adrian Moreno]

Add SHORT version of SAFE loop macros and overload the current macro
name to keep backwards compatibility.

Signed-off-by: Adrian Moreno 
---
 lib/sset.c |  8 
 lib/sset.h | 15 ++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/sset.c b/lib/sset.c
index b2e3f43ec..c3197e305 100644
--- a/lib/sset.c
+++ b/lib/sset.c
@@ -212,9 +212,9 @@ sset_add_array(struct sset *set, char **names, size_t n)
 void
 sset_clear(struct sset *set)
 {
-const char *name, *next;
+const char *name;
 
-SSET_FOR_EACH_SAFE (name, next, set) {
+SSET_FOR_EACH_SAFE (name, set) {
 sset_delete(set, SSET_NODE_FROM_NAME(name));
 }
 }
@@ -320,9 +320,9 @@ sset_at_position(const struct sset *set, struct 
sset_position *pos)
 void
 sset_intersect(struct sset *a, const struct sset *b)
 {
-const char *name, *next;
+const char *name;
 
-SSET_FOR_EACH_SAFE (name, next, a) {
+SSET_FOR_EACH_SAFE (name, a) {
 if (!sset_contains(b, name)) {
 sset_delete(a, SSET_NODE_FROM_NAME(name));
 }
diff --git a/lib/sset.h b/lib/sset.h
index f0bb8b534..214d6fb41 100644
--- a/lib/sset.h
+++ b/lib/sset.h
@@ -87,13 +87,26 @@ void sset_intersect(struct sset *, const struct sset *);
  NAME != NULL;  \
  (NAME) = SSET_NEXT(SSET, NAME))
 
-#define SSET_FOR_EACH_SAFE(NAME, NEXT, SSET)\
+#define SSET_FOR_EACH_SAFE_LONG(NAME, NEXT, SSET)   \
 for ((NAME) = SSET_FIRST(SSET); \
  (NAME != NULL  \
   ? (NEXT) = SSET_NEXT(SSET, NAME), true\
   : false); \
  (NAME) = (NEXT))
 
+#define SSET_FOR_EACH_SAFE_SHORT(NAME, SSET)   \
+for (const char * NAME__next = \
+ ((NAME) = SSET_FIRST(SSET), NULL);\
+ (NAME != NULL \
+  ? (NAME__next = SSET_NEXT(SSET, NAME), true) \
+  : (NAME__next = NULL, false));   \
+ (NAME) = NAME__next)
+
+#define SSET_FOR_EACH_SAFE(...)\
+OVERLOAD_SAFE_MACRO(SSET_FOR_EACH_SAFE_LONG,   \
+SSET_FOR_EACH_SAFE_SHORT,  \
+3, __VA_ARGS__)
+
 const char **sset_array(const struct sset *);
 const char **sset_sort(const struct sset *);
 
-- 
2.34.1

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

[ovs-dev] [PATCH v3 16/17] sparse: bump recommended version and include headers

[Adrian Moreno]

It seems versions older than 0.6.2 generate false positives. Bump the
recommended version and make sure we uset the headers from the

Suggested-by: Dumitru Ceara 
Signed-off-by: Adrian Moreno 
---
 Documentation/intro/install/general.rst | 2 +-
 acinclude.m4| 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index c4300cd53..a297aadac 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -169,7 +169,7 @@ other than plain text, only if you have the following:
 If you are going to extensively modify Open vSwitch, consider installing the
 following to obtain better warnings:
 
-- "sparse" version 0.5.1 or later
+- "sparse" version 0.6.2 or later
   (https://git.kernel.org/pub/scm/devel/sparse/sparse.git/).
 
 - GNU make.
diff --git a/acinclude.m4 b/acinclude.m4
index 0c360fd1e..f704bf36c 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -1424,7 +1424,7 @@ AC_DEFUN([OVS_ENABLE_SPARSE],
: ${SPARSE=sparse}
AC_SUBST([SPARSE])
AC_CONFIG_COMMANDS_PRE(
- [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE) $(SPARSE_WERROR) 
-I $(top_srcdir)/include/sparse $(SPARSEFLAGS) $(SPARSE_EXTRA_INCLUDES) " cgcc 
$(CGCCFLAGS),'"$CC"')'])
+ [CC='$(if $(C:0=),env REAL_CC="'"$CC"'" CHECK="$(SPARSE) $(SPARSE_WERROR) 
-I $(top_srcdir)/include/sparse -I $(top_srcdir)/include $(SPARSEFLAGS) 
$(SPARSE_EXTRA_INCLUDES) " cgcc $(CGCCFLAGS),'"$CC"')'])
 
AC_ARG_ENABLE(
  [sparse],
-- 
2.34.1

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

[ovs-dev] [PATCH v3 15/17] idlc: support short version of SAFE macros

[Adrian Moreno]

In order to be consistent with the rest of the SAFE loop macros,
overload each of the generated *_SAFE macro with a SHORT version that
does not require the user to provide the NEXT variable.

Signed-off-by: Adrian Moreno 
---
 ovsdb/ovsdb-idlc.in   | 19 +--
 utilities/ovs-vsctl.c | 36 ++--
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 10a70ae26..13c535939 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -251,10 +251,18 @@ const struct %(s)s *%(s)s_table_first(const struct 
%(s)s_table *);
 for ((ROW) = %(s)s_table_first(TABLE); \\
  (ROW); \\
  (ROW) = %(s)s_next(ROW))
-#define %(S)s_TABLE_FOR_EACH_SAFE(ROW, NEXT, TABLE) \\
+#define %(S)s_TABLE_FOR_EACH_SAFE_LONG(ROW, NEXT, TABLE) \\
 for ((ROW) = %(s)s_table_first(TABLE); \\
  (ROW) ? ((NEXT) = %(s)s_next(ROW), 1) : 0; \\
  (ROW) = (NEXT))
+#define %(S)s_TABLE_FOR_EACH_SAFE_SHORT(ROW, TABLE) \\
+for (const struct %(s)s * ROW__next = ((ROW) = 
%(s)s_table_first(TABLE), NULL); \\
+ (ROW) ? (ROW__next = %(s)s_next(ROW), 1) : (ROW__next = NULL, 0); 
\\
+ (ROW) = ROW__next)
+#define %(S)s_TABLE_FOR_EACH_SAFE(...)
\\
+OVERLOAD_SAFE_MACRO(%(S)s_TABLE_FOR_EACH_SAFE_LONG,   
\\
+%(S)s_TABLE_FOR_EACH_SAFE_SHORT, 3, __VA_ARGS__)
+
 
 const struct %(s)s *%(s)s_get_for_uuid(const struct ovsdb_idl *, const struct 
uuid *);
 const struct %(s)s *%(s)s_table_get_for_uuid(const struct %(s)s_table *, const 
struct uuid *);
@@ -264,10 +272,17 @@ const struct %(s)s *%(s)s_next(const struct %(s)s *);
 for ((ROW) = %(s)s_first(IDL); \\
  (ROW); \\
  (ROW) = %(s)s_next(ROW))
-#define %(S)s_FOR_EACH_SAFE(ROW, NEXT, IDL) \\
+#define %(S)s_FOR_EACH_SAFE_LONG(ROW, NEXT, IDL) \\
 for ((ROW) = %(s)s_first(IDL); \\
  (ROW) ? ((NEXT) = %(s)s_next(ROW), 1) : 0; \\
  (ROW) = (NEXT))
+#define %(S)s_FOR_EACH_SAFE_SHORT(ROW, IDL) \\
+for (const struct %(s)s * ROW__next = ((ROW) = %(s)s_first(IDL), 
NULL); \\
+ (ROW) ? (ROW__next = %(s)s_next(ROW), 1) : (ROW__next = NULL, 0); 
\\
+ (ROW) = ROW__next)
+#define %(S)s_FOR_EACH_SAFE(...) \\
+OVERLOAD_SAFE_MACRO(%(S)s_FOR_EACH_SAFE_LONG,\\
+%(S)s_FOR_EACH_SAFE_SHORT, 3, __VA_ARGS__)
 
 unsigned int %(s)s_get_seqno(const struct ovsdb_idl *);
 unsigned int %(s)s_row_get_seqno(const struct %(s)s *row, enum 
ovsdb_idl_change change);
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 14f5cb92e..1032089fc 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1100,14 +1100,14 @@ cmd_emer_reset(struct ctl_context *ctx)
 const struct ovsrec_bridge *br;
 const struct ovsrec_port *port;
 const struct ovsrec_interface *iface;
-const struct ovsrec_mirror *mirror, *next_mirror;
-const struct ovsrec_controller *ctrl, *next_ctrl;
-const struct ovsrec_manager *mgr, *next_mgr;
-const struct ovsrec_netflow *nf, *next_nf;
-const struct ovsrec_ssl *ssl, *next_ssl;
-const struct ovsrec_sflow *sflow, *next_sflow;
-const struct ovsrec_ipfix *ipfix, *next_ipfix;
-const struct ovsrec_flow_sample_collector_set *fscset, *next_fscset;
+const struct ovsrec_mirror *mirror;
+const struct ovsrec_controller *ctrl;
+const struct ovsrec_manager *mgr;
+const struct ovsrec_netflow *nf;
+const struct ovsrec_ssl *ssl;
+const struct ovsrec_sflow *sflow;
+const struct ovsrec_ipfix *ipfix;
+const struct ovsrec_flow_sample_collector_set *fscset;
 
 /* Reset the Open_vSwitch table. */
 ovsrec_open_vswitch_set_manager_options(vsctl_ctx->ovs, NULL, 0);
@@ -1145,35 +1145,35 @@ cmd_emer_reset(struct ctl_context *ctx)
 ovsrec_interface_set_ingress_policing_burst(iface, 0);
 }
 
-OVSREC_MIRROR_FOR_EACH_SAFE (mirror, next_mirror, idl) {
+OVSREC_MIRROR_FOR_EACH_SAFE (mirror, idl) {
 ovsrec_mirror_delete(mirror);
 }
 
-OVSREC_CONTROLLER_FOR_EACH_SAFE (ctrl, next_ctrl, idl) {
+OVSREC_CONTROLLER_FOR_EACH_SAFE (ctrl, idl) {
 ovsrec_controller_delete(ctrl);
 }
 
-OVSREC_MANAGER_FOR_EACH_SAFE (mgr, next_mgr, idl) {
+OVSREC_MANAGER_FOR_EACH_SAFE (mgr, idl) {
 ovsrec_manager_delete(mgr);
 }
 
-OVSREC_NETFLOW_FOR_EACH_SAFE (nf, next_nf, idl) {
+OVSREC_NETFLOW_FOR_EACH_SAFE (nf, idl) {
 ovsrec_netflow_delete(nf);
 }
 
-OVSREC_SSL_FOR_EACH_SAFE (ssl, next_ssl, idl) {
+OVSREC_SSL_FOR_EACH_SAFE (ssl, idl) {
 ovsrec_ssl_delete(ssl);
 }
 
-OVSREC_SFLOW_FOR_EACH_SAFE (sflow, next_sflow, idl) {
+OVSREC_SFLOW_FOR_EACH_SAFE (sflow, idl) {
 ovsrec_sflow_delete(sflow);
 }
 
-

[ovs-dev] [PATCH v3 14/17] vtep: use _SAFE iterator if freeing the iterator

[Adrian Moreno]

The _SAFE version of the iterator allows the user to free the iterator
structure safely.

Signed-off-by: Adrian Moreno 
---
 vtep/vtep-ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
index 99c4adcd5..685b0007a 100644
--- a/vtep/vtep-ctl.c
+++ b/vtep/vtep-ctl.c
@@ -738,7 +738,7 @@ del_ploc_from_mcast_mac(struct vtep_ctl_mcast_mac 
*mcast_mac,
 {
 struct vtep_ctl_ploc *ploc;
 
-LIST_FOR_EACH (ploc, locators_node, _mac->locators) {
+LIST_FOR_EACH_SAFE (ploc, locators_node, _mac->locators) {
 if (ploc->ploc_cfg == ploc_cfg) {
 ovs_list_remove(>locators_node);
 free(ploc);
-- 
2.34.1

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

[ovs-dev] [PATCH v3 12/17] hindex: remove the next variable in safe loops

[Adrian Moreno]

Using SHORT version of the *_SAFE loops makes the code cleaner and less
error prone. So, use the SHORT version and remove the extra variable
when possible for HINDEX_*_SAFE.

In order to be able to use both long and short versions without changing
the name of the macro for all the clients, overload the existing name
and select the appropriate version depending on the number of arguments.

Signed-off-by: Adrian Moreno 
---
 lib/conntrack.c |  4 ++--
 lib/hindex.h| 48 -
 tests/test-hindex.c | 31 +
 3 files changed, 72 insertions(+), 11 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 9c96b69e4..2f0777102 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -2857,8 +2857,8 @@ expectation_clean(struct conntrack *ct, const struct 
conn_key *parent_key)
 {
 ovs_rwlock_wrlock(>resources_lock);
 
-struct alg_exp_node *node, *next;
-HINDEX_FOR_EACH_WITH_HASH_SAFE (node, next, node_ref,
+struct alg_exp_node *node;
+HINDEX_FOR_EACH_WITH_HASH_SAFE (node, node_ref,
 conn_key_hash(parent_key, ct->hash_basis),
 >alg_expectation_refs) {
 if (!conn_key_cmp(>parent_key, parent_key)) {
diff --git a/lib/hindex.h b/lib/hindex.h
index fa252e6dd..61e6876a9 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -135,16 +135,32 @@ void hindex_remove(struct hindex *, struct hindex_node *);
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
-#define HINDEX_FOR_EACH_WITH_HASH_SAFE(NODE, NEXT, MEMBER, HASH, HINDEX)\
-for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,\
-hindex_node_with_hash(HINDEX, HASH),\
-struct hindex_node);\
- CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,   \
-  ITER_VAR(NODE) != NULL,   \
-  ITER_VAR(NEXT) = ITER_VAR(NODE)->s,   \
-  ITER_VAR(NEXT) != NULL);  \
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE_LONG(NODE, NEXT, MEMBER, HASH, HINDEX) \
+for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,  \
+hindex_node_with_hash(HINDEX, HASH),  \
+struct hindex_node);  \
+ CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
+  ITER_VAR(NODE) != NULL, \
+  ITER_VAR(NEXT) = ITER_VAR(NODE)->s, \
+  ITER_VAR(NEXT) != NULL);\
  UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
+/* Short version of HINDEX_FOR_EACH_WITH_HASH_SAFE */
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE_SHORT(NODE, MEMBER, HASH, HINDEX)  \
+for (INIT_MULTIVAR_SAFE_SHORT(NODE, MEMBER,   \
+hindex_node_with_hash(HINDEX, HASH),  \
+struct hindex_node);  \
+ CONDITION_MULTIVAR_SAFE_SHORT(NODE, MEMBER,  \
+   ITER_VAR(NODE) != NULL,\
+ ITER_NEXT_VAR(NODE) = ITER_VAR(NODE)->s);\
+ UPDATE_MULTIVAR_SAFE_SHORT(NODE))
+
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE(...)   \
+OVERLOAD_SAFE_MACRO(HINDEX_FOR_EACH_WITH_HASH_SAFE_LONG,  \
+HINDEX_FOR_EACH_WITH_HASH_SAFE_SHORT, \
+5, __VA_ARGS__)
+
+
 /* Returns the head node in 'hindex' with the given 'hash', or a null pointer
  * if no nodes have that hash value. */
 static inline struct hindex_node *
@@ -170,7 +186,7 @@ hindex_node_with_hash(const struct hindex *hindex, size_t 
hash)
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash index but its members remain accessible and intact). */
-#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)  \
+#define HINDEX_FOR_EACH_SAFE_LONG(NODE, NEXT, MEMBER, HINDEX) \
 for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, hindex_first(HINDEX),\
  struct hindex_node); \
  CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
@@ -179,6 +195,20 @@ hindex_node_with_hash(const struct hindex *hindex, size_t 
hash)
   ITER_VAR(NEXT) != NULL);\
  UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
+/* Short version of HINDEX_FOR_EACH_SAFE */
+#define 

[ovs-dev] [PATCH v3 13/17] rculist: use multi-variable helpers for loop macros

[Adrian Moreno]

Use multi-variable iteration helpers to rewrite rculist loops macros.

There is an important behavior change compared with the previous
implementation: When the loop ends normally (i.e: not via "break;"),
the object pointer provided by the user is NULL. This is safer
because it's not guaranteed that it would end up pointing a valid
address.

Signed-off-by: Adrian Moreno 
---
 lib/rculist.h | 83 ---
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/lib/rculist.h b/lib/rculist.h
index 1072b87af..744e4363c 100644
--- a/lib/rculist.h
+++ b/lib/rculist.h
@@ -365,35 +365,58 @@ rculist_is_singleton_protected(const struct rculist *list)
 return list_next == list->prev && list_next != list;
 }
 
-#define RCULIST_FOR_EACH(ITER, MEMBER, RCULIST) \
-for (INIT_CONTAINER(ITER, rculist_next(RCULIST), MEMBER);   \
- &(ITER)->MEMBER != (RCULIST);  \
- ASSIGN_CONTAINER(ITER, rculist_next(&(ITER)->MEMBER), MEMBER))
-#define RCULIST_FOR_EACH_CONTINUE(ITER, MEMBER, RCULIST)\
-for (ASSIGN_CONTAINER(ITER, rculist_next(&(ITER)->MEMBER), MEMBER); \
- &(ITER)->MEMBER != (RCULIST);  \
- ASSIGN_CONTAINER(ITER, rculist_next(&(ITER)->MEMBER), MEMBER))
-
-#define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST)   \
-for (INIT_CONTAINER(ITER, (RCULIST)->prev, MEMBER); \
- &(ITER)->MEMBER != (RCULIST);  \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, RCULIST) \
-for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER);   \
- &(ITER)->MEMBER != (RCULIST);  \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-
-#define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST)   \
-for (INIT_CONTAINER(ITER, rculist_next_protected(RCULIST), MEMBER); \
- &(ITER)->MEMBER != (RCULIST);  \
- ASSIGN_CONTAINER(ITER, rculist_next_protected(&(ITER)->MEMBER), \
-  MEMBER))
-
-#define RCULIST_FOR_EACH_SAFE_PROTECTED(ITER, NEXT, MEMBER, RCULIST)\
-for (INIT_CONTAINER(ITER, rculist_next_protected(RCULIST), MEMBER); \
- (&(ITER)->MEMBER != (RCULIST)  \
-  ? INIT_CONTAINER(NEXT, rculist_next_protected(&(ITER)->MEMBER), \
-   MEMBER), 1 : 0); \
- (ITER) = (NEXT))
+#define RCULIST_FOR_EACH(ITER, MEMBER, RCULIST)   \
+for (INIT_MULTIVAR(ITER, MEMBER, rculist_next(RCULIST),   \
+   const struct rculist); \
+ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));   \
+ UPDATE_MULTIVAR(ITER, ITER_VAR(ITER) = rculist_next(ITER_VAR(ITER
+
+#define RCULIST_FOR_EACH_CONTINUE(ITER, MEMBER, RCULIST)  \
+for (INIT_MULTIVAR(ITER, MEMBER, rculist_next(&(ITER)->MEMBER),   \
+   const struct rculist); \
+ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));   \
+ UPDATE_MULTIVAR(ITER, ITER_VAR(ITER) = rculist_next(ITER_VAR(ITER
+
+#define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST) \
+for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist);\
+ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));   \
+ UPDATE_MULTIVAR(ITER, ITER_VAR(ITER) = ITER_VAR(VAR).prev))
+
+#define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, RCULIST)\
+for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct rculist);\
+ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));   \
+ UPDATE_MULTIVAR(ITER, ITER_VAR(ITER) = ITER_VAR(VAR).prev))
+
+#define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST) \
+for (INIT_MULTIVAR(ITER, MEMBER, rculist_next_protected(RCULIST), \
+   struct rculist);   \
+ CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));   \
+ UPDATE_MULTIVAR(ITER,\
+ ITER_VAR(ITER) = rculist_next_protected(ITER_VAR(ITER))) \
+
+#define RCULIST_FOR_EACH_SAFE_SHORT_PROTECTED(ITER, MEMBER, RCULIST)  \
+for (INIT_MULTIVAR_SAFE_SHORT(ITER, MEMBER,   \
+  rculist_next_protected(RCULIST),\
+  struct rculist);\
+ CONDITION_MULTIVAR_SAFE_SHORT(ITER, MEMBER,   

[ovs-dev] [PATCH v3 11/17] hindex: use multi-variable iterators

[Adrian Moreno]

Re-write hindex's loops using multi-variable helpers.

For safe loops, use the LONG version to maintain backwards
compatibility.

Signed-off-by: Adrian Moreno 
---
 lib/hindex.h| 46 ++---
 tests/test-hindex.c |  6 ++
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/lib/hindex.h b/lib/hindex.h
index 876c5a9e3..fa252e6dd 100644
--- a/lib/hindex.h
+++ b/lib/hindex.h
@@ -128,18 +128,22 @@ void hindex_remove(struct hindex *, struct hindex_node *);
  * Evaluates HASH only once.
  */
 #define HINDEX_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HINDEX)   \
-for (INIT_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
- NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); \
- ASSIGN_CONTAINER(NODE, (NODE)->MEMBER.s, MEMBER))
+for (INIT_MULTIVAR(NODE, MEMBER, hindex_node_with_hash(HINDEX, HASH),   \
+   struct hindex_node); \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);  \
+ UPDATE_MULTIVAR(NODE, ITER_VAR(NODE) = ITER_VAR(NODE)->s))
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
-#define HINDEX_FOR_EACH_WITH_HASH_SAFE(NODE, NEXT, MEMBER, HASH, HINDEX) \
-for (INIT_CONTAINER(NODE, hindex_node_with_hash(HINDEX, HASH), MEMBER); \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) \
-  ? INIT_CONTAINER(NEXT, (NODE)->MEMBER.s, MEMBER), 1   \
-  : 0); \
- (NODE) = (NEXT))
+#define HINDEX_FOR_EACH_WITH_HASH_SAFE(NODE, NEXT, MEMBER, HASH, HINDEX)\
+for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,\
+hindex_node_with_hash(HINDEX, HASH),\
+struct hindex_node);\
+ CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER,   \
+  ITER_VAR(NODE) != NULL,   \
+  ITER_VAR(NEXT) = ITER_VAR(NODE)->s,   \
+  ITER_VAR(NEXT) != NULL);  \
+ UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
 /* Returns the head node in 'hindex' with the given 'hash', or a null pointer
  * if no nodes have that hash value. */
@@ -157,19 +161,23 @@ hindex_node_with_hash(const struct hindex *hindex, size_t 
hash)
 /* Iteration. */
 
 /* Iterates through every node in HINDEX. */
-#define HINDEX_FOR_EACH(NODE, MEMBER, HINDEX)   \
-for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);\
- NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER); \
- ASSIGN_CONTAINER(NODE, hindex_next(HINDEX, &(NODE)->MEMBER), MEMBER))
+#define HINDEX_FOR_EACH(NODE, MEMBER, HINDEX) \
+for (INIT_MULTIVAR(NODE, MEMBER, hindex_first(HINDEX),\
+   struct hindex_node);   \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);\
+ UPDATE_MULTIVAR(NODE,\
+ ITER_VAR(NODE) = hindex_next(HINDEX, ITER_VAR(NODE
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash index but its members remain accessible and intact). */
-#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)  \
-for (INIT_CONTAINER(NODE, hindex_first(HINDEX), MEMBER);  \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER) \
-  ? INIT_CONTAINER(NEXT, hindex_next(HINDEX, &(NODE)->MEMBER), 
MEMBER), 1 \
-  : 0); \
- (NODE) = (NEXT))
+#define HINDEX_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HINDEX)  \
+for (INIT_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, hindex_first(HINDEX),\
+ struct hindex_node); \
+ CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
+  ITER_VAR(NODE) != NULL, \
+ITER_VAR(NEXT) = hindex_next(HINDEX, ITER_VAR(NODE)), \
+  ITER_VAR(NEXT) != NULL);\
+ UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
 struct hindex_node *hindex_first(const struct hindex *);
 struct hindex_node *hindex_next(const struct hindex *,
diff --git a/tests/test-hindex.c b/tests/test-hindex.c
index af06be5fc..95e49284e 100644
--- a/tests/test-hindex.c
+++ b/tests/test-hindex.c
@@ -265,6 +265,11 @@ test_hindex_for_each_safe(hash_func *hash)
 i = 0;
 n_remaining = n;
   

[ovs-dev] [PATCH v3 09/17] hmap: use short version of safe loops if possible

[Adrian Moreno]

Using SHORT version of the *_SAFE loops makes the code cleaner and less
error prone. So, use the SHORT version and remove the extra variable
when possible for hmap and all its derived types.

In order to be able to use both long and short versions without changing
the name of the macro for all the clients, overload the existing name
and select the appropriate version depending on the number of arguments.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/hmap.h   | 26 ++---
 include/openvswitch/shash.h  | 15 --
 lib/cfm.c|  4 +--
 lib/classifier.c |  4 +--
 lib/dns-resolve.c|  4 +--
 lib/dpif-netdev.c| 19 ++--
 lib/hmapx.c  |  4 +--
 lib/hmapx.h  | 14 +++--
 lib/json.c   |  4 +--
 lib/lacp.c   |  4 +--
 lib/mac-learning.c   |  4 +--
 lib/namemap.c|  4 +--
 lib/netdev-dpdk.c|  4 +--
 lib/netdev-linux.c   |  4 +--
 lib/netdev-offload-tc.c  |  4 +--
 lib/ofp-msgs.c   |  4 +--
 lib/ovsdb-cs.c   | 12 
 lib/ovsdb-idl.c  | 16 +--
 lib/ovsdb-map-op.c   |  4 +--
 lib/ovsdb-set-op.c   |  4 +--
 lib/pcap-file.c  |  4 +--
 lib/perf-counter.c   |  4 +--
 lib/poll-loop.c  |  4 +--
 lib/seq.c|  4 +--
 lib/shash.c  |  8 +++---
 lib/simap.c  |  4 +--
 lib/simap.h  | 16 +--
 lib/smap.c   |  4 +--
 lib/smap.h   | 15 --
 lib/stopwatch.c  |  4 +--
 ofproto/bond.c   |  4 +--
 ofproto/connmgr.c| 20 ++---
 ofproto/in-band.c|  4 +--
 ofproto/netflow.c|  8 +++---
 ofproto/ofproto-dpif-ipfix.c |  8 +++---
 ofproto/ofproto-dpif-sflow.c |  4 +--
 ofproto/ofproto-dpif-xlate.c |  8 +++---
 ofproto/ofproto-dpif.c   |  4 +--
 ofproto/ofproto.c| 16 +--
 ovsdb/condition.c|  8 +++---
 ovsdb/jsonrpc-server.c   | 24 
 ovsdb/monitor.c  | 12 
 ovsdb/ovsdb-server.c | 11 ---
 ovsdb/ovsdb-tool.c   |  7 ++---
 ovsdb/query.c|  4 +--
 ovsdb/raft-private.c |  4 +--
 ovsdb/raft.c | 18 ++--
 ovsdb/relay.c|  4 +--
 ovsdb/replication.c  |  8 +++---
 ovsdb/table.c|  4 +--
 ovsdb/transaction-forward.c  |  4 +--
 ovsdb/transaction.c  |  8 +++---
 tests/test-cmap.c|  4 +--
 tests/test-hmap.c| 32 +
 tests/test-list.c|  2 +-
 utilities/ovs-vsctl.c|  4 +--
 vswitchd/bridge.c| 56 ++--
 vtep/vtep-ctl.c  |  6 ++--
 58 files changed, 305 insertions(+), 216 deletions(-)

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index 91a2c1257..0d67ca819 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -181,18 +181,36 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
-#define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP) \
-HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)
+#define HMAP_FOR_EACH_SAFE_LONG(NODE, NEXT, MEMBER, HMAP) \
+HMAP_FOR_EACH_SAFE_LONG_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)
 
-#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)\
+#define HMAP_FOR_EACH_SAFE_LONG_INIT(NODE, NEXT, MEMBER, HMAP, ...)   \
 for (INIT_MULTIVAR_SAFE_LONG_EXP(NODE, NEXT, MEMBER, hmap_first(HMAP),\
-  struct hmap_node, __VA_ARGS__); \
+ struct hmap_node, __VA_ARGS__);  \
  CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
   ITER_VAR(NODE) != NULL, \
 ITER_VAR(NEXT) = hmap_next(HMAP, ITER_VAR(NODE)), \
   ITER_VAR(NEXT) != NULL);\
  UPDATE_MULTIVAR_SAFE_LONG(NODE, NEXT))
 
+/* Short versions of HMAP_FOR_EACH_SAFE */
+#define HMAP_FOR_EACH_SAFE_SHORT(NODE, MEMBER, HMAP)  \
+HMAP_FOR_EACH_SAFE_SHORT_INIT (NODE, MEMBER, HMAP, (void) 0)
+
+#define HMAP_FOR_EACH_SAFE_SHORT_INIT(NODE, MEMBER, HMAP, ...)\
+for (INIT_MULTIVAR_SAFE_SHORT_EXP(NODE, MEMBER, hmap_first(HMAP), \
+  struct hmap_node, __VA_ARGS__); \
+ CONDITION_MULTIVAR_SAFE_SHORT(NODE, MEMBER,  \
+   ITER_VAR(NODE) != NULL,\
+

[ovs-dev] [PATCH v3 10/17] cmap: use multi-variable iterators

[Adrian Moreno]

Re-write cmap's loops using multi-variable helpers.

For iterators based on cmap_cursor, we just need to make sure the NODE
variable is not used after the loop, so we set it to NULL.

Signed-off-by: Adrian Moreno 
---
 lib/cmap.h| 24 ++--
 tests/test-cmap.c |  3 +++
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/cmap.h b/lib/cmap.h
index c502d2311..9098b373d 100644
--- a/lib/cmap.h
+++ b/lib/cmap.h
@@ -108,6 +108,8 @@ size_t cmap_replace(struct cmap *, struct cmap_node 
*old_node,
  *
  * CMAP and HASH are evaluated only once.  NODE is evaluated many times.
  *
+ * After a normal exit of the loop (not through a "break;" statement) NODE is
+ * NULL.
  *
  * Thread-safety
  * =
@@ -128,15 +130,17 @@ size_t cmap_replace(struct cmap *, struct cmap_node 
*old_node,
  * CMAP_FOR_EACH_WITH_HASH_PROTECTED may only be used if CMAP is guaranteed not
  * to change during iteration.  It may be very slightly faster.
  */
-#define CMAP_NODE_FOR_EACH(NODE, MEMBER, CMAP_NODE) \
-for (INIT_CONTAINER(NODE, CMAP_NODE, MEMBER);   \
- (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);   \
- ASSIGN_CONTAINER(NODE, cmap_node_next(&(NODE)->MEMBER), MEMBER))
-#define CMAP_NODE_FOR_EACH_PROTECTED(NODE, MEMBER, CMAP_NODE)   \
-for (INIT_CONTAINER(NODE, CMAP_NODE, MEMBER);   \
- (NODE) != OBJECT_CONTAINING(NULL, NODE, MEMBER);   \
- ASSIGN_CONTAINER(NODE, cmap_node_next_protected(&(NODE)->MEMBER), \
-  MEMBER))
+#define CMAP_NODE_FOR_EACH(NODE, MEMBER, CMAP_NODE)\
+for (INIT_MULTIVAR(NODE, MEMBER, CMAP_NODE, struct cmap_node); \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL); \
+ UPDATE_MULTIVAR(NODE, \
+ ITER_VAR(NODE) = cmap_node_next(ITER_VAR(NODE
+#define CMAP_NODE_FOR_EACH_PROTECTED(NODE, MEMBER, CMAP_NODE)  \
+for (INIT_MULTIVAR(NODE, MEMBER, CMAP_NODE, struct cmap_node); \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL); \
+ UPDATE_MULTIVAR(NODE, \
+ITER_VAR(NODE) = cmap_node_next_protected(ITER_VAR(NODE
+
 #define CMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, CMAP)   \
 CMAP_NODE_FOR_EACH(NODE, MEMBER, cmap_find(CMAP, HASH))
 #define CMAP_FOR_EACH_WITH_HASH_PROTECTED(NODE, MEMBER, HASH, CMAP) \
@@ -223,7 +227,7 @@ unsigned long cmap_find_batch(const struct cmap *cmap, 
unsigned long map,
  ? (INIT_CONTAINER(NODE, (CURSOR)->node, MEMBER),   \
 cmap_cursor_advance(CURSOR),\
 true)   \
- : false)
+ : (NODE = NULL, false))
 
 #define CMAP_CURSOR_FOR_EACH(NODE, MEMBER, CURSOR, CMAP)\
 for (*(CURSOR) = cmap_cursor_start(CMAP);   \
diff --git a/tests/test-cmap.c b/tests/test-cmap.c
index bc1f45642..588a5dea6 100644
--- a/tests/test-cmap.c
+++ b/tests/test-cmap.c
@@ -74,6 +74,7 @@ check_cmap(struct cmap *cmap, const int values[], size_t n,
 cmap_values[i++] = e->value;
 }
 assert(i == n);
+assert(e == NULL);
 
 /* Here we test iteration with cmap_next_position() */
 i = 0;
@@ -107,6 +108,7 @@ check_cmap(struct cmap *cmap, const int values[], size_t n,
 count += e->value == values[i];
 }
 assert(count == 1);
+assert(e == NULL);
 }
 
 /* Check that all the values are there in batched lookup. */
@@ -130,6 +132,7 @@ check_cmap(struct cmap *cmap, const int values[], size_t n,
 CMAP_NODE_FOR_EACH (e, node, nodes[k]) {
 count += e->value == values[i + k];
 }
+assert(e == NULL);
 }
 assert(count == j); /* j elements in a batch. */
 }
-- 
2.34.1

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

[ovs-dev] [PATCH v3 08/17] hmap: implement UB-safe hmap pop iterator

[Adrian Moreno]

HMAP_FOR_EACH_POP iterator has an additional difficulty, which is the
use of two iterator variables of different types.

In order to re-write this loop in a UB-safe manner, create a iterator
struct to be used as loop variable.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/hmap.h | 31 +++
 tests/test-hmap.c  |  1 +
 2 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index 9827e993d..91a2c1257 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -203,26 +203,33 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
  UPDATE_MULTIVAR(NODE,\
  ITER_VAR(NODE) = hmap_next(HMAP, ITER_VAR(NODE   \
 
-static inline struct hmap_node *
-hmap_pop_helper__(struct hmap *hmap, size_t *bucket) {
+struct hmap_pop_helper_iter__ {
+size_t bucket;
+struct hmap_node *node;
+};
+
+static inline void
+hmap_pop_helper__(struct hmap *hmap, struct hmap_pop_helper_iter__ *iter) {
 
-for (; *bucket <= hmap->mask; (*bucket)++) {
-struct hmap_node *node = hmap->buckets[*bucket];
+for (; iter->bucket <= hmap->mask; (iter->bucket)++) {
+struct hmap_node *node = hmap->buckets[iter->bucket];
 
 if (node) {
 hmap_remove(hmap, node);
-return node;
+iter->node = node;
+return;
 }
 }
-
-return NULL;
+iter->node = NULL;
 }
 
-#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP)   \
-for (size_t bucket__ = 0;   \
- INIT_CONTAINER(NODE, hmap_pop_helper__(HMAP, __), MEMBER),  \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))\
- || ((NODE = NULL), false);)
+#define HMAP_FOR_EACH_POP(NODE, MEMBER, HMAP) \
+for (struct hmap_pop_helper_iter__ ITER_VAR(NODE) = { 0, NULL };  \
+ hmap_pop_helper__(HMAP, _VAR(NODE)),\
+ (ITER_VAR(NODE).node != NULL) ?  \
+(((NODE) = OBJECT_CONTAINING(ITER_VAR(NODE).node, \
+ NODE, MEMBER)),1):   \
+(((NODE) = NULL), 0);)
 
 static inline struct hmap_node *hmap_first(const struct hmap *);
 static inline struct hmap_node *hmap_next(const struct hmap *,
diff --git a/tests/test-hmap.c b/tests/test-hmap.c
index a40ac8953..47b475538 100644
--- a/tests/test-hmap.c
+++ b/tests/test-hmap.c
@@ -317,6 +317,7 @@ test_hmap_for_each_pop(hash_func *hash)
 i++;
 }
 assert(i == n);
+assert(e == NULL);
 
 hmap_destroy();
 }
-- 
2.34.1

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

[ovs-dev] [PATCH v3 06/17] list: use short version of safe loops if possible

[Adrian Moreno]

Using the SHORT version of the *_SAFE loops makes the code cleaner
and less error-prone. So, use the SHORT version and remove the extra
variable when possible.

In order to be able to use both long and short versions without changing
the name of the macro for all the clients, overload the existing name
and select the appropriate version depending on the number of arguments.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/list.h   | 30 --
 lib/conntrack.c  |  4 ++--
 lib/fat-rwlock.c |  4 ++--
 lib/id-fpool.c   |  3 +--
 lib/ipf.c| 22 +++---
 lib/lldp/lldpd-structs.c |  7 +++
 lib/lldp/lldpd.c |  8 
 lib/mcast-snooping.c | 12 ++--
 lib/netdev-afxdp.c   |  4 ++--
 lib/netdev-dpdk.c|  4 ++--
 lib/ofp-msgs.c   |  4 ++--
 lib/ovs-lldp.c   | 12 ++--
 lib/ovsdb-idl.c  | 30 +++---
 lib/seq.c|  4 ++--
 lib/tnl-ports.c  | 16 
 lib/unixctl.c|  8 
 lib/vconn.c  |  4 ++--
 ofproto/connmgr.c|  8 
 ofproto/ofproto-dpif-ipfix.c |  4 ++--
 ofproto/ofproto-dpif-trace.c |  4 ++--
 ofproto/ofproto-dpif-xlate.c |  4 ++--
 ofproto/ofproto-dpif.c   | 24 +++-
 ovsdb/jsonrpc-server.c   | 16 
 ovsdb/monitor.c  | 24 
 ovsdb/ovsdb.c|  4 ++--
 ovsdb/raft.c | 15 +++
 ovsdb/transaction-forward.c  |  6 +++---
 ovsdb/transaction.c  | 28 ++--
 ovsdb/trigger.c  |  4 ++--
 tests/test-list.c| 29 +
 utilities/ovs-ofctl.c|  4 ++--
 utilities/ovs-vsctl.c|  8 
 vswitchd/bridge.c| 16 
 vtep/vtep-ctl.c  | 12 ++--
 34 files changed, 218 insertions(+), 168 deletions(-)

diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h
index 8cd76b892..77a611051 100644
--- a/include/openvswitch/list.h
+++ b/include/openvswitch/list.h
@@ -93,7 +93,8 @@ static inline bool ovs_list_is_short(const struct ovs_list *);
  CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));\
  UPDATE_MULTIVAR(VAR, (ITER_VAR(VAR) = ITER_VAR(VAR)->prev)))
 
-#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST)   \
+/* LONG version of SAFE iterators */
+#define LIST_FOR_EACH_REVERSE_SAFE_LONG(VAR, PREV, MEMBER, LIST)  \
 for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, \
  struct ovs_list);\
  CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER,  \
@@ -102,7 +103,7 @@ static inline bool ovs_list_is_short(const struct ovs_list 
*);
   ITER_VAR(PREV) != (LIST));  \
  UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV))
 
-#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST)   \
+#define LIST_FOR_EACH_SAFE_LONG(VAR, NEXT, MEMBER, LIST)  \
 for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, \
  struct ovs_list);\
  CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER,  \
@@ -111,6 +112,31 @@ static inline bool ovs_list_is_short(const struct ovs_list 
*);
   ITER_VAR(NEXT) != (LIST));  \
  UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT))
 
+/* SHORT version of SAFE iterators */
+#define LIST_FOR_EACH_REVERSE_SAFE_SHORT(VAR, MEMBER, LIST)   \
+for (INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, (LIST)->prev, struct ovs_list);\
+ CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER,   \
+   ITER_VAR(VAR) != (LIST),   \
+ ITER_NEXT_VAR(VAR) = ITER_VAR(VAR)->prev);   \
+ UPDATE_MULTIVAR_SAFE_SHORT(VAR))
+
+#define LIST_FOR_EACH_SAFE_SHORT(VAR, MEMBER, LIST)   \
+for (INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, (LIST)->next, struct ovs_list);\
+ CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER,   \
+   ITER_VAR(VAR) != (LIST),   \
+ ITER_NEXT_VAR(VAR) = ITER_VAR(VAR)->next);   \
+ UPDATE_MULTIVAR_SAFE_SHORT(VAR))
+
+#define LIST_FOR_EACH_SAFE(...)  \
+OVERLOAD_SAFE_MACRO(LIST_FOR_EACH_SAFE_LONG, \
+LIST_FOR_EACH_SAFE_SHORT,\
+4, __VA_ARGS__)
+
+#define LIST_FOR_EACH_REVERSE_SAFE(...)\
+

[ovs-dev] [PATCH v3 07/17] hmap: use multi-variable helpers for hmap loops

[Adrian Moreno]

Rewrite hmap's loops using multi-variable helpers.

For SAFE loops, use the LONG version of the multi-variable macros to
keep backwards compatibility.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/hmap.h | 65 --
 lib/ovs-numa.h |  4 +--
 tests/test-hmap.c  |  9 ++
 3 files changed, 46 insertions(+), 32 deletions(-)

diff --git a/include/openvswitch/hmap.h b/include/openvswitch/hmap.h
index 4e001cc69..9827e993d 100644
--- a/include/openvswitch/hmap.h
+++ b/include/openvswitch/hmap.h
@@ -134,17 +134,19 @@ struct hmap_node *hmap_random_node(const struct hmap *);
  * without using 'break', NODE will be NULL.  This is true for all of the
  * HMAP_FOR_EACH_*() macros.
  */
-#define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP)   \
-for (INIT_CONTAINER(NODE, hmap_first_with_hash(HMAP, HASH), MEMBER); \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))\
- || ((NODE = NULL), false); \
- ASSIGN_CONTAINER(NODE, hmap_next_with_hash(&(NODE)->MEMBER),   \
-  MEMBER))
-#define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP)   \
-for (INIT_CONTAINER(NODE, hmap_first_in_bucket(HMAP, HASH), MEMBER); \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))\
- || ((NODE = NULL), false); \
- ASSIGN_CONTAINER(NODE, hmap_next_in_bucket(&(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_WITH_HASH(NODE, MEMBER, HASH, HMAP) \
+for (INIT_MULTIVAR(NODE, MEMBER, hmap_first_with_hash(HMAP, HASH),\
+   struct hmap_node); \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);\
+ UPDATE_MULTIVAR(NODE,\
+ ITER_VAR(NODE) = hmap_next_with_hash(ITER_VAR(NODE
+
+#define HMAP_FOR_EACH_IN_BUCKET(NODE, MEMBER, HASH, HMAP) \
+for (INIT_MULTIVAR(NODE, MEMBER, hmap_first_in_bucket(HMAP, HASH),\
+   struct hmap_node); \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);\
+ UPDATE_MULTIVAR(NODE,\
+ ITER_VAR(NODE) = hmap_next_in_bucket(ITER_VAR(NODE
 
 static inline struct hmap_node *hmap_first_with_hash(const struct hmap *,
  size_t hash);
@@ -170,33 +172,36 @@ bool hmap_contains(const struct hmap *, const struct 
hmap_node *);
 /* Iterates through every node in HMAP. */
 #define HMAP_FOR_EACH(NODE, MEMBER, HMAP) \
 HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, (void) 0)
-#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...) \
-for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
- (NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))\
- || ((NODE = NULL), false); \
- ASSIGN_CONTAINER(NODE, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER))
+#define HMAP_FOR_EACH_INIT(NODE, MEMBER, HMAP, ...)   \
+for (INIT_MULTIVAR_EXP(NODE, MEMBER, hmap_first(HMAP), struct hmap_node,  \
+   __VA_ARGS__);  \
+ CONDITION_MULTIVAR(NODE, MEMBER, ITER_VAR(NODE) != NULL);\
+ UPDATE_MULTIVAR(NODE,\
+ ITER_VAR(NODE) = hmap_next(HMAP, ITER_VAR(NODE   \
 
 /* Safe when NODE may be freed (not needed when NODE may be removed from the
  * hash map but its members remain accessible and intact). */
 #define HMAP_FOR_EACH_SAFE(NODE, NEXT, MEMBER, HMAP) \
-HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, (void) 0)
-#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)  \
-for (INIT_CONTAINER(NODE, hmap_first(HMAP), MEMBER), __VA_ARGS__;   \
- ((NODE != OBJECT_CONTAINING(NULL, NODE, MEMBER))   \
-  || ((NODE = NULL), false) \
-  ? INIT_CONTAINER(NEXT, hmap_next(HMAP, &(NODE)->MEMBER), MEMBER), 1 \
-  : 0); \
- (NODE) = (NEXT))
+HMAP_FOR_EACH_SAFE_INIT (NODE, NEXT, MEMBER, HMAP, (void) NEXT)
+
+#define HMAP_FOR_EACH_SAFE_INIT(NODE, NEXT, MEMBER, HMAP, ...)\
+for (INIT_MULTIVAR_SAFE_LONG_EXP(NODE, NEXT, MEMBER, hmap_first(HMAP),\
+  struct hmap_node, __VA_ARGS__); \
+ CONDITION_MULTIVAR_SAFE_LONG(NODE, NEXT, MEMBER, \
+  ITER_VAR(NODE) != NULL, \
+  

[ovs-dev] [PATCH v3 05/17] list: ensure iterator is NULL after pop loop

[Adrian Moreno]

After the loop ends, the iterator is not guaranteed to point to a valid
object and should not be used. Make it NULL to avoid undefined behavior.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/list.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h
index 8673bab93..8cd76b892 100644
--- a/include/openvswitch/list.h
+++ b/include/openvswitch/list.h
@@ -112,8 +112,9 @@ static inline bool ovs_list_is_short(const struct ovs_list 
*);
  UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT))
 
 #define LIST_FOR_EACH_POP(ITER, MEMBER, LIST) \
-while (!ovs_list_is_empty(LIST)   \
-   && (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1))
+while (!ovs_list_is_empty(LIST) ? \
+   (INIT_CONTAINER(ITER, ovs_list_pop_front(LIST), MEMBER), 1) :  \
+   (ITER = NULL, 0))
 
 /* Inline implementations. */
 
-- 
2.34.1

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

[ovs-dev] [PATCH v3 04/17] list: use multi-variable helpers for list loops

[Adrian Moreno]

Use multi-variable iteration helpers to rewrite non-safe loops.

There is an important behavior change compared with the previous
implementation: When the loop ends normally (i.e: not via "break;"), the
object pointer provided by the user is NULL. This is safer because it's
not guaranteed that it would end up pointing a valid address.

Clang-analyzer has successfully picked the potential null-pointer
dereference on the code that triggered this change (bond.c) and nothing
else has been detected.

For _SAFE loops, use the LONG version for backwards compatibility.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/list.h | 71 ++
 ofproto/bond.c |  2 +-
 tests/test-list.c  | 15 ++--
 3 files changed, 54 insertions(+), 34 deletions(-)

diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h
index 8ad5eeb32..8673bab93 100644
--- a/include/openvswitch/list.h
+++ b/include/openvswitch/list.h
@@ -72,36 +72,47 @@ static inline bool ovs_list_is_empty(const struct ovs_list 
*);
 static inline bool ovs_list_is_singleton(const struct ovs_list *);
 static inline bool ovs_list_is_short(const struct ovs_list *);
 
-#define LIST_FOR_EACH(ITER, MEMBER, LIST)   \
-for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);\
- &(ITER)->MEMBER != (LIST); \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_CONTINUE(ITER, MEMBER, LIST)  \
-for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER); \
- &(ITER)->MEMBER != (LIST); \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.next, MEMBER))
-#define LIST_FOR_EACH_REVERSE(ITER, MEMBER, LIST)   \
-for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);\
- &(ITER)->MEMBER != (LIST); \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_REVERSE_SAFE(ITER, PREV, MEMBER, LIST)\
-for (INIT_CONTAINER(ITER, (LIST)->prev, MEMBER);\
- (&(ITER)->MEMBER != (LIST) \
-  ? INIT_CONTAINER(PREV, (ITER)->MEMBER.prev, MEMBER), 1\
-  : 0); \
- (ITER) = (PREV))
-#define LIST_FOR_EACH_REVERSE_CONTINUE(ITER, MEMBER, LIST)  \
-for (ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER);   \
- &(ITER)->MEMBER != (LIST); \
- ASSIGN_CONTAINER(ITER, (ITER)->MEMBER.prev, MEMBER))
-#define LIST_FOR_EACH_SAFE(ITER, NEXT, MEMBER, LIST)   \
-for (INIT_CONTAINER(ITER, (LIST)->next, MEMBER);   \
- (&(ITER)->MEMBER != (LIST)\
-  ? INIT_CONTAINER(NEXT, (ITER)->MEMBER.next, MEMBER), 1   \
-  : 0);\
- (ITER) = (NEXT))
-#define LIST_FOR_EACH_POP(ITER, MEMBER, LIST)  \
-while (!ovs_list_is_empty(LIST)\
+#define LIST_FOR_EACH(VAR, MEMBER, LIST)  \
+for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->next, struct ovs_list);   \
+ CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));\
+ UPDATE_MULTIVAR(VAR, (ITER_VAR(VAR) = ITER_VAR(VAR)->next)))
+
+#define LIST_FOR_EACH_CONTINUE(VAR, MEMBER, LIST) \
+for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.next, struct ovs_list);   \
+ CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));\
+ UPDATE_MULTIVAR(VAR, (ITER_VAR(VAR) = ITER_VAR(VAR)->next)))
+
+#define LIST_FOR_EACH_REVERSE(VAR, MEMBER, LIST)  \
+for (INIT_MULTIVAR(VAR, MEMBER, (LIST)->prev, struct ovs_list);   \
+ CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));\
+ UPDATE_MULTIVAR(VAR, (ITER_VAR(VAR) = ITER_VAR(VAR)->prev)))
+
+
+#define LIST_FOR_EACH_REVERSE_CONTINUE(VAR, MEMBER, LIST) \
+for (INIT_MULTIVAR(VAR, MEMBER, VAR->MEMBER.prev, struct ovs_list);   \
+ CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST));\
+ UPDATE_MULTIVAR(VAR, (ITER_VAR(VAR) = ITER_VAR(VAR)->prev)))
+
+#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST)   \
+for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, \
+ struct ovs_list);\
+ CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER,  \
+  ITER_VAR(VAR) != (LIST),\
+  ITER_VAR(PREV) = ITER_VAR(VAR)->prev,  

[ovs-dev] [PATCH v3 03/17] util: add helpers to overload SAFE macro

[Adrian Moreno]

Having both LONG and SHORT versions of the SAFE macros with different
names is not very convenient. Add helpers that facilitate overloading
such macros using a single name.

In order to work around a known issue in MSVC [1], an indirection layer
has to be introduced.

[1]
https://developercommunity.visualstudio.com/t/-va-args-seems-to-be-trated-as-a-single-parameter/460154

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/util.h | 21 +
 1 file changed, 21 insertions(+)

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index b2b4f3051..749c50185 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -270,6 +270,27 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const 
char *, const char *);
 #define UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR)  \
 ((ITER_VAR(VAR) = ITER_VAR(NEXT_VAR)), (VAR) = NULL, (NEXT_VAR) = NULL)
 
+/* Helpers to allow overloading the *_SAFE iterator macros and select either
+ * the LONG or the SHORT version depending on the number of arguments.
+ */
+#define GET_SAFE_MACRO2(_1, _2, NAME, ...) NAME
+#define GET_SAFE_MACRO3(_1, _2, _3, NAME, ...) NAME
+#define GET_SAFE_MACRO4(_1, _2, _3, _4, NAME, ...) NAME
+#define GET_SAFE_MACRO5(_1, _2, _3, _4, _5, NAME, ...) NAME
+#define GET_SAFE_MACRO6(_1, _2, _3, _4, _5, _6, NAME, ...) NAME
+#define GET_SAFE_MACRO(MAX_ARGS) GET_SAFE_MACRO ## MAX_ARGS
+
+/* MSVC treats __VA_ARGS__ as a simple token in argument lists. Introduce
+ * a level of indirection to work around that. */
+#define EXPAND_MACRO(name, args) name args
+
+/* Overload the LONG and the SHORT version of the macros. MAX_ARGS is the
+ * maximum number of arguments (i.e: the number of arguments of the LONG
+ * version). */
+#define OVERLOAD_SAFE_MACRO(LONG, SHORT, MAX_ARGS, ...) \
+EXPAND_MACRO(GET_SAFE_MACRO(MAX_ARGS), \
+ (__VA_ARGS__, LONG, SHORT))(__VA_ARGS__)
+
 /* Returns the number of elements in ARRAY. */
 #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY)
 
-- 
2.34.1

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

[ovs-dev] [PATCH v3 02/17] util: add safe multi-variable iterators

[Adrian Moreno]

Safe version of multi-variable iterator helpers declare an internal
variable to store the next value of the iterator temporarily.

Two versions of the macro are provided, one that still uses the NEXT
variable for backwards compatibility and a shorter version that does not
require the use of an additional variable provided by the user.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/util.h | 81 ++
 1 file changed, 81 insertions(+)

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index 9c09e8aea..b2b4f3051 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -189,6 +189,87 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const 
char *, const char *);
 #define UPDATE_MULTIVAR(VAR, EXPR)\
 ((EXPR), (VAR) = NULL)
 
+
+/* In the safe version of the multi-variable container iteration, the next
+ * value of the iterator is precalculated on the condition expression.
+ * This allows for the iterator to be freed inside the loop.
+ *
+ * Two versions of the macros are provided:
+ *
+ * * In the _SHORT version, the user does not have to provide a variable to
+ * store the next value of the iterator. Instead, a second iterator variable
+ * is declared in the INIT_ macro and its name is determined by
+ * ITER_NEXT_VAR(OBJECT).
+ *
+ * * In the _LONG version, the user provides another variable of the same type
+ * as the iterator object variable to store the next containing object.
+ * We still declare an iterator variable inside the loop but in this case it's
+ * name is derived from the name of the next containing variable.
+ * The value of the next containing object will only be set
+ * (via OBJECT_CONTAINING) if an additional condition is statisfied. This
+ * second condition must ensure it is safe to call OBJECT_CONTAINING on the
+ * next iterator variable.
+ * With respect to the value of the next containing object:
+ *  - Inside of the loop: the variable is either NULL or safe to use.
+ *  - Outside of the loop: the variable is NULL if the loop ends normally.
+ * If the loop ends with a "break;" statement, rules of Inside the loop
+ * apply.
+ */
+#define ITER_NEXT_VAR(NAME) NAME ## __iterator__next__
+
+/* Safe initialization declares both iterators. */
+#define INIT_MULTIVAR_SAFE_SHORT(VAR, MEMBER, POINTER, ITER_TYPE) \
+INIT_MULTIVAR_SAFE_SHORT_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0)
+
+#define INIT_MULTIVAR_SAFE_SHORT_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...)\
+ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER),\
+*ITER_NEXT_VAR(VAR) = NULL
+
+/* Evaluate the condition expression and, if satisfied, update the _next_
+ * iterator with the NEXT_EXPR.
+ * Both EXPR and NEXT_EXPR should only use ITER_VAR(VAR) and
+ * ITER_NEXT_VAR(VAR).
+ */
+#define CONDITION_MULTIVAR_SAFE_SHORT(VAR, MEMBER, EXPR, NEXT_EXPR)   \
+((EXPR) ? \
+ (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)),\
+  (NEXT_EXPR), 1) :   \
+ (((VAR) = NULL), 0))
+
+#define UPDATE_MULTIVAR_SAFE_SHORT(VAR)   \
+UPDATE_MULTIVAR(VAR, ITER_VAR(VAR) = ITER_NEXT_VAR(VAR))
+
+/*_LONG versions of the macros*/
+
+#define INIT_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR, MEMBER, POINTER, ITER_TYPE)\
+INIT_MULTIVAR_SAFE_LONG_EXP(VAR, NEXT_VAR, MEMBER, POINTER, ITER_TYPE,\
+(void) 0) \
+
+#define INIT_MULTIVAR_SAFE_LONG_EXP(VAR, NEXT_VAR, MEMBER, POINTER,   \
+ITER_TYPE, ...)   \
+ITER_TYPE  *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER),   \
+*ITER_VAR(NEXT_VAR) = NULL
+
+/* Evaluate the condition expression and, if satisfied, update the _next_
+ * iterator with the NEXT_EXPR. After, evaluate the NEXT_COND and, if
+ * satisfied, set the value to NEXT_VAR. NEXT_COND must use ITER_VAR(NEXT_VAR).
+ *
+ * Both EXPR and NEXT_EXPR should only use ITER_VAR(VAR) and
+ * ITER_VAR(NEXT_VAR).
+ */
+#define CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT_VAR, MEMBER, EXPR, NEXT_EXPR,  \
+ NEXT_COND)   \
+((EXPR) ? \
+ (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)),\
+  (NEXT_EXPR), ((NEXT_COND) ? \
+   ((NEXT_VAR) =  \
+OBJECT_CONTAINING(ITER_VAR(NEXT_VAR), NEXT_VAR, MEMBER)) :\
+   ((NEXT_VAR) = NULL)), 1) : \
+ (((VAR) = NULL), ((NEXT_VAR) = NULL), 0))
+
+#define 

[ovs-dev] [PATCH v3 00/17] Fix undefined behavior in loop macros

[Adrian Moreno]

When running builds with UBSan, some undefined behavior was detected in the 
iteration of common data data structures in OVS.
Coincidentally, a bug was reported [1] whose root cause was another, this time 
undetected, undefined behavior in the iteration macros.

>From both cases, we conclude that the way we're currently iterating the data 
>structures is prone to errors and UB. This series is an attempt to rewrite 
>those macros in a UB-safe manner.

The core problem is that #define OBJECT_CONTAINING(POINTER, OBJECT, MEMBER) 
macro is being used on invalid POINTER values. In some cases we use NULL to 
compute the end-of-loop condition. In others, we allow it to point to 
non-contained objects (e.g: a non-contained stack allocated "struct ovs_list" 
as in [1]).

In order to systematically solve this in all cases this series introduces a new 
set of macros that implement a multi-variable loop iteration. They declare a 
hidden iterator variable inside the loop, used to iterate and evaluate the loop 
condition and only compute its OBJECT_CONTAINING if it satisfies the loop 
condition. One consequence of this safety guard is that the pointer provided by 
the user is set to NULL after the loop (if not exited via "break;").

Apart from normal iteration, many OVS data structures have a _SAFE version of 
the loop which require the user to declare an extra variable to hold the next 
value of the iterator.
The use of internal iterator variables makes the declaration variable not 
necessary. However, some users might still need that extra variable. For that 
reason, this series introduces two versions of _SAFE iteration loops: 
- The _LONG version keeps using the external variable but avoids calling 
OBJECT_CONTAINING on invalid values. Note that alghough this version still uses 
an external variable, it's behavior changes slightly as the "next" variable 
could be NULL if it's not safe to calculate the OBJECT_CONTAINING of its 
internal iterator value.
- The _SHORT version removes the use of the external variable alltogether.
On relevant data structures, an initial patch rewrites the macros using the 
_LONG, backwards-compatible helpers. A second patch adds the _SHORT version and 
removes the unneeded variable from the callers. In order to be even more 
flexible, the original macro name is overloaded and the right version is 
selected depending on the number of arguments provided by the user. The first 
patch would be easy to backport and the second would make code cleaner for the 
master branch.

* Testing / reviewing notes *
In order to verify this series removes all the loop-related UB, I've tested it 
on top of Dumitru's series [2] (without patch 1/11, which can hide some still 
invalid use of OBJECT_CONTAINING).
I've also verified no extra errors are reported through clang-analyzer or ASan.

There are a number of checkpatch.py errors:
- ERROR: Inappropriate bracing around statement: Seems a limitation in 
checkpatch.py that only happens when a FOR_EACH* macro calls another FOR_EACH* 
macro. I don't think it's worth modifying checkpatch.py for this.
- ERROR: Use ovs_assert() in place of assert(): On tests/*, I'll send an 
independent patch to exclude the "tests" directory from this check.

I have verified the builds pass for MVSC in AppVeyor but have not run any 
further verification.

Note this series changes some potential undefined behavior with some potential 
NULL pointer dereferencing, which should be easier to catch by the static and 
dynamic analyzers.

* Limitations *
The proposed approach benefits code readability, therefore the name of the 
iterator variable is derived from the name of the object pointer given by the 
caller. This means that in an unlikely but still possible case in which a 
caller wants to nest two loops with the same iterating pointer object, the 
inner loop iterator variable will hide the one declared in the outer loop. This 
limitation is easy to spot (the compiler will warn) and easy to work around 
(just declaring another object pointer variable). I found no such code in the 
OvS or OVN tree.

V2 -> V3
- Added ITER_TYPE to MULTIVAR_INIT macro to avoid depending on typeof
  (missing in MVSC).
- Added macro overloading helpers that work around MVSC issues.

V1 -> V2
- Added LONG and SHORT versions of _SAFE macros with macro name overloading.
- Rebased on top of latest master.
- Added Dumitru's patch to fix sparse header inclusion and version 
recommendation.
- Homogenize argument order in helper macros: VAR and MEMBER always first.
- Added SHORT version of _SAFE loops to SSET and idlc.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2014942
[2] https://patchwork.ozlabs.org/project/openvswitch/list/?series=277900


Adrian Moreno (17):
  util: add multi-variable loop iterator macros
  util: add safe multi-variable iterators
  util: add helpers to overload SAFE macro
  list: use multi-variable helpers for list loops
  list: ensure iterator is NULL after pop loop
  list: use short 

[ovs-dev] [PATCH v3 01/17] util: add multi-variable loop iterator macros

[Adrian Moreno]

Multi-variable loop iterators avoid potential undefined behavior by
using an internal iterator variable to perform the iteration and only
referencing the containing object (via OBJECT_CONTAINING) if the
iterator has been validated via the second expression of the for
statement.

That way, the user can easily implement a loop that never tries to
obtain the object containing NULL or stack-allocated non-contained
nodes.

When the loop ends normally (not via "break;") the user-provided
variable is set to NULL.

Signed-off-by: Adrian Moreno 
---
 include/openvswitch/util.h | 44 ++
 1 file changed, 44 insertions(+)

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index 228b185c3..9c09e8aea 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char *, const 
char *, const char *);
 #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \
 ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER))
 
+
+/* Multi-variable container iterators.
+ *
+ * The following macros facilitate safe iteration over data structures
+ * contained in objects. It does so by using an internal iterator variable of
+ * the type of the member object pointer (i.e: pointer to the data structure).
+ */
+
+/* Multi-variable iterator variable name.
+ * Returns the name of the internal iterator variable.
+ */
+#define ITER_VAR(NAME) NAME ## __iterator__
+
+/* Multi-variable initialization. Creates an internal iterator variable that
+ * points to the provided pointer. The type of the iterator variable is
+ * ITER_TYPE*. It must be the same type as >MEMBER.
+ *
+ * The _EXP version evaluates the extra expressions once.
+ */
+#define INIT_MULTIVAR(VAR, MEMBER, POINTER, ITER_TYPE)  \
+INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0)
+
+#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, ...) \
+ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER)
+
+/* Multi-variable condition.
+ * Evaluates the condition expression (that must be based on the internal
+ * iterator variable). Only if the result of expression is true, the OBJECT is
+ * set to the object containing the current value of the iterator variable.
+ *
+ * It is up to the caller to make sure it is safe to run OBJECT_CONTAINING on
+ * the pointers that verify the condition.
+ */
+#define CONDITION_MULTIVAR(VAR, MEMBER, EXPR) \
+((EXPR) ? \
+ (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) :   \
+ (((VAR) = NULL), 0))
+
+/* Multi-variable update.
+ * Evaluates the expresssion that is supposed to update the iterator variable.
+ */
+#define UPDATE_MULTIVAR(VAR, EXPR)\
+((EXPR), (VAR) = NULL)
+
 /* Returns the number of elements in ARRAY. */
 #define ARRAY_SIZE(ARRAY) __ARRAY_SIZE(ARRAY)
 
-- 
2.34.1

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

[ovs-dev] [PATCH net v3 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure

[Paul Blakey via dev]

Ipv6 ttl, label and tos fields are modified without first
pulling/pushing the ipv6 header, which would have updated
the hw csum (if available). This might cause csum validation
when sending the packet to the stack, as can be seen in
the trace below.

Fix this by updating skb->csum if available.

Trace resulted by ipv6 ttl dec and then sending packet
to conntrack [actions: set(ipv6(hlimit=63)),ct(zone=99)]:
[295241.900063] s_pf0vf2: hw csum failure
[295241.923191] Call Trace:
[295241.925728]  
[295241.927836]  dump_stack+0x5c/0x80
[295241.931240]  __skb_checksum_complete+0xac/0xc0
[295241.935778]  nf_conntrack_tcp_packet+0x398/0xba0 [nf_conntrack]
[295241.953030]  nf_conntrack_in+0x498/0x5e0 [nf_conntrack]
[295241.958344]  __ovs_ct_lookup+0xac/0x860 [openvswitch]
[295241.968532]  ovs_ct_execute+0x4a7/0x7c0 [openvswitch]
[295241.979167]  do_execute_actions+0x54a/0xaa0 [openvswitch]
[295242.001482]  ovs_execute_actions+0x48/0x100 [openvswitch]
[295242.006966]  ovs_dp_process_packet+0x96/0x1d0 [openvswitch]
[295242.012626]  ovs_vport_receive+0x6c/0xc0 [openvswitch]
[295242.028763]  netdev_frame_hook+0xc0/0x180 [openvswitch]
[295242.034074]  __netif_receive_skb_core+0x2ca/0xcb0
[295242.047498]  netif_receive_skb_internal+0x3e/0xc0
[295242.052291]  napi_gro_receive+0xba/0xe0
[295242.056231]  mlx5e_handle_rx_cqe_mpwrq_rep+0x12b/0x250 [mlx5_core]
[295242.062513]  mlx5e_poll_rx_cq+0xa0f/0xa30 [mlx5_core]
[295242.067669]  mlx5e_napi_poll+0xe1/0x6b0 [mlx5_core]
[295242.077958]  net_rx_action+0x149/0x3b0
[295242.086762]  __do_softirq+0xd7/0x2d6
[295242.090427]  irq_exit+0xf7/0x100
[295242.093748]  do_IRQ+0x7f/0xd0
[295242.096806]  common_interrupt+0xf/0xf
[295242.100559]  
[295242.102750] RIP: 0033:0x7f9022e88cbd
[295242.125246] RSP: 002b:7f9022282b20 EFLAGS: 0246 ORIG_RAX: 
ffda
[295242.132900] RAX: 0005 RBX: 0010 RCX: 

[295242.140120] RDX: 7f9022282ba8 RSI: 7f9022282a30 RDI: 
7f9014005c30
[295242.147337] RBP: 7f9014014d60 R08: 0020 R09: 
7f90254a8340
[295242.154557] R10: 7f9022282a28 R11: 0246 R12: 

[295242.161775] R13: 7f902308c000 R14: 002b R15: 
7f9022b71f40

Fixes: 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action")
Signed-off-by: Paul Blakey 
---
 Changelog:
v2->v3:
  Use u8 instead of __u8
  Fix sparse warnings on conversions
v1->v2:
  Replaced push pull rcsum checksum calc with actual checksum calc

 net/openvswitch/actions.c | 51 +--
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 076774034bb9..02c63308433a 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -423,12 +423,48 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 
l4_proto,
memcpy(addr, new_addr, sizeof(__be32[4]));
 }
 
-static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask)
+static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, u8 
ipv6_tclass, u8 mask)
 {
+   u8 old_ipv6_tclass = ipv6_get_dsfield(nh);
+
+   ipv6_tclass = OVS_MASKED(old_ipv6_tclass, ipv6_tclass, mask);
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum =
+   ~csum_block_add(csum_block_sub(~skb->csum,
+  (__force __wsum) 
(ipv6_tclass << 4), 1),
+   (__force __wsum) (old_ipv6_tclass << 
4), 1);
+
+   ipv6_change_dsfield(nh, ~mask, ipv6_tclass);
+}
+
+static void set_ipv6_fl(struct sk_buff *skb, struct ipv6hdr *nh, u32 fl, u32 
mask)
+{
+   u32 old_fl;
+
+   old_fl = nh->flow_lbl[0] << 16 |  nh->flow_lbl[1] << 8 |  
nh->flow_lbl[2];
+   fl = OVS_MASKED(old_fl, fl, mask);
+
/* Bits 21-24 are always unmasked, so this retains their values. */
-   OVS_SET_MASKED(nh->flow_lbl[0], (u8)(fl >> 16), (u8)(mask >> 16));
-   OVS_SET_MASKED(nh->flow_lbl[1], (u8)(fl >> 8), (u8)(mask >> 8));
-   OVS_SET_MASKED(nh->flow_lbl[2], (u8)fl, (u8)mask);
+   nh->flow_lbl[0] = (u8)(fl >> 16);
+   nh->flow_lbl[1] = (u8)(fl >> 8);
+   nh->flow_lbl[2] = (u8)fl;
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = ~csum_block_add(csum_block_sub(~skb->csum,
+  (__force __wsum) 
htonl(fl), 0),
+   (__force __wsum) htonl(old_fl), 0);
+}
+
+static void set_ipv6_ttl(struct sk_buff *skb, struct ipv6hdr *nh, u8 new_ttl, 
u8 mask)
+{
+   new_ttl = OVS_MASKED(nh->hop_limit, new_ttl, mask);
+
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = ~csum_block_add(csum_block_sub(~skb->csum, (__force 
__wsum) new_ttl,
+  1),
+   (__force __wsum) nh->hop_limit, 1);
+

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

[wangyunjian via dev]

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

> 
> It is 'nice-to-have', maybe. In essence, it is about making sure the ofproto
> reference cannot be held by another thread while proceeding with the
> destruction.
> It simplifies the execution pattern for such destruction.
> 
> But it seems other systems relying on ofproto access were written with the
> possibility that it is currently in the process of being destroyed. Its 
> freeing is
> deferred, rules and groups are meant to proceed within this grace period.
> Granted, there is currently a bug in the rule management, but this is a bug 
> and it
> is being fixed.
> 
> So while it is correct, and while it simplifies the mental model when looking 
> at the
> lifetime of ofproto, it does not seem necessary? Am I mistaken?
> 
> If it is necessary, it would be better to be explicit about it. If multiple 
> levels of the
> object rely on RCU sync, a single overarching call with a proper comment would
> be safer to maintain. As it concerns destruct() safety, I think it is worth 
> having it
> explicitly at that level. It makes the fix more complex and more dangerous 
> with
> changes in xlate implementation however.

I will add "all_ofproto_dpifs_by_uuid_node" to "struct xlate_cfg" to avoid this 
issue
according to Adrian Moreno's suggestion.

Thanks,
Yunjian

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

Re: [ovs-dev] [PATCH v1 0/3] Fix offload rule flush race condition

[Ilya Maximets]

On 2/4/22 17:12, Gaetan Rivet wrote:
> A race condition has been identified during datapath destruction,
> along with the port offload flushes issued.
> 
> This series addresses these race conditions, cleaning up the
> port deletion process.
> 
> The last patch also cleans up the offload structure.
> It is not strictly necessary like the first two fixes,
> so I put it last. It can wait until after the code-freeze
> to be integrated.
> 
> I tested for a few hours without ASAN enabled without seeing
> issues. ASAN has been executed as part of the github CI:
> https://github.com/grivet/ovs/actions/runs/1795624401
> It is however not too relevant, as no offloads are inserted during CI.
> 
> The following patch was used to fix an unrelated CI issue:
> https://patchwork.ozlabs.org/project/openvswitch/patch/20220204150445.1481457-1-i.maxim...@ovn.org/
> 
> I also ran datapath creation + deletion loop with ASAN on an offload
> test setup, but the execution was excruciatingly slow and could
> not progress much. It reached datapath deletion without panicking
> and no crash was seen, even though I had to interrupt the test after
> a few hours.
> 
> Gaetan Rivet (2):
>   dpif-netdev: Move port flush after datapath reconfiguration
>   dpif-netdev: Use dp_netdev reference in offload threads
> 
> Sriharsha Basavapatna (1):
>   dpif-netdev: Fix a race condition in deletion of offloaded flows
> 
>  lib/dpif-netdev.c | 71 ++-
>  1 file changed, 40 insertions(+), 31 deletions(-)


Thanks!  I applied the series to master and 2.17.

I was thinking about backports and I think that it should be OK to
backport patch #2 (always enqueue deletions), because it doesn't
introduce any new issues.

But I'm not sure about the first patch.  On 2.17 flush acts like
a barrier, so no offload requests are in the offload queue after
the port removal from PMD thread and flush from the offload thread.
However, there is no such synchronization mechanism on 2.16.
Flush is performed from the main thread and some offload requests
could still be in the offload queue.  And if dp is destroyed,
processing of those offload requests will cause use-after-free.
In other words, while patch #1 can reduce the race window for flows
remaining in the hardware after the port deletion, it doesn't solve
the use-after-free problem.  So, we need some other solution.
Backporting the barrier machinery doesn't seem like a good option.

Referencing the dp on creation of the PMD thread or on creation
of the offload item might be a solution. But this will change the
time dp will actually be destroyed, so needs a careful consideration.

We may also not fix the UAF problem on 2.16 taking into account that
issue is happening only during deletion of the datapath, so should not
be very severe and 2.16 is not an LTS branch.

Thoughts?

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

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

[Adrian Moreno]

On 2/15/22 11:19, Peng He wrote:



Adrian Moreno mailto:amore...@redhat.com>> 于2022年2月15日 
周二 17:19写道：


Hi Peng,

On 2/13/22 03:14, Peng He wrote:
 >  From hepeng:
 >

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


 >
 > also from guohongzhi mailto:guohongz...@huawei.com>>:
 >

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
 >
 > Signed-off-by: Peng He mailto:hepeng.0...@bytedance.com>>
 > Signed-off-by: guohongzhi mailto:guohongz...@huawei.com>>
 > Acked-by: Mike Pattrick mailto:m...@redhat.com>>
 > ---
 >   ofproto/ofproto-dpif-xlate-cache.c |  2 +
 >   ofproto/ofproto-dpif-xlate.c       | 14 ---
 >   ofproto/ofproto-dpif.c             | 24 ++-
 >   ofproto/ofproto-provider.h         |  2 +
 >   ofproto/ofproto.c                  | 65 +++---
 >   ofproto/ofproto.h                  |  4 ++
 >   6 files changed, 90 insertions(+), 21 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 

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

[Gaëtan Rivet]

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

It is 'nice-to-have', maybe. In essence, it is about making sure the ofproto
reference cannot be held by another thread while proceeding with the 
destruction.
It simplifies the execution pattern for such destruction.

But it seems other systems relying on ofproto access were written with the
possibility that it is currently in the process of being destroyed. Its freeing
is deferred, rules and groups are meant to proceed within this grace period.
Granted, there is currently a bug in the rule management, but this is a bug
and it is being fixed.

So while it is correct, and while it simplifies the mental model when looking
at the lifetime of ofproto, it does not seem necessary? Am I mistaken?

If it is necessary, it would be better to be explicit about it. If multiple 
levels
of the object rely on RCU sync, a single overarching call with a proper comment
would be safer to maintain. As it concerns destruct() safety, I think it is
worth having it explicitly at that level. It makes the fix more complex and 
more dangerous
with changes in xlate implementation however.

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

Re: [ovs-dev] [PATCH v2 07/18] python: introduce OpenFlow Flow parsing

[Eelco Chaudron]

On 16 Feb 2022, at 12:19, Adrian Moreno wrote:

> On 2/11/22 15:12, Eelco Chaudron wrote:
>>> +def decode_nat(value):
>>> +"""Decodes the 'nat' keyword of the ct action"""
>> You were going to add an example of what the decode would look like?
>>
>>> +if not value:
>>> +return True
>> You where going to add a comment here why return true:
>>
>> “””
>>
>>> Why returning True is no data is present? I would expect None.
>> In general keys without values are decoded as key: True. Other places in the 
>> code calls them "flags", e.g: "drop" action is decoded as {"drop": True}. In 
>> this case "ct" without a value is a flag.
>>
>> But I can see how this can be confusing. I'll add a comment to clarify.
>>
>> “””
>>
>
> Sorry I missed it, will send another version.

Don’t hurry too much, will try to finish the v2 review next week, as I’m on 
training this week.

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

Re: [ovs-dev] [PATCH] Documentation: Update USDT documentation to include systemtap dependency

[Adrian Moreno]

On 2/7/22 12:32, Eelco Chaudron wrote:

Update the documentation to include details on SystemTap dependency
when enabling USDT probes.

Suggested-by: Adrian Moreno 
Signed-off-by: Eelco Chaudron 
---
  Documentation/topics/usdt-probes.rst |5 +
  1 file changed, 5 insertions(+)

diff --git a/Documentation/topics/usdt-probes.rst 
b/Documentation/topics/usdt-probes.rst
index cdded4f90..7ce19aaed 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -50,6 +50,11 @@ is used ::
  
  checking whether USDT probes are enabled... yes
  
+As USDT probes internally use the ``DTRACE_PROBExx`` macros, which are part of

+the SystemTap framework, you need to install the appropriate package for your
+Linux distribution. For example, on Fedora, you need to install the
+``systemtap-sdt-devel`` package.
+
  
  Listing available probes

  




Thanks Eelco.

Acked-by: Adrián Moreno 

--
Adrián Moreno

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

Re: [ovs-dev] [PATCH v2 07/18] python: introduce OpenFlow Flow parsing

[Adrian Moreno]

On 2/11/22 15:12, Eelco Chaudron wrote:

+def decode_nat(value):
+"""Decodes the 'nat' keyword of the ct action"""

You were going to add an example of what the decode would look like?


+if not value:
+return True

You where going to add a comment here why return true:

“””


Why returning True is no data is present? I would expect None.

In general keys without values are decoded as key: True. Other places in the code calls them "flags", e.g: 
"drop" action is decoded as {"drop": True}. In this case "ct" without a value is a flag.

But I can see how this can be confusing. I'll add a comment to clarify.

“””



Sorry I missed it, will send another version.

--
Adrián Moreno

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

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

[Gaëtan Rivet]

On Sun, Feb 13, 2022, at 03:14, Peng He wrote:
> 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
>
> Signed-off-by: Peng He 
> Signed-off-by: guohongzhi 
> Acked-by: Mike Pattrick 

Hi Peng,

Thanks for fixing the comments I had.
I still think this commit log might be too long,
but the important infos are there to understand the issue.

That being said, everything is understandable and not misleading I think,
and the fix seems correct to me. I don't see any more issues.

Acked-by: Gaetan Rivet 

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

Re: [ovs-dev] [PATCH v2] tc: Keep header rewrite actions order

[Eelco Chaudron]

On 16 Feb 2022, at 10:48, Roi Dayan wrote:

> On 2022-02-16 10:37 AM, Eelco Chaudron wrote:
>>
>>
>> On 15 Feb 2022, at 10:52, Roi Dayan wrote:
>>
>>> From: Chris Mi 
>>>
>>> Currently, tc merges all header rewrite actions into one tc pedit
>>> action. So the header rewrite actions order is lost. Save each header
>>> rewrite action into one tc pedit action to keep the order. And only
>>> append one tc csum action to the last pedit action of a series.
>>
>> I’m on training all of this week, so will try to look at this later.
>> But in the meantime, do you think it’s worth adding a test case for this?
>>
>> //Eelco
>
> maybe. but the fix is already ready for some time while it looks like we
> could add many potential cases related to tc.
> We should and will find the time later to look at adding more unit tests
> into the repo.
> thanks

The reason for asking for a test case is that when reviewing TC-related patches 
I no longer just do a visual review, I test them. This is because a lot of 
times a small change results in the introduction of problems, for example with 
the dpctl/dump-flows output. So without any test cases, I need to reverse 
engineer how to replicate the problem, and then test it.

So my request would be if you are not supplying a test case, at least supply a 
simple replicator in the commit description.

>>> Signed-off-by: Chris Mi 
>>> Reviewed-by: Roi Dayan 
>>> ---
>>>
>>> v2:
>>> - rebased to fix conflict.
>>>
>>>   lib/netdev-offload-tc.c | 22 
>>>   lib/tc.c| 54 
>>> +
>>>   lib/tc.h| 25 +++
>>>   3 files changed, 57 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index 9845e8d3feae..ab2a678c6923 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -481,10 +481,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump 
>>> *dump)
>>>
>>>   static void
>>>   parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
>>> -   struct tc_flower *flower)
>>> +   struct tc_action *action)
>>>   {
>>> -char *mask = (char *) >rewrite.mask;
>>> -char *data = (char *) >rewrite.key;
>>> +char *mask = (char *) >rewrite.mask;
>>> +char *data = (char *) >rewrite.key;
>>>
>>>   for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
>>>   char *put = NULL;
>>> @@ -877,7 +877,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>>>   }
>>>   break;
>>>   case TC_ACT_PEDIT: {
>>> -parse_flower_rewrite_to_netlink_action(buf, flower);
>>> +parse_flower_rewrite_to_netlink_action(buf, action);
>>>   }
>>>   break;
>>>   case TC_ACT_ENCAP: {
>>> @@ -1222,8 +1222,8 @@ parse_put_flow_set_masked_action(struct tc_flower 
>>> *flower,
>>>   uint64_t set_stub[1024 / 8];
>>>   struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
>>>   char *set_data, *set_mask;
>>> -char *key = (char *) >rewrite.key;
>>> -char *mask = (char *) >rewrite.mask;
>>> +char *key = (char *) >rewrite.key;
>>> +char *mask = (char *) >rewrite.mask;
>>>   const struct nlattr *attr;
>>>   int i, j, type;
>>>   size_t size;
>>> @@ -1265,14 +1265,6 @@ parse_put_flow_set_masked_action(struct tc_flower 
>>> *flower,
>>>   }
>>>   }
>>>
>>> -if (!is_all_zeros(>rewrite, sizeof flower->rewrite)) {
>>> -if (flower->rewrite.rewrite == false) {
>>> -flower->rewrite.rewrite = true;
>>> -action->type = TC_ACT_PEDIT;
>>> -flower->action_count++;
>>> -}
>>> -}
>>> -
>>>   if (hasmask && !is_all_zeros(set_mask, size)) {
>>>   VLOG_DBG_RL(, "unsupported sub attribute of set action type 
>>> %d",
>>>   type);
>>> @@ -1281,6 +1273,8 @@ parse_put_flow_set_masked_action(struct tc_flower 
>>> *flower,
>>>   }
>>>
>>>   ofpbuf_uninit(_buf);
>>> +action->type = TC_ACT_PEDIT;
>>> +flower->action_count++;
>>>   return 0;
>>>   }
>>>
>>> diff --git a/lib/tc.c b/lib/tc.c
>>> index adb2d3182ad4..b637f4c17ed9 100644
>>> --- a/lib/tc.c
>>> +++ b/lib/tc.c
>>> @@ -1006,14 +1006,14 @@ static const struct nl_policy pedit_policy[] = {
>>>   static int
>>>   nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
>>>   {
>>> -struct tc_action *action;
>>> +struct tc_action *action = >actions[flower->action_count++];
>>>   struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
>>>   const struct tc_pedit *pe;
>>>   const struct tc_pedit_key *keys;
>>>   const struct nlattr *nla, *keys_ex, *ex_type;
>>>   const void *keys_attr;
>>> -char *rewrite_key = (void *) >rewrite.key;
>>> -char *rewrite_mask = (void *) >rewrite.mask;
>>> +char *rewrite_key 

Re: [ovs-dev] [PATCH ovn v2] Copy external_ids from Logical_Router_Port to SB database

[Selvaraj Palaniyappan]

Hello Mark,

A gentle reminder to review the description and the patch.

Kind Regards,
Selva

On 08-Feb-2022, at 10:48 PM, Selvaraj Palaniyappan 
mailto:selvara...@nutanix.com>> wrote:

Hello Mark,

Thanks for reviewing the diff. Please find the background info related to this 
patch.

"The ML2 Plugin can add some useful info to NB database of Logical router 
port(LRP) table's external-ids that can be propagated to SB Port_binding table. 
Some module on compute node can consume these external-ids from Port_binding 
entries. The useful info could be subnet type on LRP and other data.”

Regarding subject line, I have used "git send-mail” with format patch o/p to 
upload the patch. Complete description has been taken from format patch o/p. If 
needed, let me send the new patch. Let me know your input.

Thanks,
Selva

On 05-Feb-2022, at 12:38 AM, Mark Michelson 
mailto:mmich...@redhat.com>> wrote:

Hello,

I had a look, and while the code does what it claims, it's not clear why you 
want to do this. Can you provide some background?

Also, it appears in this version of the patch, you put the entire description 
in the subject line :)

Thanks

On 1/27/22 07:50, Selvaraj Palaniyappan wrote:
Signed-off-by: Selvaraj Palaniyappan 
mailto:selvara...@nutanix.com>>
---
 northd/northd.c |  1 +
 ovn-nb.xml  |  6 ++
 ovn-sb.xml  |  3 ++-
 tests/ovn-northd.at | 14 ++
 4 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/northd/northd.c b/northd/northd.c
index fc7a64f99..090922ae2 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3240,6 +3240,7 @@ ovn_port_update_sbrec(struct northd_input *input_data,
 ds_destroy();
   struct smap ids = SMAP_INITIALIZER();
+smap_clone(, >nbrp->external_ids);
 sbrec_port_binding_set_external_ids(op->sb, );
   sbrec_port_binding_set_nat_addresses(op->sb, NULL, 0);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 6a6972856..293d25b32 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2895,6 +2895,12 @@
 
   
 See External IDs at the beginning of this document.
+
+  The ovn-northd program copies all these pairs into the
+   column of the
+   table in 
+  database.
+
   
 
   
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 9ddacdf09..f7c41ccdc 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3354,7 +3354,8 @@ tcp.flags = RST;
 
   The ovn-northd program populates this column with
   all entries into the  column of the
-   table of the
+   and
+   tables of the
database.
 
   
diff --git a/tests/ovn-northd.at 
b/tests/ovn-northd.at
index 84e52e701..f9c5259f1 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -144,6 +144,20 @@ AT_CHECK([test x`ovn-nbctl lsp-get-up S1-R1` = xup])
 AT_CLEANUP
 ])
 +OVN_FOR_EACH_NORTHD([
+AT_SETUP([check external id propagation to SBDB])
+ovn_start
+
+ovn-nbctl lr-add ro
+ovn-nbctl lrp-add ro lrp0 00:00:00:00:00:01 192.168.1.1/24
+ovn-nbctl --wait=sb set logical_router_port lrp0 external_ids=test=123
+AT_CHECK([ovn-sbctl --columns=external_ids --bare find Port_Binding 
logical_port=lrp0],
+[0], [test=123
+])
+
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([check IPv6 RA config propagation to SBDB])
 ovn_start



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

Re: [ovs-dev] [PATCH v2] tc: Keep header rewrite actions order

[Roi Dayan via dev]

On 2022-02-16 10:37 AM, Eelco Chaudron wrote:



On 15 Feb 2022, at 10:52, Roi Dayan wrote:


From: Chris Mi 

Currently, tc merges all header rewrite actions into one tc pedit
action. So the header rewrite actions order is lost. Save each header
rewrite action into one tc pedit action to keep the order. And only
append one tc csum action to the last pedit action of a series.


I’m on training all of this week, so will try to look at this later.
But in the meantime, do you think it’s worth adding a test case for this?

//Eelco


maybe. but the fix is already ready for some time while it looks like we
could add many potential cases related to tc.
We should and will find the time later to look at adding more unit tests
into the repo.
thanks





Signed-off-by: Chris Mi 
Reviewed-by: Roi Dayan 
---

v2:
- rebased to fix conflict.

  lib/netdev-offload-tc.c | 22 
  lib/tc.c| 54 +
  lib/tc.h| 25 +++
  3 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 9845e8d3feae..ab2a678c6923 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -481,10 +481,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)

  static void
  parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
-   struct tc_flower *flower)
+   struct tc_action *action)
  {
-char *mask = (char *) >rewrite.mask;
-char *data = (char *) >rewrite.key;
+char *mask = (char *) >rewrite.mask;
+char *data = (char *) >rewrite.key;

  for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
  char *put = NULL;
@@ -877,7 +877,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
  }
  break;
  case TC_ACT_PEDIT: {
-parse_flower_rewrite_to_netlink_action(buf, flower);
+parse_flower_rewrite_to_netlink_action(buf, action);
  }
  break;
  case TC_ACT_ENCAP: {
@@ -1222,8 +1222,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
  uint64_t set_stub[1024 / 8];
  struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
  char *set_data, *set_mask;
-char *key = (char *) >rewrite.key;
-char *mask = (char *) >rewrite.mask;
+char *key = (char *) >rewrite.key;
+char *mask = (char *) >rewrite.mask;
  const struct nlattr *attr;
  int i, j, type;
  size_t size;
@@ -1265,14 +1265,6 @@ parse_put_flow_set_masked_action(struct tc_flower 
*flower,
  }
  }

-if (!is_all_zeros(>rewrite, sizeof flower->rewrite)) {
-if (flower->rewrite.rewrite == false) {
-flower->rewrite.rewrite = true;
-action->type = TC_ACT_PEDIT;
-flower->action_count++;
-}
-}
-
  if (hasmask && !is_all_zeros(set_mask, size)) {
  VLOG_DBG_RL(, "unsupported sub attribute of set action type %d",
  type);
@@ -1281,6 +1273,8 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
  }

  ofpbuf_uninit(_buf);
+action->type = TC_ACT_PEDIT;
+flower->action_count++;
  return 0;
  }

diff --git a/lib/tc.c b/lib/tc.c
index adb2d3182ad4..b637f4c17ed9 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1006,14 +1006,14 @@ static const struct nl_policy pedit_policy[] = {
  static int
  nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
  {
-struct tc_action *action;
+struct tc_action *action = >actions[flower->action_count++];
  struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
  const struct tc_pedit *pe;
  const struct tc_pedit_key *keys;
  const struct nlattr *nla, *keys_ex, *ex_type;
  const void *keys_attr;
-char *rewrite_key = (void *) >rewrite.key;
-char *rewrite_mask = (void *) >rewrite.mask;
+char *rewrite_key = (void *) >rewrite.key;
+char *rewrite_mask = (void *) >rewrite.mask;
  size_t keys_ex_size, left;
  int type, i = 0, err;

@@ -1092,7 +1092,6 @@ nl_parse_act_pedit(struct nlattr *options, struct 
tc_flower *flower)
  i++;
  }

-action = >actions[flower->action_count++];
  action->type = TC_ACT_PEDIT;

  return 0;
@@ -2399,14 +2398,14 @@ nl_msg_put_act_flags(struct ofpbuf *request) {
   * first_word_mask/last_word_mask - the mask to use for the first/last read
   * (as we read entire words). */
  static void
-calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m,
+calc_offsets(struct tc_action *action, struct flower_key_to_pedit *m,
   int *cur_offset, int *cnt, ovs_be32 *last_word_mask,
   ovs_be32 *first_word_mask, ovs_be32 **mask, ovs_be32 **data)
  {
  int start_offset, max_offset, total_size;
  int diff, right_zero_bits, left_zero_bits;
-char *rewrite_key = (void *) 

Re: [ovs-dev] [PATCH v2] tc: Keep header rewrite actions order

[Eelco Chaudron]

On 15 Feb 2022, at 10:52, Roi Dayan wrote:

> From: Chris Mi 
>
> Currently, tc merges all header rewrite actions into one tc pedit
> action. So the header rewrite actions order is lost. Save each header
> rewrite action into one tc pedit action to keep the order. And only
> append one tc csum action to the last pedit action of a series.

I’m on training all of this week, so will try to look at this later.
But in the meantime, do you think it’s worth adding a test case for this?

//Eelco

> Signed-off-by: Chris Mi 
> Reviewed-by: Roi Dayan 
> ---
>
> v2:
> - rebased to fix conflict.
>
>  lib/netdev-offload-tc.c | 22 
>  lib/tc.c| 54 
> +
>  lib/tc.h| 25 +++
>  3 files changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 9845e8d3feae..ab2a678c6923 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -481,10 +481,10 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump 
> *dump)
>
>  static void
>  parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
> -   struct tc_flower *flower)
> +   struct tc_action *action)
>  {
> -char *mask = (char *) >rewrite.mask;
> -char *data = (char *) >rewrite.key;
> +char *mask = (char *) >rewrite.mask;
> +char *data = (char *) >rewrite.key;
>
>  for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
>  char *put = NULL;
> @@ -877,7 +877,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>  }
>  break;
>  case TC_ACT_PEDIT: {
> -parse_flower_rewrite_to_netlink_action(buf, flower);
> +parse_flower_rewrite_to_netlink_action(buf, action);
>  }
>  break;
>  case TC_ACT_ENCAP: {
> @@ -1222,8 +1222,8 @@ parse_put_flow_set_masked_action(struct tc_flower 
> *flower,
>  uint64_t set_stub[1024 / 8];
>  struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
>  char *set_data, *set_mask;
> -char *key = (char *) >rewrite.key;
> -char *mask = (char *) >rewrite.mask;
> +char *key = (char *) >rewrite.key;
> +char *mask = (char *) >rewrite.mask;
>  const struct nlattr *attr;
>  int i, j, type;
>  size_t size;
> @@ -1265,14 +1265,6 @@ parse_put_flow_set_masked_action(struct tc_flower 
> *flower,
>  }
>  }
>
> -if (!is_all_zeros(>rewrite, sizeof flower->rewrite)) {
> -if (flower->rewrite.rewrite == false) {
> -flower->rewrite.rewrite = true;
> -action->type = TC_ACT_PEDIT;
> -flower->action_count++;
> -}
> -}
> -
>  if (hasmask && !is_all_zeros(set_mask, size)) {
>  VLOG_DBG_RL(, "unsupported sub attribute of set action type %d",
>  type);
> @@ -1281,6 +1273,8 @@ parse_put_flow_set_masked_action(struct tc_flower 
> *flower,
>  }
>
>  ofpbuf_uninit(_buf);
> +action->type = TC_ACT_PEDIT;
> +flower->action_count++;
>  return 0;
>  }
>
> diff --git a/lib/tc.c b/lib/tc.c
> index adb2d3182ad4..b637f4c17ed9 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1006,14 +1006,14 @@ static const struct nl_policy pedit_policy[] = {
>  static int
>  nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower)
>  {
> -struct tc_action *action;
> +struct tc_action *action = >actions[flower->action_count++];
>  struct nlattr *pe_attrs[ARRAY_SIZE(pedit_policy)];
>  const struct tc_pedit *pe;
>  const struct tc_pedit_key *keys;
>  const struct nlattr *nla, *keys_ex, *ex_type;
>  const void *keys_attr;
> -char *rewrite_key = (void *) >rewrite.key;
> -char *rewrite_mask = (void *) >rewrite.mask;
> +char *rewrite_key = (void *) >rewrite.key;
> +char *rewrite_mask = (void *) >rewrite.mask;
>  size_t keys_ex_size, left;
>  int type, i = 0, err;
>
> @@ -1092,7 +1092,6 @@ nl_parse_act_pedit(struct nlattr *options, struct 
> tc_flower *flower)
>  i++;
>  }
>
> -action = >actions[flower->action_count++];
>  action->type = TC_ACT_PEDIT;
>
>  return 0;
> @@ -2399,14 +2398,14 @@ nl_msg_put_act_flags(struct ofpbuf *request) {
>   * first_word_mask/last_word_mask - the mask to use for the first/last read
>   * (as we read entire words). */
>  static void
> -calc_offsets(struct tc_flower *flower, struct flower_key_to_pedit *m,
> +calc_offsets(struct tc_action *action, struct flower_key_to_pedit *m,
>   int *cur_offset, int *cnt, ovs_be32 *last_word_mask,
>   ovs_be32 *first_word_mask, ovs_be32 **mask, ovs_be32 **data)
>  {
>  int start_offset, max_offset, total_size;
>  int diff, right_zero_bits, left_zero_bits;
> -char *rewrite_key = (void *) >rewrite.key;
> -char *rewrite_mask = (void *) >rewrite.mask;
> +char