[PATCH iproute2] vxlan: add support for flowlab inherit

[PATCH iproute2] vxlan: add support for flowlab inherit

[Alce Lafranque]

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.

In relation to the commit "c6e9dba3be5e" ('vxlan: add support for
                                           flowlabel inherit)

```
$ ./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
```
```
$ ./ip/ip -d l l vxlan1
8: vxlan1: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether 36:2c:83:91:53:9e brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
    vxlan id 100 remote 2001:db8::1 local 2001:db8::2 srcport 0 0 dstport 8472 ttl auto flowlabel inherit ageing 300 [...]
```

```
$ ./ip/ip link set vxlan1 type vxlan flowlabel 10
$ ./ip/ip -d l l vxlan1
8: vxlan1: <BROADCAST,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/ether 36:2c:83:91:53:9e brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535
    vxlan id 100 remote 2001:db8::1 local 2001:db8::2 srcport 0 0 dstport 8472 ttl auto flowlabel 0xa ageing 300 [...]
```

Signed-off-by: Alce Lafranque <alce@lafranque.net>
Co-developed-by: Vincent Bernat <vincent@bernat.ch>
Signed-off-by: Vincent Bernat <vincent@bernat.ch>
---
 ip/iplink_vxlan.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 7781d60b..0b72a545 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -72,7 +72,7 @@ static void print_explain(FILE *f)
 		"	TOS	:= { NUMBER | inherit }\n"
 		"	TTL	:= { 1..255 | auto | inherit }\n"
 		"	DF	:= { unset | set | inherit }\n"
-		"	LABEL := 0-1048575\n"
+		"	LABEL   := { 0-1048575 | inherit }\n"
 	);
 }
 
@@ -214,10 +214,16 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			check_duparg(&attrs, IFLA_VXLAN_LABEL, "flowlabel",
 				     *argv);
-			if (get_u32(&uval, *argv, 0) ||
-			    (uval & ~LABEL_MAX_MASK))
-				invarg("invalid flowlabel", *argv);
-			addattr32(n, 1024, IFLA_VXLAN_LABEL, htonl(uval));
+			if (strcmp(*argv, "inherit") == 0) {
+				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_INHERIT);
+			} else {
+				if (get_u32(&uval, *argv, 0) ||
+				    (uval & ~LABEL_MAX_MASK))
+					invarg("invalid flowlabel", *argv);
+				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_FIXED);
+				addattr32(n, 1024, IFLA_VXLAN_LABEL,
+					  htonl(uval));
+			}
 		} else if (!matches(*argv, "ageing")) {
 			__u32 age;
 
@@ -580,12 +586,25 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			print_string(PRINT_ANY, "df", "df %s ", "inherit");
 	}
 
-	if (tb[IFLA_VXLAN_LABEL]) {
-		__u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
-
-		if (label)
-			print_0xhex(PRINT_ANY, "label",
-				    "flowlabel %#llx ", ntohl(label));
+	enum ifla_vxlan_label_policy policy = VXLAN_LABEL_FIXED;
+	if (tb[IFLA_VXLAN_LABEL_POLICY]) {
+		policy = rta_getattr_u32(tb[IFLA_VXLAN_LABEL_POLICY]);
+	}
+	switch (policy) {
+	case VXLAN_LABEL_FIXED:
+		if (tb[IFLA_VXLAN_LABEL]) {
+			__u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
+
+			if (label)
+				print_0xhex(PRINT_ANY, "label",
+					    "flowlabel %#llx ", ntohl(label));
+		}
+		break;
+	case VXLAN_LABEL_INHERIT:
+		print_string(PRINT_FP, NULL, "flowlabel %s ", "inherit");
+		break;
+	default:
+		break;
 	}
 
 	if (tb[IFLA_VXLAN_AGEING]) {
-- 
2.39.2

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Ido Schimmel]

s/flowlab/flowlabel/ in subject

My understanding is that new features should be targeted at
iproute2-next. See the README.

On Sat, Jan 20, 2024 at 06:44:18AM -0600, Alce Lafranque wrote:
> @@ -214,10 +214,16 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>  			NEXT_ARG();
>  			check_duparg(&attrs, IFLA_VXLAN_LABEL, "flowlabel",
>  				     *argv);
> -			if (get_u32(&uval, *argv, 0) ||
> -			    (uval & ~LABEL_MAX_MASK))
> -				invarg("invalid flowlabel", *argv);
> -			addattr32(n, 1024, IFLA_VXLAN_LABEL, htonl(uval));
> +			if (strcmp(*argv, "inherit") == 0) {
> +				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_INHERIT);
> +			} else {
> +				if (get_u32(&uval, *argv, 0) ||
> +				    (uval & ~LABEL_MAX_MASK))
> +					invarg("invalid flowlabel", *argv);
> +				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_FIXED);

