Re: [PATCH net backport 5.6.14-5.8.3 v1] net: openvswitch: introduce common code for flushing flows

2020-08-27 Thread Tonghao Zhang
On Wed, Aug 26, 2020 at 6:31 PM Greg KH  wrote:
>
> On Tue, Aug 25, 2020 at 01:25:32PM +0800, xiangxia.m@gmail.com wrote:
> > From: Tonghao Zhang 
> >
> > [ Upstream commit 77b981c82c1df7c7ad32a046f17f007450b46954 ]
>
> That is not what this commit is :(
>
> Please fix up and resend with the correct commit.
Sorry for that. v2 is sent, please review.
http://patchwork.ozlabs.org/project/netdev/patch/20200827061952.5789-1-xiangxia.m@gmail.com/
> thanks,
>
> greg k-h



-- 
Best regards, Tonghao


Re: [PATCH net backport 5.6.14-5.8.3 v1] net: openvswitch: introduce common code for flushing flows

2020-08-26 Thread Greg KH
On Tue, Aug 25, 2020 at 01:25:32PM +0800, xiangxia.m@gmail.com wrote:
> From: Tonghao Zhang 
> 
> [ Upstream commit 77b981c82c1df7c7ad32a046f17f007450b46954 ]

That is not what this commit is :(

Please fix up and resend with the correct commit.

thanks,

greg k-h


[PATCH net backport 5.6.14-5.8.3 v1] net: openvswitch: introduce common code for flushing flows

2020-08-24 Thread xiangxia . m . yue
From: Tonghao Zhang 

[ Upstream commit 77b981c82c1df7c7ad32a046f17f007450b46954 ]

Backport this commit to 5.6.14 - 5.8.3.

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 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html
Signed-off-by: Tonghao Zhang 
Reviewed-by: Cong Wang 
Signed-off-by: David S. Miller 
---
 net/openvswitch/datapath.c   | 10 +-
 net/openvswitch/flow_table.c | 35 +++
 net/openvswitch/flow_table.h |  3 +++
 3 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 94b024534987..03b81aa99975 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1736,6 +1736,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 = >table;
int i;
 
for (i = 0; i < DP_VPORT_HASH_BUCKETS; i++) {
@@ -1754,7 +1755,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(>rcu, destroy_dp_rcu);
 }
 
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 2398d7238300..f198bbb0c517 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -345,19 +345,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;
@@ -369,18 +365,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(>rcu, flow_tbl_destroy_rcu_cb);
-   call_rcu(_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(>rcu, flow_tbl_destroy_rcu_cb);
+   call_rcu(_ti->rcu, flow_tbl_destroy_rcu_cb);
 }
 
 /* No need for locking this function is called from RCU callback or
@@ -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);
 }
 
 struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *ti,
@@ -511,7 +505,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 8a5cea6ae111..8ea8fc957377 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -86,4 +86,7 @@ bool ovs_flow_cmp(const struct sw_flow *, const struct 
sw_flow_match *);
 
 void ovs_flow_mask_key(struct sw_flow_key