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

[Sabrina Dubroca <sd@queasysnail.net>]

2018-11-29, 07:33:11 -0800, Roopa Prabhu wrote:
> On Thu, Nov 29, 2018 at 5:56 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2018-11-28, 14:27:57 -0800, Roopa Prabhu wrote:
> > > From: Roopa Prabhu <roopa@cumulusnetworks.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@cumulusnetworks.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.

Yes, I see it now. Sequential review got me :(
I think you can just add a note in the commit message, "will be fixed
in a further patch introducing the vxlan_nl2flag() helper" or similar.

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

Looking at vxlan_rcv(), there's also a possible issue with the GBP
flag:

	if (vs->flags & VXLAN_F_GBP)
		vxlan_parse_gbp_hdr(&unparsed, skb, vs->flags, md);

I don't know why that flag must match on all tunnels for a socket, but
given that it was the reason for commit ac5132d1a03f ("vxlan: Only
bind to sockets with compatible flags enabled"), I'd leave it alone as
well.

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

-- 
Sabrina