I think I mentioned this during the review of the kernel patch, but the
current approach relies on old kernels ignoring the
'IFLA_VXLAN_LABEL_POLICY' attribute which is not nice. My personal
preference would be to add a new keyword for the new attribute:

# ip link set dev vx0 type vxlan flowlabel_policy inherit
# ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10

But let's see what David thinks.

> +				addattr32(n, 1024, IFLA_VXLAN_LABEL,
> +					  htonl(uval));
> +			}
>  		} else if (!matches(*argv, "ageing")) {
>  			__u32 age;
>  
> @@ -580,12 +586,25 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>  			print_string(PRINT_ANY, "df", "df %s ", "inherit");
>  	}
>  
> -	if (tb[IFLA_VXLAN_LABEL]) {
> -		__u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
> -
> -		if (label)
> -			print_0xhex(PRINT_ANY, "label",
> -				    "flowlabel %#llx ", ntohl(label));
> +	enum ifla_vxlan_label_policy policy = VXLAN_LABEL_FIXED;
> +	if (tb[IFLA_VXLAN_LABEL_POLICY]) {
> +		policy = rta_getattr_u32(tb[IFLA_VXLAN_LABEL_POLICY]);
> +	}

Checkpatch says:

WARNING: Missing a blank line after declarations
#112: FILE: ip/iplink_vxlan.c:590:
+       enum ifla_vxlan_label_policy policy = VXLAN_LABEL_FIXED;
+       if (tb[IFLA_VXLAN_LABEL_POLICY]) {

WARNING: braces {} are not necessary for single statement blocks
#112: FILE: ip/iplink_vxlan.c:590:
+       if (tb[IFLA_VXLAN_LABEL_POLICY]) {
+               policy = rta_getattr_u32(tb[IFLA_VXLAN_LABEL_POLICY]);
+       }

> +	switch (policy) {
> +	case VXLAN_LABEL_FIXED:
> +		if (tb[IFLA_VXLAN_LABEL]) {
> +			__u32 label = rta_getattr_u32(tb[IFLA_VXLAN_LABEL]);
> +
> +			if (label)
> +				print_0xhex(PRINT_ANY, "label",
> +					    "flowlabel %#llx ", ntohl(label));
> +		}
> +		break;
> +	case VXLAN_LABEL_INHERIT:
> +		print_string(PRINT_FP, NULL, "flowlabel %s ", "inherit");
> +		break;
> +	default:
> +		break;
>  	}
>  
>  	if (tb[IFLA_VXLAN_AGEING]) {
> -- 
> 2.39.2
> 
> 

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Vincent Bernat]

On 2024-01-22 13:24, Ido Schimmel wrote:
> s/flowlab/flowlabel/ in subject
> 
> My understanding is that new features should be targeted at
> iproute2-next. See the README.

You may be more familiar than I am about this, but since the kernel part 
is already in net, it should go to the stable branch of iproute2.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Stephen Hemminger]

On Mon, 22 Jan 2024 22:11:32 +0100
Vincent Bernat <vincent@bernat.ch> wrote:

> On 2024-01-22 13:24, Ido Schimmel wrote:
> > s/flowlab/flowlabel/ in subject
> > 
> > My understanding is that new features should be targeted at
> > iproute2-next. See the README.  
> 
> You may be more familiar than I am about this, but since the kernel part 
> is already in net, it should go to the stable branch of iproute2.

There is no stable branch. Only current (based of Linus tree)
and next (for net-next kernel).

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/22/24 5:10 PM, Stephen Hemminger wrote:
> On Mon, 22 Jan 2024 22:11:32 +0100
> Vincent Bernat <vincent@bernat.ch> wrote:
> 
>> On 2024-01-22 13:24, Ido Schimmel wrote:
>>> s/flowlab/flowlabel/ in subject
>>>
>>> My understanding is that new features should be targeted at
>>> iproute2-next. See the README.  
>>
>> You may be more familiar than I am about this, but since the kernel part 
>> is already in net, it should go to the stable branch of iproute2.
> 
> There is no stable branch. Only current (based of Linus tree)
> and next (for net-next kernel).

to expand: iproute2 follows the netdev model which means -main is for
bug fixes, and -next is for new features.

If a new kernel feature needs support in iproute2, then the patch to
iproute2 needs to be sent during the same development window as the
kernel support. New feature support will not be added to -main branch
regardless of when the kernel side landed.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/22/24 5:24 AM, Ido Schimmel wrote:
> s/flowlab/flowlabel/ in subject
> 
> My understanding is that new features should be targeted at
> iproute2-next. See the README.
> 
> On Sat, Jan 20, 2024 at 06:44:18AM -0600, Alce Lafranque wrote:
>> @@ -214,10 +214,16 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
>>  			NEXT_ARG();
>>  			check_duparg(&attrs, IFLA_VXLAN_LABEL, "flowlabel",
>>  				     *argv);
>> -			if (get_u32(&uval, *argv, 0) ||
>> -			    (uval & ~LABEL_MAX_MASK))
>> -				invarg("invalid flowlabel", *argv);
>> -			addattr32(n, 1024, IFLA_VXLAN_LABEL, htonl(uval));
>> +			if (strcmp(*argv, "inherit") == 0) {
>> +				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_INHERIT);
>> +			} else {
>> +				if (get_u32(&uval, *argv, 0) ||
>> +				    (uval & ~LABEL_MAX_MASK))
>> +					invarg("invalid flowlabel", *argv);
>> +				addattr32(n, 1024, IFLA_VXLAN_LABEL_POLICY, VXLAN_LABEL_FIXED);
> 
> I think I mentioned this during the review of the kernel patch, but the
> current approach relies on old kernels ignoring the
> 'IFLA_VXLAN_LABEL_POLICY' attribute which is not nice. 

Common theme with vxlan attributes :-(


> My personal
> preference would be to add a new keyword for the new attribute:
> 
> # ip link set dev vx0 type vxlan flowlabel_policy inherit
> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
> 
> But let's see what David thinks.
> 


A new keyword for the new attribute seems like the most robust.

That said, inherit is already used in several ip commands for dscp, ttl
and flowlabel for example; I do not see a separate keyword - e.g.,
ip6tunnel.c.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Vincent Bernat]

On 2024-01-23 01:41, David Ahern wrote:
>> My personal
>> preference would be to add a new keyword for the new attribute:
>>
>> # ip link set dev vx0 type vxlan flowlabel_policy inherit
>> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
>>
>> But let's see what David thinks.
>>
> 
> A new keyword for the new attribute seems like the most robust.
> 
> That said, inherit is already used in several ip commands for dscp, ttl
> and flowlabel for example; I do not see a separate keyword - e.g.,
> ip6tunnel.c.

The implementation for flowlabel was modeled along ttl. We did diverge 
for kernel, we can diverge for iproute2 as well. However, I am unsure if 
you say we should go for option A (new attribute) or option B (do like 
for dscp/ttl).

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/23/24 12:58 AM, Vincent Bernat wrote:
> On 2024-01-23 01:41, David Ahern wrote:
>>> My personal
>>> preference would be to add a new keyword for the new attribute:
>>>
>>> # ip link set dev vx0 type vxlan flowlabel_policy inherit
>>> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
>>>
>>> But let's see what David thinks.
>>>
>>
>> A new keyword for the new attribute seems like the most robust.
>>
>> That said, inherit is already used in several ip commands for dscp, ttl
>> and flowlabel for example; I do not see a separate keyword - e.g.,
>> ip6tunnel.c.
> 
> The implementation for flowlabel was modeled along ttl. We did diverge
> for kernel, we can diverge for iproute2 as well. However, I am unsure if
> you say we should go for option A (new attribute) or option B (do like
> for dscp/ttl).

A divergent kernel API does not mean the command line for iproute2 needs
to be divergent. Consistent syntax across ip commands is best from a
user perspective. What are the downsides to making 'inherit' for
flowlabel work for vxlan like it does for ip6tunnel, ip6tnl and gre6?
Presumably inherit is relevant for geneve? (We really need to stop
making these tweaks on a single protocol basis.)

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Vincent Bernat]

