act_ife: move tcfa_lock down to where necessary

The only time we need to take tcfa_lock is when adding
a new metainfo to an existing ife->metalist. We don't need
to take tcfa_lock so early and so broadly in tcf_ife_init().

This means we can always take ife_mod_lock first, avoid the
reverse locking ordering warning as reported by Vlad.

Reported-by: Vlad Buslov <vladbu@mellanox.com>
Tested-by: Vlad Buslov <vladbu@mellanox.com>
Cc: Vlad Buslov <vladbu@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 9decbb74b3ac837aa1f2ad57b29feb2436abb987..244a8cf48183509c76364e71ecc7505f0623d3bf 100644 (file)
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -265,11 +265,8 @@ static const char *ife_meta_id2name(u32 metaid)
 #endif
 
 /* called when adding new meta information
- * under ife->tcf_lock for existing action
 */
-static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
-                               void *val, int len, bool exists,
-                               bool rtnl_held)
+static int load_metaops_and_vet(u32 metaid, void *val, int len, bool rtnl_held)
 {
        struct tcf_meta_ops *ops = find_ife_oplist(metaid);
        int ret = 0;
@@ -277,15 +274,11 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
        if (!ops) {
                ret = -ENOENT;
 #ifdef CONFIG_MODULES
-               if (exists)
-                       spin_unlock_bh(&ife->tcf_lock);
                if (rtnl_held)
                        rtnl_unlock();
                request_module("ife-meta-%s", ife_meta_id2name(metaid));
                if (rtnl_held)
                        rtnl_lock();
-               if (exists)
-                       spin_lock_bh(&ife->tcf_lock);
                ops = find_ife_oplist(metaid);
 #endif
        }
@@ -302,10 +295,9 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
 }
 
 /* called when adding new meta information
- * under ife->tcf_lock for existing action
 */
 static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
-                       int len, bool atomic)
+                       int len, bool atomic, bool exists)
 {
        struct tcf_meta_info *mi = NULL;
        struct tcf_meta_ops *ops = find_ife_oplist(metaid);
@@ -332,12 +324,16 @@ static int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval,
                }
        }
 
+       if (exists)
+               spin_lock_bh(&ife->tcf_lock);
        list_add_tail(&mi->metalist, &ife->metalist);
+       if (exists)
+               spin_unlock_bh(&ife->tcf_lock);
 
        return ret;
 }
 
-static int use_all_metadata(struct tcf_ife_info *ife)
+static int use_all_metadata(struct tcf_ife_info *ife, bool exists)
 {
        struct tcf_meta_ops *o;
        int rc = 0;
@@ -345,7 +341,7 @@ static int use_all_metadata(struct tcf_ife_info *ife)
 
        read_lock(&ife_mod_lock);
        list_for_each_entry(o, &ifeoplist, list) {
-               rc = add_metainfo(ife, o->metaid, NULL, 0, true);
+               rc = add_metainfo(ife, o->metaid, NULL, 0, true, exists);
                if (rc == 0)
                        installed += 1;
        }
@@ -422,7 +418,6 @@ static void tcf_ife_cleanup(struct tc_action *a)
                kfree_rcu(p, rcu);
 }
 
-/* under ife->tcf_lock for existing action */
 static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
                             bool exists, bool rtnl_held)
 {
@@ -436,12 +431,11 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
                        val = nla_data(tb[i]);
                        len = nla_len(tb[i]);
 
-                       rc = load_metaops_and_vet(ife, i, val, len, exists,
-                                                 rtnl_held);
+                       rc = load_metaops_and_vet(i, val, len, rtnl_held);
                        if (rc != 0)
                                return rc;
 
-                       rc = add_metainfo(ife, i, val, len, exists);
+                       rc = add_metainfo(ife, i, val, len, false, exists);
                        if (rc)
                                return rc;
                }
@@ -540,8 +534,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
                p->eth_type = ife_type;
        }
 
-       if (exists)
-               spin_lock_bh(&ife->tcf_lock);
 
        if (ret == ACT_P_CREATED)
                INIT_LIST_HEAD(&ife->metalist);
@@ -551,10 +543,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
                                       NULL, NULL);
                if (err) {
 metadata_parse_err:
-                       if (exists)
-                               spin_unlock_bh(&ife->tcf_lock);
                        tcf_idr_release(*a, bind);
-
                        kfree(p);
                        return err;
                }
@@ -569,17 +558,16 @@ metadata_parse_err:
                 * as we can. You better have at least one else we are
                 * going to bail out
                 */
-               err = use_all_metadata(ife);
+               err = use_all_metadata(ife, exists);
                if (err) {
-                       if (exists)
-                               spin_unlock_bh(&ife->tcf_lock);
                        tcf_idr_release(*a, bind);
-
                        kfree(p);
                        return err;
                }
        }
 
+       if (exists)
+               spin_lock_bh(&ife->tcf_lock);
        ife->tcf_action = parm->action;
        /* protected by tcf_lock when modifying existing action */
        rcu_swap_protected(ife->params, p, 1);