netdev - Re: [PATCH net-next 03/11] vxlan: Allow configuration of DF behaviour

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: <20181107113548.17ec930d@redhat.com>
Date:   Wed, 7 Nov 2018 11:35:48 +0100
From:   Stefano Brivio <sbrivio@...hat.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Xin Long <lucien.xin@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 03/11] vxlan: Allow configuration of DF
 behaviour

Stephen, thanks for reviewing this.

On Tue, 6 Nov 2018 21:00:18 -0800
Stephen Hemminger <stephen@...workplumber.org> wrote:

> On Tue,  6 Nov 2018 22:38:59 +0100
> Stefano Brivio <sbrivio@...hat.com> wrote:
> 
> > 			df = htons(IP_DF);
> >  		}
> >  
> > +		if (!df) {
> > +			if (vxlan->cfg.df == VXLAN_DF_SET) {
> > +				df = htons(IP_DF);  
> 
> I am confused, this looks like this new flag is duplicating the exiting tunnel DF flag.
> (in info->key.tun.flags). Why is another flag needed?

The reason is that for non-lwt tunnels the tunnel key is not used in
VXLAN, and this patch is intended for non-lwt tunnels only, as external
control planes already have a way to set the DF bit.

But now I see that the way I wrote this is misleading: this should
really be in an if (rdst) or if (!info) clause. I'll place this into
the if (!info) block just above. TTL and TOS are handled in a similar
way, using the if (rdst) clause above.

Functionally, it's equivalent, because external control planes will
never set non-default values for vxlan->cfg.df, but still not a good
reason to write it this way.

Similar story for GENEVE, I will place that under if
(!geneve->collect_md) instead.

With a notable difference, though: GENEVE already uses struct
ip_tunnel_key for some of its interface configuration, so I had already
thought about adding a TUNNEL_DONT_FRAGMENT_INHERIT flag that could be
used in tun_flags.

However, I would use the last available bit there, and this wouldn't be
terribly useful: an external control plane already has access to the
inner DF bit, and would most likely decide on its own whether to set DF
or not, by setting that in tun_flags. So I'd rather keep that in struct
geneve_dev.

-- 
Stefano