On 2024-01-23 17:19, David Ahern wrote:

>>>> My personal
>>>> preference would be to add a new keyword for the new attribute:
>>>>
>>>> # ip link set dev vx0 type vxlan flowlabel_policy inherit
>>>> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
>>>>
>>>> But let's see what David thinks.
>>>>
>>>
>>> A new keyword for the new attribute seems like the most robust.
>>>
>>> That said, inherit is already used in several ip commands for dscp, ttl
>>> and flowlabel for example; I do not see a separate keyword - e.g.,
>>> ip6tunnel.c.
>>
>> The implementation for flowlabel was modeled along ttl. We did diverge
>> for kernel, we can diverge for iproute2 as well. However, I am unsure if
>> you say we should go for option A (new attribute) or option B (do like
>> for dscp/ttl).
> 
> A divergent kernel API does not mean the command line for iproute2 needs
> to be divergent. Consistent syntax across ip commands is best from a
> user perspective. What are the downsides to making 'inherit' for
> flowlabel work for vxlan like it does for ip6tunnel, ip6tnl and gre6?
> Presumably inherit is relevant for geneve? (We really need to stop
> making these tweaks on a single protocol basis.)

Currently, the patch implements "inherit" without a new keyword, like 
this is done for the other protocols. I don't really see a downside, 
except the kernel could one day implement a policy that may be difficult 
to express this way (inherit-during-the-day-fixed-during-the-night).

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/24/24 3:00 PM, Vincent Bernat wrote:
> On 2024-01-23 17:19, David Ahern wrote:
> 
>>>>> My personal
>>>>> preference would be to add a new keyword for the new attribute:
>>>>>
>>>>> # ip link set dev vx0 type vxlan flowlabel_policy inherit
>>>>> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
>>>>>
>>>>> But let's see what David thinks.
>>>>>
>>>>
>>>> A new keyword for the new attribute seems like the most robust.
>>>>
>>>> That said, inherit is already used in several ip commands for dscp, ttl
>>>> and flowlabel for example; I do not see a separate keyword - e.g.,
>>>> ip6tunnel.c.
>>>
>>> The implementation for flowlabel was modeled along ttl. We did diverge
>>> for kernel, we can diverge for iproute2 as well. However, I am unsure if
>>> you say we should go for option A (new attribute) or option B (do like
>>> for dscp/ttl).
>>
>> A divergent kernel API does not mean the command line for iproute2 needs
>> to be divergent. Consistent syntax across ip commands is best from a
>> user perspective. What are the downsides to making 'inherit' for
>> flowlabel work for vxlan like it does for ip6tunnel, ip6tnl and gre6?
>> Presumably inherit is relevant for geneve? (We really need to stop
>> making these tweaks on a single protocol basis.)
> 
> Currently, the patch implements "inherit" without a new keyword, like
> this is done for the other protocols. I don't really see a downside,
> except the kernel could one day implement a policy that may be difficult
> to express this way (inherit-during-the-day-fixed-during-the-night).

