netdev - Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func

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: <CAJieiUiONZiO0vyDzJpAiQK-hg2nU93qCvx0Tcgvbz=7e=e1_w@mail.gmail.com>
Date:   Thu, 29 Nov 2018 07:27:17 -0800
From:   Roopa Prabhu <roopa@...ulusnetworks.com>
To:     sd@...asysnail.net
Cc:     David Miller <davem@...emloft.net>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/3] vxlan: move flag sets to use a helper func

On Thu, Nov 29, 2018 at 6:19 AM Sabrina Dubroca <sd@...asysnail.net> wrote:
>
> 2018-11-28, 14:27:59 -0800, Roopa Prabhu wrote:
> > +/* Set/clear flags based on attribute */
> > +static void vxlan_nl2flag(struct vxlan_config *conf, struct nlattr *tb[],
> > +                       int attrtype, unsigned long mask)
> > +{
> > +     unsigned long flags;
> > +
> > +     if (!tb[attrtype])
> > +             return;
> > +
> > +     if (nla_get_u8(tb[attrtype]))
> > +             flags = conf->flags | mask;
> > +     else
> > +             flags = conf->flags & ~mask;
> > +
> > +     conf->flags = flags;
> > +}
> > +
> >  static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> >                        struct net_device *dev, struct vxlan_config *conf,
> >                        bool changelink, struct netlink_ext_ack *extack)
> > @@ -3458,45 +3475,27 @@ static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
> >       if (data[IFLA_VXLAN_TTL])
> >               conf->ttl = nla_get_u8(data[IFLA_VXLAN_TTL]);
> >
> > -     if (data[IFLA_VXLAN_TTL_INHERIT])
> > -             conf->flags |= VXLAN_F_TTL_INHERIT;
> > +     vxlan_nl2flag(conf, data, IFLA_VXLAN_TTL_INHERIT,
> > +                   VXLAN_F_TTL_INHERIT);
>
> IFLA_VXLAN_TTL_INHERIT is a NLA_FLAG. It doesn't have any u8 data to
> get. Same for:
>
>         [IFLA_VXLAN_GBP]        = { .type = NLA_FLAG, },
>         [IFLA_VXLAN_GPE]        = { .type = NLA_FLAG, },
>         [IFLA_VXLAN_REMCSUM_NOPARTIAL]  = { .type = NLA_FLAG },
>         [IFLA_VXLAN_TTL_INHERIT]        = { .type = NLA_FLAG },
>
>

good catch. i see it, some flags are NLA_U8 and some NLA_FLAG. will fix.


>
> nit: This patch would have been easier to review if it came first in
> the series. Converting:

I considered that. But the first patch really removes some checks
making it easier for the follow-on patches...and that mostly dictated
the order.

>
>     if (nla_get_u8(data[IFLA_VXLAN_FOO]))
>         conf->flags |= VXLAN_F_FOO;
>
> to this new helper, which in effect does
>
>     if (nla_get_u8(data[IFLA_VXLAN_FOO]))
>         conf->flags |= VXLAN_F_FOO;
>     else
>         conf->flags &= ~VXLAN_F_FOO;
>
> seems to change behavior, but since you're (unless I missed one) only
> applying it to flags that couldn't be changed before patch 1, it looks
> fine.

It does not change behavior...the earlier code only supported the
flags during create time and did not support changing of the flag with
changelink.
this patch adds changelink support. But if you do see any change in
behavior, pls report. thanks.