net_sched: reorder pernet ops and act ops registrations

Krister reported a kernel NULL pointer dereference after
tcf_action_init_1() invokes a_o->init(), it is a race condition
where one thread calling tcf_register_action() to initialize
the netns data after putting act ops in the global list and
the other thread searching the list and then calling
a_o->init(net, ...).

Fix this by moving the pernet ops registration before making
the action ops visible. This is fine because: a) we don't
rely on act_base in pernet ops->init(), b) in the worst case we
have a fully initialized netns but ops is still not ready so
new actions still can't be created.

Reported-by: Krister Johansen <kjlx@templeofstupid.com>
Tested-by: Krister Johansen <kjlx@templeofstupid.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index c9102172ce3b0fa72ceabacc8475ae30298d0ec4..a512b18c0088506bc577d8b3a1113871206c6d47 100644 (file)
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -341,22 +341,25 @@ int tcf_register_action(struct tc_action_ops *act,
        if (!act->act || !act->dump || !act->init || !act->walk || !act->lookup)
                return -EINVAL;
 
+       /* We have to register pernet ops before making the action ops visible,
+        * otherwise tcf_action_init_1() could get a partially initialized
+        * netns.
+        */
+       ret = register_pernet_subsys(ops);
+       if (ret)
+               return ret;
+
        write_lock(&act_mod_lock);
        list_for_each_entry(a, &act_base, head) {
                if (act->type == a->type || (strcmp(act->kind, a->kind) == 0)) {
                        write_unlock(&act_mod_lock);
+                       unregister_pernet_subsys(ops);
                        return -EEXIST;
                }
        }
        list_add_tail(&act->head, &act_base);
        write_unlock(&act_mod_lock);
 
-       ret = register_pernet_subsys(ops);
-       if (ret) {
-               tcf_unregister_action(act, ops);
-               return ret;
-       }
-
        return 0;
 }
 EXPORT_SYMBOL(tcf_register_action);
@@ -367,8 +370,6 @@ int tcf_unregister_action(struct tc_action_ops *act,
        struct tc_action_ops *a;
        int err = -ENOENT;
 
-       unregister_pernet_subsys(ops);
-
        write_lock(&act_mod_lock);
        list_for_each_entry(a, &act_base, head) {
                if (a == act) {
@@ -378,6 +379,8 @@ int tcf_unregister_action(struct tc_action_ops *act,
                }
        }
        write_unlock(&act_mod_lock);
+       if (!err)
+               unregister_pernet_subsys(ops);
        return err;
 }
 EXPORT_SYMBOL(tcf_unregister_action);