Wouldn't other uses of inherit be subject to the same kind of problem?
ie., my primary point is for consistency in behavior across commands.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Vincent Bernat]

On 2024-01-25 16:50, David Ahern wrote:

>>>>>> My personal
>>>>>> preference would be to add a new keyword for the new attribute:
>>>>>>
>>>>>> # ip link set dev vx0 type vxlan flowlabel_policy inherit
>>>>>> # ip link set dev vx0 type vxlan flowlabel_policy fixed flowlabel 10
>>>>>>
>>>>>> But let's see what David thinks.
>>>>>>
>>>>>
>>>>> A new keyword for the new attribute seems like the most robust.
>>>>>
>>>>> That said, inherit is already used in several ip commands for dscp, ttl
>>>>> and flowlabel for example; I do not see a separate keyword - e.g.,
>>>>> ip6tunnel.c.
>>>>
>>>> The implementation for flowlabel was modeled along ttl. We did diverge
>>>> for kernel, we can diverge for iproute2 as well. However, I am unsure if
>>>> you say we should go for option A (new attribute) or option B (do like
>>>> for dscp/ttl).
>>>
>>> A divergent kernel API does not mean the command line for iproute2 needs
>>> to be divergent. Consistent syntax across ip commands is best from a
>>> user perspective. What are the downsides to making 'inherit' for
>>> flowlabel work for vxlan like it does for ip6tunnel, ip6tnl and gre6?
>>> Presumably inherit is relevant for geneve? (We really need to stop
>>> making these tweaks on a single protocol basis.)
>>
>> Currently, the patch implements "inherit" without a new keyword, like
>> this is done for the other protocols. I don't really see a downside,
>> except the kernel could one day implement a policy that may be difficult
>> to express this way (inherit-during-the-day-fixed-during-the-night).
> 
> Wouldn't other uses of inherit be subject to the same kind of problem?
> ie., my primary point is for consistency in behavior across commands.

