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

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: <20200618130347.GA2557669@bistromath.localdomain>
Date:   Thu, 18 Jun 2020 15:03:47 +0200
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     Stefano Brivio <sbrivio@...hat.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] geneve: allow changing DF behavior after creation

2020-06-18, 12:26:29 +0200, Stefano Brivio wrote:
> On Thu, 18 Jun 2020 12:13:22 +0200
> Sabrina Dubroca <sd@...asysnail.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@...asysnail.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@...hat.com>

Thanks.

-- 
Sabrina