diff mbox series

[v2] net: openvswitch: introduce common code for flushing flows

Message ID 20200812095639.4062-1-xiangxia.m.yue@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series [v2] net: openvswitch: introduce common code for flushing flows | expand

Commit Message

Tonghao Zhang Aug. 12, 2020, 9:56 a.m. UTC 
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

To avoid some issues, for example RCU usage warning and double free,
we should flush the flows under ovs_lock. This patch refactors
table_instance_destroy and introduces table_instance_flow_flush
which can be invoked by __dp_destroy or ovs_flow_tbl_flush.

Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
Reported-by: Johan Knöös <jknoos@google.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
v2:
* Change local var coding style
* Add reported and fixed tag
---
 net/openvswitch/datapath.c   | 10 +++++++++-
 net/openvswitch/flow_table.c | 35 +++++++++++++++--------------------
 net/openvswitch/flow_table.h |  3 +++
 3 files changed, 27 insertions(+), 21 deletions(-)

Comments

Cong Wang Aug. 13, 2020, 7:04 p.m. UTC | #1 
On Wed, Aug 12, 2020 at 2:59 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> To avoid some issues, for example RCU usage warning and double free,
> we should flush the flows under ovs_lock. This patch refactors
> table_instance_destroy and introduces table_instance_flow_flush
> which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
>
> Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
> Reported-by: Johan Knöös <jknoos@google.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks.
David Miller Aug. 13, 2020, 10:53 p.m. UTC | #2 
From: xiangxia.m.yue@gmail.com
Date: Wed, 12 Aug 2020 17:56:39 +0800

> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> To avoid some issues, for example RCU usage warning and double free,
> we should flush the flows under ovs_lock. This patch refactors
> table_instance_destroy and introduces table_instance_flow_flush
> which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
> 
> Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
> Reported-by: Johan Knöös <jknoos@google.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Applied, thank you.
Johan Knöös Aug. 14, 2020, 7:27 p.m. UTC | #3 
On Thu, Aug 13, 2020 at 3:53 PM David Miller <davem@davemloft.net> wrote:
>
> From: xiangxia.m.yue@gmail.com
> Date: Wed, 12 Aug 2020 17:56:39 +0800
>
> > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > To avoid some issues, for example RCU usage warning and double free,
> > we should flush the flows under ovs_lock. This patch refactors
> > table_instance_destroy and introduces table_instance_flow_flush
> > which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
> >
> > Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
> > Reported-by: Johan Knöös <jknoos@google.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> Applied, thank you.

Tonghao, does the following change to your commit make sense to be
able to apply it on 5.6.14 (e3ac9117b18596b7363d5b7904ab03a7d782b40c)?

@@ -393,7 +387,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)

        free_percpu(table->mask_cache);
        kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
-       table_instance_destroy(table, ti, ufid_ti, false);
+       table_instance_destroy(ti, ufid_ti);
 }
Tonghao Zhang Aug. 15, 2020, 7:39 a.m. UTC | #4 
On Sat, Aug 15, 2020 at 3:28 AM Johan Knöös <jknoos@google.com> wrote:
>
> On Thu, Aug 13, 2020 at 3:53 PM David Miller <davem@davemloft.net> wrote:
> >
> > From: xiangxia.m.yue@gmail.com
> > Date: Wed, 12 Aug 2020 17:56:39 +0800
> >
> > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > To avoid some issues, for example RCU usage warning and double free,
> > > we should flush the flows under ovs_lock. This patch refactors
> > > table_instance_destroy and introduces table_instance_flow_flush
> > > which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
> > >
> > > Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
> > > Reported-by: Johan Knöös <jknoos@google.com>
> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >
> > Applied, thank you.
>
> Tonghao, does the following change to your commit make sense to be
> able to apply it on 5.6.14 (e3ac9117b18596b7363d5b7904ab03a7d782b40c)?
Not applied cleanly, if necessary I can send v3 for 5.6.14.
> @@ -393,7 +387,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
>
>         free_percpu(table->mask_cache);
>         kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
> -       table_instance_destroy(table, ti, ufid_ti, false);
> +       table_instance_destroy(ti, ufid_ti);
>  }
Johan Knöös Aug. 17, 2020, 8:13 p.m. UTC | #5 
On Sat, Aug 15, 2020 at 12:48 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Sat, Aug 15, 2020 at 3:28 AM Johan Knöös <jknoos@google.com> wrote:
> >
> > On Thu, Aug 13, 2020 at 3:53 PM David Miller <davem@davemloft.net> wrote:
> > >
> > > From: xiangxia.m.yue@gmail.com
> > > Date: Wed, 12 Aug 2020 17:56:39 +0800
> > >
> > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > > >
> > > > To avoid some issues, for example RCU usage warning and double free,
> > > > we should flush the flows under ovs_lock. This patch refactors
> > > > table_instance_destroy and introduces table_instance_flow_flush
> > > > which can be invoked by __dp_destroy or ovs_flow_tbl_flush.
> > > >
> > > > Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy flow-table")
> > > > Reported-by: Johan Knöös <jknoos@google.com>
> > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
> > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > >
> > > Applied, thank you.
> >
> > Tonghao, does the following change to your commit make sense to be
> > able to apply it on 5.6.14 (e3ac9117b18596b7363d5b7904ab03a7d782b40c)?
> Not applied cleanly, if necessary I can send v3 for 5.6.14.

