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

[Daniel Borkmann <daniel@iogearbox.net>]

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