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;
}