That would be appreciated. Thanks!

> > @@ -393,7 +387,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
> >
> >         free_percpu(table->mask_cache);
> >         kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
> > -       table_instance_destroy(table, ti, ufid_ti, false);
> > +       table_instance_destroy(ti, ufid_ti);
> >  }
>
>
>
> --
> Best regards, Tonghao
diff mbox series

Patch

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 42f8cc70bb2c..6e47ef7ef036 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1756,6 +1756,7 @@  static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
 /* Called with ovs_mutex. */
 static void __dp_destroy(struct datapath *dp)
 {
+	struct flow_table *table = &dp->table;
 	int i;
 
 	for (i = 0; i < DP_VPORT_HASH_BUCKETS; i++) {
@@ -1774,7 +1775,14 @@  static void __dp_destroy(struct datapath *dp)
 	 */
 	ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
 
-	/* RCU destroy the flow table */
+	/* Flush sw_flow in the tables. RCU cb only releases resource
+	 * such as dp, ports and tables. That may avoid some issues
+	 * such as RCU usage warning.
+	 */
+	table_instance_flow_flush(table, ovsl_dereference(table->ti),
+				  ovsl_dereference(table->ufid_ti));
+
+	/* RCU destroy the ports, meters and flow tables. */
 	call_rcu(&dp->rcu, destroy_dp_rcu);
 }
 
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 8c12675cbb67..e2235849a57e 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -473,19 +473,15 @@  static void table_instance_flow_free(struct flow_table *table,
 	flow_mask_remove(table, flow->mask);
 }
 
-static void table_instance_destroy(struct flow_table *table,
-				   struct table_instance *ti,
-				   struct table_instance *ufid_ti,
-				   bool deferred)
+/* Must be called with OVS mutex held. */
+void table_instance_flow_flush(struct flow_table *table,
+			       struct table_instance *ti,
+			       struct table_instance *ufid_ti)
 {
 	int i;
 
-	if (!ti)
-		return;
-
-	BUG_ON(!ufid_ti);
 	if (ti->keep_flows)
-		goto skip_flows;
+		return;
 
 	for (i = 0; i < ti->n_buckets; i++) {
 		struct sw_flow *flow;
@@ -497,18 +493,16 @@  static void table_instance_destroy(struct flow_table *table,
 
 			table_instance_flow_free(table, ti, ufid_ti,
 						 flow, false);
-			ovs_flow_free(flow, deferred);
+			ovs_flow_free(flow, true);
 		}
 	}
+}
 
-skip_flows:
-	if (deferred) {
-		call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
-		call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
-	} else {
-		__table_instance_destroy(ti);
-		__table_instance_destroy(ufid_ti);
-	}
+static void table_instance_destroy(struct table_instance *ti,
+				   struct table_instance *ufid_ti)
+{
+	call_rcu(&ti->rcu, flow_tbl_destroy_rcu_cb);
+	call_rcu(&ufid_ti->rcu, flow_tbl_destroy_rcu_cb);
 }
 
 /* No need for locking this function is called from RCU callback or
@@ -523,7 +517,7 @@  void ovs_flow_tbl_destroy(struct flow_table *table)
 
 	call_rcu(&mc->rcu, mask_cache_rcu_cb);
 	call_rcu(&ma->rcu, mask_array_rcu_cb);
-	table_instance_destroy(table, ti, ufid_ti, false);
+	table_instance_destroy(ti, ufid_ti);
 }
 
 struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
@@ -641,7 +635,8 @@  int ovs_flow_tbl_flush(struct flow_table *flow_table)
 	flow_table->count = 0;
 	flow_table->ufid_count = 0;
 
-	table_instance_destroy(flow_table, old_ti, old_ufid_ti, true);
+	table_instance_flow_flush(flow_table, old_ti, old_ufid_ti);
+	table_instance_destroy(old_ti, old_ufid_ti);
 	return 0;
 
 err_free_ti:
diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
index 74ce48fecba9..6e7d4ac59353 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -105,5 +105,8 @@  void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
 		       bool full, const struct sw_flow_mask *mask);
 
 void ovs_flow_masks_rebalance(struct flow_table *table);
+void table_instance_flow_flush(struct flow_table *table,
+			       struct table_instance *ti,
+			       struct table_instance *ufid_ti);
 
 #endif /* flow_table.h */