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

[Ido Schimmel <idosch@nvidia.com>]

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@lafranque.net>
> Co-developed-by: Vincent Bernat <vincent@bernat.ch>
> Signed-off-by: Vincent Bernat <vincent@bernat.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