On Wed, Jul 29, 2015 at 2:35 PM, Daniel Borkmann <dan...@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;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
