netdev - Re: [PATCH net-next v4] vxlan: add support for flowlabel inherit

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] [day] [month] [year] [list]

Message-ID: <ZS0jywD/ktdhcdFj@shredder>
Date: Mon, 16 Oct 2023 14:51:39 +0300
From: Ido Schimmel <idosch@...dia.com>
To: Alce Lafranque <alce@...ranque.net>
Cc: "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
	Vincent Bernat <vincent@...nat.ch>
Subject: Re: [PATCH net-next v4] vxlan: add support for flowlabel inherit

On Sat, Oct 14, 2023 at 08:21:02AM -0500, Alce Lafranque wrote:
> By default, VXLAN encapsulation over IPv6 sets the flow label to 0, with
> an option for a fixed value. This commits add the ability to inherit the
> flow label from the inner packet, like for other tunnel implementations.
> This enables devices using only L3 headers for ECMP to correctly balance
> VXLAN-encapsulated IPv6 packets.
> 
> ```
> $ ./ip/ip link add dummy1 type dummy
> $ ./ip/ip addr add 2001:db8::2/64 dev dummy1
> $ ./ip/ip link set up dev dummy1
> $ ./ip/ip link add vxlan1 type vxlan id 100 flowlabel inherit remote 2001:db8::1 local 2001:db8::2
> $ ./ip/ip link set up dev vxlan1
> $ ./ip/ip addr add 2001:db8:1::2/64 dev vxlan1
> $ ./ip/ip link set arp off dev vxlan1
> $ ping -q 2001:db8:1::1 &
> $ tshark -d udp.port==8472,vxlan -Vpni dummy1 -c1
> [...]
> Internet Protocol Version 6, Src: 2001:db8::2, Dst: 2001:db8::1
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... 1011 0001 1010 1111 1011 = Flow Label: 0xb1afb
> [...]
> Virtual eXtensible Local Area Network
>     Flags: 0x0800, VXLAN Network ID (VNI)
>     Group Policy ID: 0
>     VXLAN Network Identifier (VNI): 100
> [...]
> Internet Protocol Version 6, Src: 2001:db8:1::2, Dst: 2001:db8:1::1
>     0110 .... = Version: 6
>     .... 0000 0000 .... .... .... .... .... = Traffic Class: 0x00 (DSCP: CS0, ECN: Not-ECT)
>         .... 0000 00.. .... .... .... .... .... = Differentiated Services Codepoint: Default (0)
>         .... .... ..00 .... .... .... .... .... = Explicit Congestion Notification: Not ECN-Capable Transport (0)
>     .... 1011 0001 1010 1111 1011 = Flow Label: 0xb1afb
> ```
> 
> Signed-off-by: Alce Lafranque <alce@...ranque.net>
> Co-developed-by: Vincent Bernat <vincent@...nat.ch>
> Signed-off-by: Vincent Bernat <vincent@...nat.ch>
> 
> ---
> v4:
>   - Fix tabs
> v3: https://lore.kernel.org/all/20231014131320.51810-1-alce@lafranque.net/
>   - Adopt policy label inherit by default

I don't think it's valid to change the default behavior. I checked and
the "inherit" policy isn't even the default in ip6gre where unlike vxlan
the inner flow information isn't already encoded in the outer headers.

>   - Set policy to label fixed when flowlabel is set

I assume this was added because of the previous change and can be
removed. I thought about what you said earlier that old kernels don't
reject IFLA_VXLAN_LABEL_POLICY so iproute2 can send it when setting the
flow label to a fixed number. If iproute2 maintainers aren't OK with it
we might need to add a new keyword for the flow label policy instead of
folding it into the "flowlabel" keyword.

>   - Rename IFLA_VXLAN_LABEL_BEHAVIOR to IFLA_VXLAN_LABEL_POLICY
> v2: https://lore.kernel.org/all/20231007142624.739192-1-alce@lafranque.net/
>   - Use an enum instead of flag to define label behavior
> v1: https://lore.kernel.org/all/4444C5AE-FA5A-49A4-9700-7DD9D7916C0F.1@mail.lac-coloc.fr/

[...]

>  static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
> @@ -3653,13 +3664,18 @@ static int vxlan_config_validate(struct net *src_net, struct vxlan_config *conf,
>  			}
>  		}
>  	}
> -

Seems like an unrelated change.

>  	if (conf->label && !use_ipv6) {
>  		NL_SET_ERR_MSG(extack,
>  			       "Label attribute only applies to IPv6 VXLAN devices");
>  		return -EINVAL;
>  	}
>  
> +	if (conf->label_policy && !use_ipv6) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Label policy only applies to IPv6 VXLAN devices");
> +		return -EINVAL;
> +	}
> +
>  	if (conf->remote_ifindex) {
>  		struct net_device *lowerdev;
>  

The rest looks fine to me.

Thanks