netdev - Re: [PATCH net-next 2/4] net: sched: add erspan option support to act_tunnel

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: <CADvbK_cuyBZW5D0PnkQrEQaOQVrWbt3DLdaOMvQkRgEh40NfOQ@mail.gmail.com>
Date:   Wed, 20 Nov 2019 13:15:00 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     network dev <netdev@...r.kernel.org>, davem <davem@...emloft.net>,
        Simon Horman <simon.horman@...ronome.com>
Subject: Re: [PATCH net-next 2/4] net: sched: add erspan option support to act_tunnel_key

On Wed, Nov 20, 2019 at 8:27 AM Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>
> On Tue, 19 Nov 2019 17:31:47 +0800, Xin Long wrote:
> > @@ -149,6 +159,49 @@ tunnel_key_copy_vxlan_opt(const struct nlattr *nla, void *dst, int dst_len,
> >       return sizeof(struct vxlan_metadata);
> >  }
> >
> > +static int
> > +tunnel_key_copy_erspan_opt(const struct nlattr *nla, void *dst, int dst_len,
> > +                        struct netlink_ext_ack *extack)
> > +{
> > +     struct nlattr *tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_MAX + 1];
> > +     int err;
> > +
> > +     err = nla_parse_nested(tb, TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_MAX, nla,
> > +                            erspan_opt_policy, extack);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     if (!tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_VER]) {
> > +             NL_SET_ERR_MSG(extack, "Missing tunnel key erspan option ver");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (dst) {
> > +             struct erspan_metadata *md = dst;
> > +
> > +             nla = tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_VER];
> > +             md->version = nla_get_u8(nla);
> > +
> > +             if (md->version == 1 &&
> > +                 tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_INDEX]) {
> > +                     nla = tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_INDEX];
> > +                     md->u.index = nla_get_be32(nla);
> > +             } else if (md->version == 2 &&
> > +                        tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_DIR] &&
> > +                        tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_HWID]) {
> > +                     nla = tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_DIR];
> > +                     md->u.md2.dir = nla_get_u8(nla);
> > +                     nla = tb[TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_HWID];
> > +                     set_hwid(&md->u.md2, nla_get_u8(nla));
> > +             } else {
> > +                     NL_SET_ERR_MSG(extack, "erspan ver is incorrect or some option is missed");
>
> I think s/missed/missing/
ah right.

>
> But I think it'd be better if the validation was done also when dst is
> not yet allocated. I don't think it matters today, just think it'd be
> cleaner.
sure, I can improve in that way.

>
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     return sizeof(struct erspan_metadata);
> > +}
> > +
> >  static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
> >                               int dst_len, struct netlink_ext_ack *extack)
> >  {
> > @@ -190,6 +243,18 @@ static int tunnel_key_copy_opts(const struct nlattr *nla, u8 *dst,
> >                       opts_len += opt_len;
> >                       type = TUNNEL_VXLAN_OPT;
> >                       break;
> > +             case TCA_TUNNEL_KEY_ENC_OPTS_ERSPAN:
> > +                     if (type) {
> > +                             NL_SET_ERR_MSG(extack, "Wrong type for erspan options");
>
> Wrong or duplicate, right? If I'm reading this right unlike for Geneve
> opts there can be only one instance of opts for other types.
yes, 'Duplicate' is better,
will change to "Duplicate type for erspan options", as well as for vxlan.

Thanks.

>
> > +                             return -EINVAL;
> > +                     }
> > +                     opt_len = tunnel_key_copy_erspan_opt(attr, dst,
> > +                                                          dst_len, extack);
> > +                     if (opt_len < 0)
> > +                             return opt_len;
> > +                     opts_len += opt_len;
> > +                     type = TUNNEL_ERSPAN_OPT;
> > +                     break;
> >               }
> >       }
> >