On 23 Jul 2020, at 13:06, Hillf Danton wrote:
Wed, 22 Jul 2020 23:27:19 -0700
syzbot found the following issue on:
<SNIP>
Fixes: eac87c413bf9 ("net: openvswitch: reorder masks array based on
usage")
by moving cancel_delayed_work_sync() in to the rcu cb, therefore out
of ovs
lock. To facilitate that, add a flag in datapath to inform the kworker
that
there is no more work needed.
I was thinking of re-working the patch and move the handling to the
“struct ovs_net” instead of the datapath. This way the rebalance
worker can rebalance all datapaths in the netns. Than I can move
cancel_delayed_work_sync() from __dp_destroy()
to ovs_exit_net(), i.e. outside the ovs lock scope.
However, your fix would be way less intrusive. Are you planning on
sending it as a patch? If so, maybe add a comment around the called_rcu
variable to be more clear where it’s used for, or maybe rename it to
something like called_destory_rcu?
If you think my approach would be better let me know, and I work on a
patch.
Feedback anyone?
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -82,6 +82,7 @@ struct datapath {
u32 max_headroom;
+ int called_rcu;
/* Switch meters. */
struct dp_meter_table meter_tbl;
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -161,6 +161,7 @@ static void destroy_dp_rcu(struct rcu_he
{
struct datapath *dp = container_of(rcu, struct datapath, rcu);
+ cancel_delayed_work_sync(&dp->masks_rebalance);
ovs_flow_tbl_destroy(&dp->table);
free_percpu(dp->stats_percpu);
kfree(dp->ports);
@@ -1760,11 +1761,9 @@ static void __dp_destroy(struct datapath
*/
ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
+ dp->called_rcu = true;
/* RCU destroy the flow table */
call_rcu(&dp->rcu, destroy_dp_rcu);
-
- /* Cancel remaining work. */
- cancel_delayed_work_sync(&dp->masks_rebalance);
}
static int ovs_dp_cmd_del(struct sk_buff *skb, struct genl_info
*info)
@@ -2356,6 +2355,8 @@ static void ovs_dp_masks_rebalance(struc
ovs_flow_masks_rebalance(&dp->table);
ovs_unlock();
+ if (dp->called_rcu)
+ return;
schedule_delayed_work(&dp->masks_rebalance,
msecs_to_jiffies(DP_MASKS_REBALANCE_INTERVAL));
}
