netdev - Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpVtk1AowsH_jDz24yNqAZTCOPtMk=KM0dhFtYUJLfOk4Q@mail.gmail.com>
Date:	Sun, 8 May 2016 20:08:41 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	David Miller <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [net-next PATCH v2 1/6] net sched: vlan action fix late binding

On Sun, May 8, 2016 at 10:26 AM, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index c1682ab..352653f 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -77,7 +77,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>         int action;
>         __be16 push_vid = 0;
>         __be16 push_proto = 0;
> -       int ret = 0;
> +       int ret = 0, aexists = 0;
>         int err;
>
>         if (!nla)
> @@ -90,15 +90,25 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>         if (!tb[TCA_VLAN_PARMS])
>                 return -EINVAL;
>         parm = nla_data(tb[TCA_VLAN_PARMS]);
> +       aexists = tcf_hash_check(tn, parm->index, a, bind);


I think 'exists' is a better name than 'aexists', shorter and clear.


> +       if (aexists && bind)
> +               return 0;
> +
>         switch (parm->v_action) {
>         case TCA_VLAN_ACT_POP:
>                 break;
>         case TCA_VLAN_ACT_PUSH:
> -               if (!tb[TCA_VLAN_PUSH_VLAN_ID])
> +               if (!tb[TCA_VLAN_PUSH_VLAN_ID]) {
> +                       if (aexists)
> +                               tcf_hash_release(a, bind);
>                         return -EINVAL;
> +               }
>                 push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
> -               if (push_vid >= VLAN_VID_MASK)
> +               if (push_vid >= VLAN_VID_MASK) {
> +                       if (aexists)
> +                               tcf_hash_release(a, bind);
>                         return -ERANGE;
> +               }
>
>                 if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
>                         push_proto = nla_get_be16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]);
> @@ -114,11 +124,13 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>                 }
>                 break;
>         default:
> +               if (aexists)
> +                       tcf_hash_release(a, bind);


Introduce a goto to reduce duplicated cleanup code?


>                 return -EINVAL;
>         }
>         action = parm->v_action;
>
> -       if (!tcf_hash_check(tn, parm->index, a, bind)) {
> +       if (!aexists) {
>                 ret = tcf_hash_create(tn, parm->index, est, a,
>                                       sizeof(*v), bind, false);
>                 if (ret)
> @@ -126,8 +138,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
>                 ret = ACT_P_CREATED;
>         } else {
> -               if (bind)
> -                       return 0;
>                 tcf_hash_release(a, bind);
>                 if (!ovr)
>                         return -EEXIST;


The rest looks good to me.