Honestly, I have a hard time finding a real downside. The day we need to 
specify both a value and a policy, it will still be time to introduce a 
new keyword. For now, it seems better to be consistent with the other 
protocols and with the other keywords (ttl, for example) using the same 
approach.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/25/24 11:28 PM, Vincent Bernat wrote:
> Honestly, I have a hard time finding a real downside. The day we need to
> specify both a value and a policy, it will still be time to introduce a
> new keyword. For now, it seems better to be consistent with the other
> protocols and with the other keywords (ttl, for example) using the same
> approach.

ok. let's move forward without the new keyword with the understanding it
is not perfect, but at least consistent across commands should a problem
arise. Consistency allows simpler workarounds.

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Ido Schimmel]

On Fri, Jan 26, 2024 at 10:17:36AM -0700, David Ahern wrote:
> On 1/25/24 11:28 PM, Vincent Bernat wrote:
> > Honestly, I have a hard time finding a real downside. The day we need to
> > specify both a value and a policy, it will still be time to introduce a
> > new keyword. For now, it seems better to be consistent with the other
> > protocols and with the other keywords (ttl, for example) using the same
> > approach.
> 
> ok. let's move forward without the new keyword with the understanding it
> is not perfect, but at least consistent across commands should a problem
> arise. Consistency allows simpler workarounds.

I find it weird that the argument for the current approach is
consistency when the commands are already inconsistent:

1. VXLAN flow label can be specified either in decimal or hex, but the
expectation is decimal according to the help message. ip6gre flow label
can only be configured as hex:

# ip link help vxlan
[...]
        LABEL := 0-1048575

# ip link help ip6gre
[...]
        FLOWLABEL := { 0x0..0xfffff | inherit }

# ip link add name vx0 up type vxlan vni 10010 local 2001:db8:1::1 flowlabel 100 dstport 4789
# ip -d -j -p link show dev vx0 | jq '.[]["linkinfo"]["info_data"]["label"]'
"0x64"

# ip link add name g6 up type ip6gre local 2001:db8:1::1 flowlabel 100
# ip -d -j -p link show dev g6 | jq '.[]["linkinfo"]["info_data"]["flowlabel"]'
"0x00100"

2. The keywords in the JSON output are different as you can see above.
The format of the value is also different.

3. Value of 0 is not printed for VXLAN, but is printed for ip6gre:

# ip link add name vx0 up type vxlan vni 10010 local 2001:db8:1::1 flowlabel 0 dstport 4789
# ip -d -j -p link show dev vx0 | jq '.[]["linkinfo"]["info_data"]["label"]'
null

# ip link add name g6 up type ip6gre local 2001:db8:1::1 flowlabel 0
# ip -d -j -p link show dev g6 | jq '.[]["linkinfo"]["info_data"]["flowlabel"]'
"0x00000"

If you do decide to move forward with the current approach, then at
least the JSON output needs to be modified to print something when
"inherit" is set. With current patch:

# ip link add name vx0 up type vxlan vni 10010 local 2001:db8:1::1 flowlabel inherit dstport 4789
# ip -d -j -p link show dev vx0 | jq '.[]["linkinfo"]["info_data"]["label"]'
null

I would also try to avoid sending the new 'IFLA_VXLAN_LABEL_POLICY' attribute
for existing use cases: When creating a VXLAN device with a fixed flow label or
when simply modifying an already fixed flow label. I would expect kernels
6.5-6.7 to reject the new attribute as since kernel 6.5 the VXLAN driver
enforces strict validation. However, it's not the case:

# uname -r
6.7.0-custom
# ip link help vxlan | grep LABEL | grep inherit
        LABEL   := { 0-1048575 | inherit }
# ip link add name vx0 up type vxlan vni 10010 local 2001:db8:1::1 flowlabel 100 dstport 4789
# echo $?
0

