Commit 28e6b67f authored by Daniel Borkmann's avatar Daniel Borkmann Committed by David S. Miller

net: sched: fix refcount imbalance in actions

Since commit 55334a5d ("net_sched: act: refuse to remove bound action
outside"), we end up with a wrong reference count for a tc action.

Test case 1:

  FOO="1,6 0 0 4294967295,"
  BAR="1,6 0 0 4294967294,"
  tc filter add dev foo parent 1: bpf bytecode "$FOO" flowid 1:1 \
     action bpf bytecode "$FOO"
  tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 1 ref 1 bind 1
  tc actions replace action bpf bytecode "$BAR" index 1
  tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967294' default-action pipe
    index 1 ref 2 bind 1
  tc actions replace action bpf bytecode "$FOO" index 1
  tc actions show action bpf
    action order 0: bpf bytecode '1,6 0 0 4294967295' default-action pipe
    index 1 ref 3 bind 1

Test case 2:

  FOO="1,6 0 0 4294967295,"
  tc filter add dev foo parent 1: bpf bytecode "$FOO" flowid 1:1 action ok
  tc actions show action gact
    action order 0: gact action pass
    random type none pass val 0
     index 1 ref 1 bind 1
  tc actions add action drop index 1
    RTNETLINK answers: File exists [...]
  tc actions show action gact
    action order 0: gact action pass
     random type none pass val 0
     index 1 ref 2 bind 1
  tc actions add action drop index 1
    RTNETLINK answers: File exists [...]
  tc actions show action gact
    action order 0: gact action pass
     random type none pass val 0
     index 1 ref 3 bind 1

What happens is that in tcf_hash_check(), we check tcf_common for a given
index and increase tcfc_refcnt and conditionally tcfc_bindcnt when we've
found an existing action. Now there are the following cases:

  1) We do a late binding of an action. In that case, we leave the
     tcfc_refcnt/tcfc_bindcnt increased and are done with the ->init()
     handler. This is correctly handeled.

  2) We replace the given action, or we try to add one without replacing
     and find out that the action at a specific index already exists
     (thus, we go out with error in that case).

In case of 2), we have to undo the reference count increase from
tcf_hash_check() in the tcf_hash_check() function. Currently, we fail to
do so because of the 'tcfc_bindcnt > 0' check which bails out early with
an -EPERM error.

Now, while commit 55334a5d prevents 'tc actions del action ...' on an
already classifier-bound action to drop the reference count (which could
then become negative, wrap around etc), this restriction only accounts for
invocations outside a specific action's ->init() handler.

One possible solution would be to add a flag thus we possibly trigger
the -EPERM ony in situations where it is indeed relevant.

After the patch, above test cases have correct reference count again.

Fixes: 55334a5d ("net_sched: act: refuse to remove bound action outside")
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Reviewed-by: default avatarCong Wang <cwang@twopensource.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 990c9b34
...@@ -99,7 +99,6 @@ struct tc_action_ops { ...@@ -99,7 +99,6 @@ struct tc_action_ops {
int tcf_hash_search(struct tc_action *a, u32 index); int tcf_hash_search(struct tc_action *a, u32 index);
void tcf_hash_destroy(struct tc_action *a); void tcf_hash_destroy(struct tc_action *a);
int tcf_hash_release(struct tc_action *a, int bind);
u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo); u32 tcf_hash_new_index(struct tcf_hashinfo *hinfo);
int tcf_hash_check(u32 index, struct tc_action *a, int bind); int tcf_hash_check(u32 index, struct tc_action *a, int bind);
int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a, int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
...@@ -107,6 +106,13 @@ int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a, ...@@ -107,6 +106,13 @@ int tcf_hash_create(u32 index, struct nlattr *est, struct tc_action *a,
void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est); void tcf_hash_cleanup(struct tc_action *a, struct nlattr *est);
void tcf_hash_insert(struct tc_action *a); void tcf_hash_insert(struct tc_action *a);
int __tcf_hash_release(struct tc_action *a, bool bind, bool strict);
static inline int tcf_hash_release(struct tc_action *a, bool bind)
{
return __tcf_hash_release(a, bind, false);
}
int tcf_register_action(struct tc_action_ops *a, unsigned int mask); int tcf_register_action(struct tc_action_ops *a, unsigned int mask);
int tcf_unregister_action(struct tc_action_ops *a); int tcf_unregister_action(struct tc_action_ops *a);
int tcf_action_destroy(struct list_head *actions, int bind); int tcf_action_destroy(struct list_head *actions, int bind);
......
...@@ -45,7 +45,7 @@ void tcf_hash_destroy(struct tc_action *a) ...@@ -45,7 +45,7 @@ void tcf_hash_destroy(struct tc_action *a)
} }
EXPORT_SYMBOL(tcf_hash_destroy); EXPORT_SYMBOL(tcf_hash_destroy);
int tcf_hash_release(struct tc_action *a, int bind) int __tcf_hash_release(struct tc_action *a, bool bind, bool strict)
{ {
struct tcf_common *p = a->priv; struct tcf_common *p = a->priv;
int ret = 0; int ret = 0;
...@@ -53,7 +53,7 @@ int tcf_hash_release(struct tc_action *a, int bind) ...@@ -53,7 +53,7 @@ int tcf_hash_release(struct tc_action *a, int bind)
if (p) { if (p) {
if (bind) if (bind)
p->tcfc_bindcnt--; p->tcfc_bindcnt--;
else if (p->tcfc_bindcnt > 0) else if (strict && p->tcfc_bindcnt > 0)
return -EPERM; return -EPERM;
p->tcfc_refcnt--; p->tcfc_refcnt--;
...@@ -64,9 +64,10 @@ int tcf_hash_release(struct tc_action *a, int bind) ...@@ -64,9 +64,10 @@ int tcf_hash_release(struct tc_action *a, int bind)
ret = 1; ret = 1;
} }
} }
return ret; return ret;
} }
EXPORT_SYMBOL(tcf_hash_release); EXPORT_SYMBOL(__tcf_hash_release);
static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb, static int tcf_dump_walker(struct sk_buff *skb, struct netlink_callback *cb,
struct tc_action *a) struct tc_action *a)
...@@ -136,7 +137,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a) ...@@ -136,7 +137,7 @@ static int tcf_del_walker(struct sk_buff *skb, struct tc_action *a)
head = &hinfo->htab[tcf_hash(i, hinfo->hmask)]; head = &hinfo->htab[tcf_hash(i, hinfo->hmask)];
hlist_for_each_entry_safe(p, n, head, tcfc_head) { hlist_for_each_entry_safe(p, n, head, tcfc_head) {
a->priv = p; a->priv = p;
ret = tcf_hash_release(a, 0); ret = __tcf_hash_release(a, false, true);
if (ret == ACT_P_DELETED) { if (ret == ACT_P_DELETED) {
module_put(a->ops->owner); module_put(a->ops->owner);
n_i++; n_i++;
...@@ -408,7 +409,7 @@ int tcf_action_destroy(struct list_head *actions, int bind) ...@@ -408,7 +409,7 @@ int tcf_action_destroy(struct list_head *actions, int bind)
int ret = 0; int ret = 0;
list_for_each_entry_safe(a, tmp, actions, list) { list_for_each_entry_safe(a, tmp, actions, list) {
ret = tcf_hash_release(a, bind); ret = __tcf_hash_release(a, bind, true);
if (ret == ACT_P_DELETED) if (ret == ACT_P_DELETED)
module_put(a->ops->owner); module_put(a->ops->owner);
else if (ret < 0) else if (ret < 0)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment