* [PATCH net] net: sched: fix refcount imbalance in actions
@ 2015-07-29 21:35 Daniel Borkmann
2015-07-30 0:33 ` Cong Wang
2015-07-30 21:21 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-29 21:35 UTC (permalink / raw)
To: davem; +Cc: cwang, ast, jhs, netdev, Daniel Borkmann
Since commit 55334a5db5cd ("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 55334a5db5cd 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: 55334a5db5cd ("net_sched: act: refuse to remove bound action outside")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/net/act_api.h | 8 +++++++-
net/sched/act_api.c | 11 ++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 3ee4c92..931738b 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -99,7 +99,6 @@ struct tc_action_ops {
int tcf_hash_search(struct tc_action *a, u32 index);
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);
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,
@@ -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_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_unregister_action(struct tc_action_ops *a);
int tcf_action_destroy(struct list_head *actions, int bind);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index af427a3..43ec926 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -45,7 +45,7 @@ void tcf_hash_destroy(struct tc_action *a)
}
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;
int ret = 0;
@@ -53,7 +53,7 @@ int tcf_hash_release(struct tc_action *a, int bind)
if (p) {
if (bind)
p->tcfc_bindcnt--;
- else if (p->tcfc_bindcnt > 0)
+ else if (strict && p->tcfc_bindcnt > 0)
return -EPERM;
p->tcfc_refcnt--;
@@ -64,9 +64,10 @@ int tcf_hash_release(struct tc_action *a, int bind)
ret = 1;
}
}
+
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,
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)];
hlist_for_each_entry_safe(p, n, head, tcfc_head) {
a->priv = p;
- ret = tcf_hash_release(a, 0);
+ ret = __tcf_hash_release(a, false, true);
if (ret == ACT_P_DELETED) {
module_put(a->ops->owner);
n_i++;
@@ -408,7 +409,7 @@ int tcf_action_destroy(struct list_head *actions, int bind)
int ret = 0;
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)
module_put(a->ops->owner);
else if (ret < 0)
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix refcount imbalance in actions
2015-07-29 21:35 [PATCH net] net: sched: fix refcount imbalance in actions Daniel Borkmann
@ 2015-07-30 0:33 ` Cong Wang
2015-07-30 0:55 ` Daniel Borkmann
2015-07-30 21:21 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: Cong Wang @ 2015-07-30 0:33 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David Miller, Alexei Starovoitov, Jamal Hadi Salim, netdev
On Wed, Jul 29, 2015 at 2:35 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 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 55334a5db5cd 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.
>
Hmm, so in the tcf_hash_check() !=0 path we forget to decrease the refcount,
maybe we should not increase it at all? Does the following simpler fix make
any sense? I will think more about this tomorrow.
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index af427a3..bd63a39 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -53,8 +53,11 @@ int tcf_hash_release(struct tc_action *a, int bind)
if (p) {
if (bind)
p->tcfc_bindcnt--;
- else if (p->tcfc_bindcnt > 0)
- return -EPERM;
+ else {
+ if (p->tcfc_bindcnt > 0)
+ return -EPERM;
+ return ret;
+ }
p->tcfc_refcnt--;
if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
@@ -214,9 +217,10 @@ int tcf_hash_check(u32 index, struct tc_action
*a, int bind)
struct tcf_hashinfo *hinfo = a->ops->hinfo;
struct tcf_common *p = NULL;
if (index && (p = tcf_hash_lookup(index, hinfo)) != NULL) {
- if (bind)
+ if (bind) {
p->tcfc_bindcnt++;
- p->tcfc_refcnt++;
+ p->tcfc_refcnt++;
+ }
a->priv = p;
return 1;
}
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index a42a3b2..2685450 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -98,6 +98,8 @@ static int tcf_mirred_init(struct net *net, struct
nlattr *nla,
return ret;
ret = ACT_P_CREATED;
} else {
+ if (bind)
+ return 0;
if (!ovr) {
tcf_hash_release(a, bind);
return -EEXIST;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix refcount imbalance in actions
2015-07-30 0:33 ` Cong Wang
@ 2015-07-30 0:55 ` Daniel Borkmann
2015-07-30 18:48 ` Cong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-30 0:55 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Alexei Starovoitov, Jamal Hadi Salim, netdev
On 07/30/2015 02:33 AM, Cong Wang wrote:
...
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index af427a3..bd63a39 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -53,8 +53,11 @@ int tcf_hash_release(struct tc_action *a, int bind)
> if (p) {
> if (bind)
> p->tcfc_bindcnt--;
> - else if (p->tcfc_bindcnt > 0)
> - return -EPERM;
> + else {
> + if (p->tcfc_bindcnt > 0)
> + return -EPERM;
> + return ret;
> + }
Hm, so this seems not correct: if we only ever increase tcfc_refcnt
when there's bind=1, and only ever decrease when bind=1, then we
will never free the action as we do start out from ref=1 in case
it has been added without initial binding, if I see this correctly.
> p->tcfc_refcnt--;
> if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
> @@ -214,9 +217,10 @@ int tcf_hash_check(u32 index, struct tc_action
> *a, int bind)
> struct tcf_hashinfo *hinfo = a->ops->hinfo;
> struct tcf_common *p = NULL;
> if (index && (p = tcf_hash_lookup(index, hinfo)) != NULL) {
> - if (bind)
> + if (bind) {
> p->tcfc_bindcnt++;
> - p->tcfc_refcnt++;
> + p->tcfc_refcnt++;
> + }
> a->priv = p;
> return 1;
> }
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index a42a3b2..2685450 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -98,6 +98,8 @@ static int tcf_mirred_init(struct net *net, struct
> nlattr *nla,
> return ret;
> ret = ACT_P_CREATED;
> } else {
> + if (bind)
> + return 0;
> if (!ovr) {
> tcf_hash_release(a, bind);
> return -EEXIST;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix refcount imbalance in actions
2015-07-30 0:55 ` Daniel Borkmann
@ 2015-07-30 18:48 ` Cong Wang
2015-07-30 20:01 ` Daniel Borkmann
0 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2015-07-30 18:48 UTC (permalink / raw)
To: Daniel Borkmann
Cc: David Miller, Alexei Starovoitov, Jamal Hadi Salim, netdev
On Wed, Jul 29, 2015 at 5:55 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Hm, so this seems not correct: if we only ever increase tcfc_refcnt
> when there's bind=1, and only ever decrease when bind=1, then we
> will never free the action as we do start out from ref=1 in case
> it has been added without initial binding, if I see this correctly.
>
Right, I think your patch should be fine for net. The code is kinda messy,
but we can always clean up the logic for net-next.
Reviewed-by: Cong Wang <cwang@twopensource.com>
(It looks like mirred doesn't handle bind == true case correctly, I will send a
separated for it after your patch.)
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix refcount imbalance in actions
2015-07-30 18:48 ` Cong Wang
@ 2015-07-30 20:01 ` Daniel Borkmann
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2015-07-30 20:01 UTC (permalink / raw)
To: Cong Wang; +Cc: David Miller, Alexei Starovoitov, Jamal Hadi Salim, netdev
On 07/30/2015 08:48 PM, Cong Wang wrote:
...
> Right, I think your patch should be fine for net. The code is kinda messy,
> but we can always clean up the logic for net-next.
I agree with you. I.e. there could just be a single refcount taking care
of the cleanup/destruction, etc.
> Reviewed-by: Cong Wang <cwang@twopensource.com>
>
> (It looks like mirred doesn't handle bind == true case correctly, I will send a
> separated for it after your patch.)
Okay. Thanks for the review Cong!
Cheers,
Daniel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: sched: fix refcount imbalance in actions
2015-07-29 21:35 [PATCH net] net: sched: fix refcount imbalance in actions Daniel Borkmann
2015-07-30 0:33 ` Cong Wang
@ 2015-07-30 21:21 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2015-07-30 21:21 UTC (permalink / raw)
To: daniel; +Cc: cwang, ast, jhs, netdev
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 29 Jul 2015 23:35:25 +0200
> Since commit 55334a5db5cd ("net_sched: act: refuse to remove bound action
> outside"), we end up with a wrong reference count for a tc action.
...
> 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 55334a5db5cd 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: 55334a5db5cd ("net_sched: act: refuse to remove bound action outside")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Applied and queued up for -stable, thanks Daniel.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-30 21:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-29 21:35 [PATCH net] net: sched: fix refcount imbalance in actions Daniel Borkmann
2015-07-30 0:33 ` Cong Wang
2015-07-30 0:55 ` Daniel Borkmann
2015-07-30 18:48 ` Cong Wang
2015-07-30 20:01 ` Daniel Borkmann
2015-07-30 21:21 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.