[net] net: sched: fix refcount imbalance in actions

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.

                        return -EEXIST;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html