[ovs-dev] [PATCH 1/2] datapath: Fix race and clarify locking.

Jarno Rajahalme jrajahalme at nicira.com
Tue Feb 4 19:10:42 UTC 2014


ovs_vport_cmd_dump() did rcu_read_lock() only after getting the
datapath, which could have been deleted in between.  Resolved by
taking rcu_read_lock() before the get_dp() call.

Remove unnecessary locking from functions that are always called with
appropriate locking (get_dp(), ovs_dp_cmd_fill_info(),
lookup_datapath()).

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 datapath/datapath.c |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 5f1b34c..7992330 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -116,20 +116,19 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *,
 static int queue_userspace_packet(struct datapath *dp, struct sk_buff *,
 				  const struct dp_upcall_info *);
 
-/* Must be called with rcu_read_lock or ovs_mutex. */
+/* Must be called with rcu_read_lock or ovs_mutex, so no additional protection
+ * is needed here. */
 static struct datapath *get_dp(struct net *net, int dp_ifindex)
 {
 	struct datapath *dp = NULL;
 	struct net_device *dev;
 
-	rcu_read_lock();
 	dev = dev_get_by_index_rcu(net, dp_ifindex);
 	if (dev) {
 		struct vport *vport = ovs_internal_dev_get_vport(dev);
 		if (vport)
 			dp = vport->dp;
 	}
-	rcu_read_unlock();
 
 	return dp;
 }
@@ -175,6 +174,7 @@ static struct hlist_head *vport_hash_bucket(const struct datapath *dp,
 	return &dp->ports[port_no & (DP_VPORT_HASH_BUCKETS - 1)];
 }
 
+/* Called with ovs_mutex or RCU read lock. */
 struct vport *ovs_lookup_vport(const struct datapath *dp, u16 port_no)
 {
 	struct vport *vport;
@@ -650,7 +650,7 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
 		+ nla_total_size(acts->actions_len); /* OVS_FLOW_ATTR_ACTIONS */
 }
 
-/* Called with ovs_mutex. */
+/* Must be called with rcu_read_lock or ovs_mutex. */
 static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
 				  struct sk_buff *skb, u32 portid,
 				  u32 seq, u32 flags, u8 cmd)
@@ -741,6 +741,7 @@ error:
 	return err;
 }
 
+/* Must be called with rcu_read_lock or ovs_mutex. */
 static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
 					       struct genl_info *info)
 {
@@ -751,6 +752,7 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
 	return genlmsg_new_unicast(len, info, GFP_KERNEL);
 }
 
+/* Must be called with rcu_read_lock or ovs_mutex. */
 static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
 					       struct datapath *dp,
 					       struct genl_info *info,
@@ -1091,6 +1093,7 @@ static size_t ovs_dp_cmd_msg_size(void)
 	return msgsize;
 }
 
+/* Must be called with rcu_read_lock or ovs_mutex. */
 static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
 				u32 portid, u32 seq, u32 flags, u8 cmd)
 {
@@ -1106,9 +1109,7 @@ static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
 
 	ovs_header->dp_ifindex = get_dpifindex(dp);
 
-	rcu_read_lock();
 	err = nla_put_string(skb, OVS_DP_ATTR_NAME, ovs_dp_name(dp));
-	rcu_read_unlock();
 	if (err)
 		goto nla_put_failure;
 
@@ -1133,6 +1134,7 @@ error:
 	return -EMSGSIZE;
 }
 
+/* Must be called with rcu_read_lock or ovs_mutex. */
 static struct sk_buff *ovs_dp_cmd_build_info(struct datapath *dp,
 					     struct genl_info *info, u8 cmd)
 {
@@ -1151,7 +1153,7 @@ static struct sk_buff *ovs_dp_cmd_build_info(struct datapath *dp,
 	return skb;
 }
 
-/* Called with ovs_mutex. */
+/* Called with rcu_read_lock or ovs_mutex. */
 static struct datapath *lookup_datapath(struct net *net,
 					struct ovs_header *ovs_header,
 					struct nlattr *a[OVS_DP_ATTR_MAX + 1])
@@ -1163,10 +1165,8 @@ static struct datapath *lookup_datapath(struct net *net,
 	else {
 		struct vport *vport;
 
-		rcu_read_lock();
 		vport = ovs_vport_locate(net, nla_data(a[OVS_DP_ATTR_NAME]));
 		dp = vport && vport->port_no == OVSP_LOCAL ? vport->dp : NULL;
-		rcu_read_unlock();
 	}
 	return dp ? dp : ERR_PTR(-ENODEV);
 }
@@ -1764,11 +1764,12 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	int bucket = cb->args[0], skip = cb->args[1];
 	int i, j = 0;
 
+	rcu_read_lock();
 	dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
-	if (!dp)
+	if (!dp) {
+		rcu_read_unlock();
 		return -ENODEV;
-
-	rcu_read_lock();
+	}
 	for (i = bucket; i < DP_VPORT_HASH_BUCKETS; i++) {
 		struct vport *vport;
 
-- 
1.7.10.4




More information about the dev mailing list