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

[Cong Wang]

On Sat, Oct 1, 2016 at 8:13 PM, Krister Johansen
<k...@templeofstupid.com> wrote:
> A tc_action_ops structure is visibile as soon as it is placed in the
> act_base list.  When tcf_regsiter_action adds an item to this list and
> drops act_mod_lock, registration is not complete until
> register_pernet_subsys() finishes.

Hmm, good catch, but does the fix have to be so complicated?

How about moving register_pernet_subsys() under act_mod_lock?
Similar is needed for unregister too of course. This also means
we need to convert act_mod_lock to a mutex which allows blocking.
Fortunately, we don't have to take act_mod_lock in any atomic context.

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.

Thanks!

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d09d068..4aac846 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -330,7 +330,22 @@ void tcf_hashinfo_destroy(const struct tc_action_ops *ops,
 EXPORT_SYMBOL(tcf_hashinfo_destroy);
 
 static LIST_HEAD(act_base);
-static DEFINE_RWLOCK(act_mod_lock);
+static DEFINE_MUTEX(act_mod_lock);
+
+static int __tcf_unregister_action(struct tc_action_ops *act)
+{
+       struct tc_action_ops *a;
+       int err = -ENOENT;
+
+       list_for_each_entry(a, &act_base, head) {
+               if (a == act) {
+                       list_del_rcu(&act->head);
+                       err = 0;
+                       break;
+               }
+       }
+       return err;
+}
 
 int tcf_register_action(struct tc_action_ops *act,
                        struct pernet_operations *ops)
@@ -341,22 +356,23 @@ int tcf_register_action(struct tc_action_ops *act,
        if (!act->act || !act->dump || !act->init || !act->walk || !act->lookup)
                return -EINVAL;
 
-       write_lock(&act_mod_lock);
+       mutex_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);
+                       mutex_unlock(&act_mod_lock);
                        return -EEXIST;
                }
        }
-       list_add_tail(&act->head, &act_base);
-       write_unlock(&act_mod_lock);
+       list_add_tail_rcu(&act->head, &act_base);
 
        ret = register_pernet_subsys(ops);
        if (ret) {
-               tcf_unregister_action(act, ops);
+               __tcf_unregister_action(act);
+               mutex_unlock(&act_mod_lock);
                return ret;
        }
 
+       mutex_unlock(&act_mod_lock);
        return 0;
 }
 EXPORT_SYMBOL(tcf_register_action);
@@ -364,20 +380,13 @@ EXPORT_SYMBOL(tcf_register_action);
 int tcf_unregister_action(struct tc_action_ops *act,
                          struct pernet_operations *ops)
 {
-       struct tc_action_ops *a;
        int err = -ENOENT;
 
+       mutex_lock(&act_mod_lock);
        unregister_pernet_subsys(ops);
+       err = __tcf_unregister_action(act);
+       mutex_unlock(&act_mod_lock);
 
-       write_lock(&act_mod_lock);
-       list_for_each_entry(a, &act_base, head) {
-               if (a == act) {
-                       list_del(&act->head);
-                       err = 0;
-                       break;
-               }
-       }
-       write_unlock(&act_mod_lock);
        return err;
 }
 EXPORT_SYMBOL(tcf_unregister_action);
@@ -388,15 +397,15 @@ static struct tc_action_ops *tc_lookup_action_n(char 
*kind)
        struct tc_action_ops *a, *res = NULL;
 
        if (kind) {
-               read_lock(&act_mod_lock);
-               list_for_each_entry(a, &act_base, head) {
+               rcu_read_lock();
+               list_for_each_entry_rcu(a, &act_base, head) {
                        if (strcmp(kind, a->kind) == 0) {
                                if (try_module_get(a->owner))
                                        res = a;
                                break;
                        }
                }
-               read_unlock(&act_mod_lock);
+               rcu_read_unlock();
        }
        return res;
 }
@@ -407,15 +416,15 @@ static struct tc_action_ops *tc_lookup_action(struct 
nlattr *kind)
        struct tc_action_ops *a, *res = NULL;
 
        if (kind) {
-               read_lock(&act_mod_lock);
-               list_for_each_entry(a, &act_base, head) {
+               rcu_read_lock();
+               list_for_each_entry_rcu(a, &act_base, head) {
                        if (nla_strcmp(kind, a->kind) == 0) {
                                if (try_module_get(a->owner))
                                        res = a;
                                break;
                        }
                }
-               read_unlock(&act_mod_lock);
+               rcu_read_unlock();
        }
        return res;
 }