Re: [PATCH net] Panic when tc_lookup_action_n finds a partially initialized action.

[Cong Wang]

On Tue, Oct 4, 2016 at 11:52 PM, Krister Johansen
<k...@templeofstupid.com> wrote:
> On Mon, Oct 03, 2016 at 11:22:33AM -0700, Cong Wang wrote:
>> Please try the attached patch. I also convert the read path to RCU
>> to avoid a possible deadlock. A quick test shows no lockdep splat.
>
> I tried this patch, but it doesn't solve the problem.  I got a panic on
> my very first try:

Thanks for testing it.


> The problem here is the same as before: by using RCU the race isn't
> fixed because the module is still discoverable from act_base before the
> pernet initialization is completed.
>
> You can see from the trap frame that the first two arguments to
> tcf_hash_check were 0.  It couldn't look up the correct per-subsystem
> pointer because the id hadn't yet been registered.

I thought the problem is that we don't do pernet ops registration and
action ops registration atomically therefore chose to use mutex+RCU,
but I was wrong, the problem here is just ordering, we need to finish
the pernet initialization before making action ops visible.

If so, why not just reorder them? Does the attached patch make any
sense now? Our pernet init doesn't rely on act_base, so even we have
some race, the worst case is after we initialize the pernet netns for an
action but its ops still not visible, which seems fine (at least no crash).

Or I still miss something here?

(Sorry that I don't have the environment to reproduce your bug)

Thanks for your patience and testing!

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d09d068..6024920 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -341,22 +341,21 @@ int tcf_register_action(struct tc_action_ops *act,
        if (!act->act || !act->dump || !act->init || !act->walk || !act->lookup)
                return -EINVAL;
 
+       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 +366,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 +375,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);