It seems to be an oversight in kernel commit 56738f460841 ("netlink: add strict
parsing for future attributes") which states "Also, for new attributes, we need
not accept them when the policy doesn't declare their usage". I do get a
failure with the following diff [1] (there's probably a nicer way):

# uname -r
6.7.0-custom-dirty
# ip link help vxlan | grep LABEL | grep inherit
        LABEL   := { 0-1048575 | inherit }
# ip link add name vx0 up type vxlan vni 10010 local 2001:db8:1::1 flowlabel 100 dstport 4789
Error: Unknown attribute type.

Regarding the comment about the
"inherit-during-the-day-fixed-during-the-night" policy, I'm familiar
with at least one hardware implementation that supports a policy of
"inherit flow label when IPv6, otherwise set flow label to X" and it
indeed won't be possible to express it with the single keyword approach.

[1]
diff --git a/lib/nlattr.c b/lib/nlattr.c
index dc15e7888fc1..8da3be8a76dd 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -619,7 +619,9 @@ static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype,
                u16 type = nla_type(nla);
 
                if (type == 0 || type > maxtype) {
-                       if (validate & NL_VALIDATE_MAXTYPE) {
+                       if ((validate & NL_VALIDATE_MAXTYPE) ||
+                           (policy && policy[0].strict_start_type &&
+                            type >= policy[0].strict_start_type)) {
                                NL_SET_ERR_MSG_ATTR(extack, nla,
                                                    "Unknown attribute type");
                                return -EINVAL;

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[Vincent Bernat]

On 2024-01-28 13:35, Ido Schimmel wrote:
> On Fri, Jan 26, 2024 at 10:17:36AM -0700, David Ahern wrote:
>> On 1/25/24 11:28 PM, Vincent Bernat wrote:
>>> Honestly, I have a hard time finding a real downside. The day we need to
>>> specify both a value and a policy, it will still be time to introduce a
>>> new keyword. For now, it seems better to be consistent with the other
>>> protocols and with the other keywords (ttl, for example) using the same
>>> approach.
>>
>> ok. let's move forward without the new keyword with the understanding it
>> is not perfect, but at least consistent across commands should a problem
>> arise. Consistency allows simpler workarounds.
> 
> I find it weird that the argument for the current approach is
> consistency when the commands are already inconsistent:
[...]

It's still more consistent than adding a keyword. But we are OK to add a 
keyword if needed. It seems there is no agreement yet on this. David, do 
you prefer a keyword?

> I would also try to avoid sending the new 'IFLA_VXLAN_LABEL_POLICY' attribute
> for existing use cases: When creating a VXLAN device with a fixed flow label or
> when simply modifying an already fixed flow label. I would expect kernels
> 6.5-6.7 to reject the new attribute as since kernel 6.5 the VXLAN driver
> enforces strict validation. However, it's not the case:
[...]

This should be solved if there is a new keyword. If we do not introduce 
a new keyword, this means querying the current state before setting the 
new state as we need to know what the previous policy was.

> Regarding the comment about the
> "inherit-during-the-day-fixed-during-the-night" policy, I'm familiar
> with at least one hardware implementation that supports a policy of
> "inherit flow label when IPv6, otherwise set flow label to X" and it
> indeed won't be possible to express it with the single keyword approach.

Good example (better than mine).

Re: [PATCH iproute2] vxlan: add support for flowlab inherit

[David Ahern]

On 1/28/24 5:35 AM, Ido Schimmel wrote:
> On Fri, Jan 26, 2024 at 10:17:36AM -0700, David Ahern wrote:
>> On 1/25/24 11:28 PM, Vincent Bernat wrote:
>>> Honestly, I have a hard time finding a real downside. The day we need to
>>> specify both a value and a policy, it will still be time to introduce a
>>> new keyword. For now, it seems better to be consistent with the other
>>> protocols and with the other keywords (ttl, for example) using the same
>>> approach.
>>
>> ok. let's move forward without the new keyword with the understanding it
>> is not perfect, but at least consistent across commands should a problem
>> arise. Consistency allows simpler workarounds.
> 
> I find it weird that the argument for the current approach is
> consistency when the commands are already inconsistent:

I am 5+ years removed from working with tunneling protocols on a regular
basis, and the brain cells holding the details and nuances of vxlan,
geneve, etc command lines have long since been recycled. My attempt here
is to build a consensus on how to move forward by offering some guiding
principles - like the kernel API puts absolutely no constraint on
iproute2 command line syntax and that a package like iproute2 should be
consistent across commands.

This is open source and is best served by voices chiming in with
detailed perspectives. Lacking a decisive reason to choose one option
over another, I will err on the side of consistency. Dealing with subtle
differences in command lines is a real pain to users and it is a shame
that what exists is so radically different.