[PATCH net] geneve: allow changing DF behavior after creation

[PATCH net] geneve: allow changing DF behavior after creation

[Sabrina Dubroca]

Currently, trying to change the DF parameter of a geneve device does
nothing:

    # ip -d link show geneve1
    14: geneve1: <snip>
        link/ether <snip>
        geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
    # ip link set geneve1 type geneve id 1 df unset
    # ip -d link show geneve1
    14: geneve1: <snip>
        link/ether <snip>
        geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>

We just need to update the value in geneve_changelink.

Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/geneve.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 75266580b586..4661ef865807 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
 	geneve->collect_md = metadata;
 	geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
 	geneve->ttl_inherit = ttl_inherit;
+	geneve->df = df;
 	geneve_unquiesce(geneve, gs4, gs6);
 
 	return 0;
-- 
2.27.0

Re: [PATCH net] geneve: allow changing DF behavior after creation

[Stefano Brivio]

On Thu, 18 Jun 2020 12:13:22 +0200
Sabrina Dubroca <sd@queasysnail.net> wrote:

> Currently, trying to change the DF parameter of a geneve device does
> nothing:
> 
>     # ip -d link show geneve1
>     14: geneve1: <snip>
>         link/ether <snip>
>         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
>     # ip link set geneve1 type geneve id 1 df unset
>     # ip -d link show geneve1
>     14: geneve1: <snip>
>         link/ether <snip>
>         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> 
> We just need to update the value in geneve_changelink.
> 
> Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  drivers/net/geneve.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 75266580b586..4661ef865807 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
>  	geneve->collect_md = metadata;
>  	geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
>  	geneve->ttl_inherit = ttl_inherit;
> +	geneve->df = df;

I introduced this bug as I didn't notice the asymmetry with VXLAN,
where vxlan_nl2conf() takes care of this for both new links and link
changes.

Here, this block is duplicated in geneve_configure(), which,
somewhat surprisingly given the name, is not called from
geneve_changelink(). Did you consider factoring out (at least) this
block to have it shared?

Either way,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [PATCH net] geneve: allow changing DF behavior after creation

[Sabrina Dubroca]

2020-06-18, 12:26:29 +0200, Stefano Brivio wrote:
> On Thu, 18 Jun 2020 12:13:22 +0200
> Sabrina Dubroca <sd@queasysnail.net> wrote:
> 
> > Currently, trying to change the DF parameter of a geneve device does
> > nothing:
> > 
> >     # ip -d link show geneve1
> >     14: geneve1: <snip>
> >         link/ether <snip>
> >         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> >     # ip link set geneve1 type geneve id 1 df unset
> >     # ip -d link show geneve1
> >     14: geneve1: <snip>
> >         link/ether <snip>
> >         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> > 
> > We just need to update the value in geneve_changelink.
> > 
> > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > ---
> >  drivers/net/geneve.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> > index 75266580b586..4661ef865807 100644
> > --- a/drivers/net/geneve.c
> > +++ b/drivers/net/geneve.c
> > @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
> >  	geneve->collect_md = metadata;
> >  	geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
> >  	geneve->ttl_inherit = ttl_inherit;
> > +	geneve->df = df;
> 
> I introduced this bug as I didn't notice the asymmetry with VXLAN,
> where vxlan_nl2conf() takes care of this for both new links and link
> changes.

Yeah, I didn't notice either :/

> Here, this block is duplicated in geneve_configure(), which,
> somewhat surprisingly given the name, is not called from
> geneve_changelink(). Did you consider factoring out (at least) this
> block to have it shared?

Then I'd have to introduce another lovely function with an absurdly
long argument list. I'd rather clean that up in all of geneve and
introduce something like struct vxlan_config, but it's a bit much for
net. I'll do that once this fix finds its way into net-next.

> 
> Either way,
> 
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

Thanks.

-- 
Sabrina

Re: [PATCH net] geneve: allow changing DF behavior after creation

[Stefano Brivio]

On Thu, 18 Jun 2020 15:03:47 +0200
Sabrina Dubroca <sd@queasysnail.net> wrote:

> 2020-06-18, 12:26:29 +0200, Stefano Brivio wrote:
> > On Thu, 18 Jun 2020 12:13:22 +0200
> > Sabrina Dubroca <sd@queasysnail.net> wrote:
> >   
> > > Currently, trying to change the DF parameter of a geneve device does
> > > nothing:
> > > 
> > >     # ip -d link show geneve1
> > >     14: geneve1: <snip>
> > >         link/ether <snip>
> > >         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> > >     # ip link set geneve1 type geneve id 1 df unset
> > >     # ip -d link show geneve1
> > >     14: geneve1: <snip>
> > >         link/ether <snip>
> > >         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> > > 
> > > We just need to update the value in geneve_changelink.
> > > 
> > > Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> > > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> > > ---
> > >  drivers/net/geneve.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> > > index 75266580b586..4661ef865807 100644
> > > --- a/drivers/net/geneve.c
> > > +++ b/drivers/net/geneve.c
> > > @@ -1649,6 +1649,7 @@ static int geneve_changelink(struct net_device *dev, struct nlattr *tb[],
> > >  	geneve->collect_md = metadata;
> > >  	geneve->use_udp6_rx_checksums = use_udp6_rx_checksums;
> > >  	geneve->ttl_inherit = ttl_inherit;
> > > +	geneve->df = df;  
> > 
> > I introduced this bug as I didn't notice the asymmetry with VXLAN,
> > where vxlan_nl2conf() takes care of this for both new links and link
> > changes.  
> 
> Yeah, I didn't notice either :/
> 
> > Here, this block is duplicated in geneve_configure(), which,
> > somewhat surprisingly given the name, is not called from
> > geneve_changelink(). Did you consider factoring out (at least) this
> > block to have it shared?  
> 
> Then I'd have to introduce another lovely function with an absurdly
> long argument list. I'd rather clean that up in all of geneve and
> introduce something like struct vxlan_config, but it's a bit much for
> net. I'll do that once this fix finds its way into net-next.

Yeah, sure, I didn't mean you should simply copy and paste that
somewhere. Either something like struct vxlan_config used around the
current logic, or perhaps even better, something like vxlan_nl2conf()
does.

I didn't check whether something in GENEVE is so special compared to
VXLAN as to prevent this, though.

-- 
Stefano

Re: [PATCH net] geneve: allow changing DF behavior after creation

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Thu, 18 Jun 2020 12:13:22 +0200

> Currently, trying to change the DF parameter of a geneve device does
> nothing:
> 
>     # ip -d link show geneve1
>     14: geneve1: <snip>
>         link/ether <snip>
>         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
>     # ip link set geneve1 type geneve id 1 df unset
>     # ip -d link show geneve1
>     14: geneve1: <snip>
>         link/ether <snip>
>         geneve id 1 remote 10.0.0.1 ttl auto df set dstport 6081 <snip>
> 
> We just need to update the value in geneve_changelink.
> 
> Fixes: a025fb5f49ad ("geneve: Allow configuration of DF behaviour")
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Applied and queued up for -stable, thanks.