netdev - Re: [PATCH net-next v2 1/3] vxlan: support changelink for a few more attributes

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]

Date:   Thu, 29 Nov 2018 07:33:11 -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 1/3] vxlan: support changelink for a few more attributes

On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca <sd@...asysnail.net> wrote:
>
> 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> > From: Roopa Prabhu <roopa@...ulusnetworks.com>
> >
> > We started very conservative when supporting changelink
> > especially because not all attribute changes could be
> > tested. This patch opens up a few more attributes for
> > changelink. The reason for choosing this set of attributes
> > is based on code references for these attributes. I have
> > tested TTL changes and did some changelink api testing
> > to sanity test the others.
> >
> > Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
> > ---
> >  drivers/net/vxlan.c | 36 ++++--------------------------------
> >  1 file changed, 4 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> > index 9110662..73caa65 100644
> > --- a/drivers/net/vxlan.c
> > +++ b/drivers/net/vxlan.c
> > @@ -3438,11 +3438,8 @@ 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]) {
> > -             if (changelink)
> > -                     return -EOPNOTSUPP;
> > +     if (data[IFLA_VXLAN_TTL_INHERIT])
> >               conf->flags |= VXLAN_F_TTL_INHERIT;
> > -     }
>
> This doesn't give us an option to disable TTL_INHERIT after it was
> enabled once. Same thing with GBP, GPE, REMCSUM_NOPARTIAL.

that is provided by patch3, with the changelink patch. I can squash
them, if thats easier.


>
> > -     if (data[IFLA_VXLAN_GBP]) {
> > -             if (changelink)
> > -                     return -EOPNOTSUPP;
> > +     if (data[IFLA_VXLAN_GBP])
> >               conf->flags |= VXLAN_F_GBP;
> > -     }
> >
> > -     if (data[IFLA_VXLAN_GPE]) {
> > -             if (changelink)
> > -                     return -EOPNOTSUPP;
> > +     if (data[IFLA_VXLAN_GPE])
> >               conf->flags |= VXLAN_F_GPE;
> > -     }
>
> GPE implies running a different setup function (vxlan_raw_setup() vs
> vxlan_ether_setup()), that vxlan_config_apply() only calls for
> !changelink. I think this is incomplete.
>
> I think we'd also end up with mixed tunnel types (GPE/!GPE) on the
> same socket, I'm not sure how that would work. Normally, they each try
> to create a separate socket, and pass the GPE flag on to the
> associated vxlan_sock. I suspect that's also a problem with rx
> offload.

that is good to know. I will drop the change to GPE and also the rx
offload flag and let somebody else using it
open it up for changelink. thanks for the review


> > -     if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL]) {
> > -             if (changelink)
> > -                     return -EOPNOTSUPP;
> > +     if (data[IFLA_VXLAN_REMCSUM_NOPARTIAL])
> >               conf->flags |= VXLAN_F_REMCSUM_NOPARTIAL;
> > -     }
> >
> >       if (tb[IFLA_MTU]) {
> >               if (changelink)
> > --