Properly lock dp_mutex around changes to the datapath.
authorBen Pfaff <blp@nicira.com>
Tue, 30 Dec 2008 21:03:48 +0000 (13:03 -0800)
committerBen Pfaff <blp@nicira.com>
Tue, 30 Dec 2008 21:05:15 +0000 (13:05 -0800)
We weren't locking dp_mutex() here but it really is necessary.  See the
comment on dp_mutex itself for details.

This actually restores some of the locking removed by commit 47b8652d
"Simplify use of dp_mutex."  That commit is correct that we can take
dp_mutex at a high level in dp_genl_openflow(), but it removes locking
from functions that are not called through dp_genl_openflow(): in
particular any Netlink command other than DP_GENL_C_OPENFLOW does not
go through that function, so those commands need to acquire the mutex
themselves.

datapath/datapath.c

index e10b520696cd900c67085cd4ef7df5d79c0c3486..e5f011d1336de48ab5091fb1d0a9be2216f0105d 100644 (file)
@@ -258,25 +258,27 @@ static int new_dp(int dp_idx, const char *dp_name)
        struct datapath *dp;
        int err;
 
+       mutex_lock(&dp_mutex);
        if (dp_idx == -1) 
                dp_idx = gen_dp_idx();
 
+       err = -EINVAL;
        if (dp_idx < 0 || dp_idx >= DP_MAX)
-               return -EINVAL;
+               goto err_unlock;
 
+       err = -ENODEV;
        if (!try_module_get(THIS_MODULE))
-               return -ENODEV;
+               goto err_unlock;
 
        /* Exit early if a datapath with that number already exists. */
-       if (dps[dp_idx]) {
-               err = -EEXIST;
-               goto err_unlock;
-       }
+       err = -EEXIST;
+       if (dps[dp_idx])
+               goto err_put;
 
        err = -ENOMEM;
        dp = kzalloc(sizeof *dp, GFP_KERNEL);
        if (dp == NULL)
-               goto err_unlock;
+               goto err_put;
 
        dp->dp_idx = dp_idx;
 
@@ -304,6 +306,7 @@ static int new_dp(int dp_idx, const char *dp_name)
                goto err_destroy_chain;
 
        dps[dp_idx] = dp;
+       mutex_unlock(&dp_mutex);
 
        return 0;
 
@@ -315,9 +318,11 @@ err_destroy_dp_dev:
        dp_dev_destroy(dp);
 err_free_dp:
        kfree(dp);
-err_unlock:
+err_put:
        module_put(THIS_MODULE);
-               return err;
+err_unlock:
+       mutex_unlock(&dp_mutex);
+       return err;
 }
 
 /* Find and return a free port number under 'dp'. */
@@ -1172,6 +1177,7 @@ static int dp_genl_del(struct sk_buff *skb, struct genl_info *info)
        struct datapath *dp;
        int err;
 
+       mutex_lock(&dp_mutex);
        dp = lookup_dp(info);
        if (IS_ERR(dp))
                err = PTR_ERR(dp);
@@ -1179,6 +1185,7 @@ static int dp_genl_del(struct sk_buff *skb, struct genl_info *info)
                del_dp(dp);
                err = 0;
        }
+       mutex_unlock(&dp_mutex);
        return err;
 }
 
@@ -1256,10 +1263,11 @@ static int dp_genl_add_del_port(struct sk_buff *skb, struct genl_info *info)
                return -EINVAL;
 
        /* Get datapath. */
+       mutex_lock(&dp_mutex);
        dp = lookup_dp(info);
        if (IS_ERR(dp)) {
                err = PTR_ERR(dp);
-               goto out;
+               goto out_unlock;
        }
 
        /* Get interface to add/remove. */
@@ -1267,7 +1275,7 @@ static int dp_genl_add_del_port(struct sk_buff *skb, struct genl_info *info)
                        nla_data(info->attrs[DP_GENL_A_PORTNAME]));
        if (!port) {
                err = -ENOENT;
-               goto out;
+               goto out_unlock;
        }
 
        /* Execute operation. */
@@ -1283,7 +1291,8 @@ static int dp_genl_add_del_port(struct sk_buff *skb, struct genl_info *info)
 
 out_put:
        dev_put(port);
-out:
+out_unlock:
+       mutex_unlock(&dp_mutex);
        return err;
 }