netdev - Re: [PATCH iproute2] vxlan: add support for flowlab 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZbZJ-IS20fe8wmQv@shredder>
Date: Sun, 28 Jan 2024 14:35:04 +0200
From: Ido Schimmel <idosch@...sch.org>
To: David Ahern <dsahern@...il.com>
Cc: Vincent Bernat <vincent@...nat.ch>, Alce Lafranque <alce@...ranque.net>,
	netdev@...r.kernel.org, stephen@...workplumber.org
Subject: Re: [PATCH iproute2] vxlan: add support for flowlab inherit

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;