[PATCH net v2 0/2] gre: Fix regressions in IPv6 link-local address generation.

[PATCH net v2 0/2] gre: Fix regressions in IPv6 link-local address generation.

[Guillaume Nault]

IPv6 link-local address generation has some special cases for GRE
devices. This has led to several regressions in the past, and some of
them are still not fixed. This series fixes the remaining problems,
like the ipv6.conf.<dev>.addr_gen_mode sysctl being ignored and the
router discovery process not being started (see details in patch 1).

To avoid any further regressions, patch 2 adds selftests covering
IPv4 and IPv6 gre/gretap devices with all combinations of currently
supported addr_gen_mode values.

v2: Add Makefile entry for the new selftest (patch 2).

Guillaume Nault (2):
  gre: Fix IPv6 link-local address generation.
  selftests: Add IPv6 link-local address generation tests for GRE
    devices.

 net/ipv6/addrconf.c                           |  15 +-
 tools/testing/selftests/net/Makefile          |   1 +
 .../testing/selftests/net/gre_ipv6_lladdr.sh  | 227 ++++++++++++++++++
 3 files changed, 237 insertions(+), 6 deletions(-)
 create mode 100755 tools/testing/selftests/net/gre_ipv6_lladdr.sh

-- 
2.39.2

[PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

[Guillaume Nault]

Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
devices in most cases and fall back to using add_v4_addrs() only in
case the GRE configuration is incompatible with addrconf_addr_gen().

GRE used to use addrconf_addr_gen() until commit e5dd729460ca
("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
address") restricted this use to gretap devices and created
add_v4_addrs() (borrowed from SIT) for non-Ethernet GRE ones.

The original problem came when commit 9af28511be10 ("addrconf: refuse
isatap eui64 for INADDR_ANY") made __ipv6_isatap_ifid() fail when its
addr parameter was 0. The commit says that this would create an invalid
address, however, I couldn't find any RFC saying that the generated
interface identifier would be wrong. Anyway, since plain gre devices
pass their local tunnel address to __ipv6_isatap_ifid(), that commit
broke their IPv6 link-local address generation when the local address
was unspecified.

Then commit e5dd729460ca ("ip/ip6_gre: use the same logic as SIT
interfaces when computing v6LL address") tried to fix that case by
defining add_v4_addrs() and calling it to generated the IPv6 link-local
address instead of using addrconf_addr_gen() (appart for gretap devices
which would still use the regular addrconf_addr_gen(), since they have
a MAC address).

That broke several use cases because add_v4_addrs() isn't properly
integrated into the rest of IPv6 Neighbor Discovery code. Several of
these shortcomings have been fixed over time, but add_v4_addrs()
remains broken on several aspects. In particular, it doesn't send any
Router Sollicitations, so the SLAAC process doesn't start until the
interface receives a Router Advertisement. Also, add_v4_addrs() mostly
ignores the address generation mode of the interface
(/proc/sys/net/ipv6/conf/*/addr_gen_mode), thus breaking the
IN6_ADDR_GEN_MODE_RANDOM and IN6_ADDR_GEN_MODE_STABLE_PRIVACY cases.

Fix all this by reverting to addrconf_addr_gen() in all cases but the
very specific one that remains incompatible.

Fix the situation by using add_v4_addrs() only in the specific scenario
where normal method would fail. That is, for interfaces that have all
of the following characteristics:

  * transport IP packets directly, not Ethernet (that is, not gretap),
  * run over IPv4,
  * tunnel endpoint is INADDR_ANY (that is, 0),
  * device address generation mode is EUI64.

In all other cases, revert back to the regular addrconf_addr_gen().

Also, remove the special case for ip6gre interfaces in add_v4_addrs(),
since ip6gre devices now always use addrconf_addr_gen() instead.

Fixes: e5dd729460ca ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL address")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 net/ipv6/addrconf.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ac8cc1076536..8b6258819dad 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3209,16 +3209,13 @@ static void add_v4_addrs(struct inet6_dev *idev)
 	struct in6_addr addr;
 	struct net_device *dev;
 	struct net *net = dev_net(idev->dev);
-	int scope, plen, offset = 0;
+	int scope, plen;
 	u32 pflags = 0;
 
 	ASSERT_RTNL();
 
 	memset(&addr, 0, sizeof(struct in6_addr));
-	/* in case of IP6GRE the dev_addr is an IPv6 and therefore we use only the last 4 bytes */
-	if (idev->dev->addr_len == sizeof(struct in6_addr))
-		offset = sizeof(struct in6_addr) - 4;
-	memcpy(&addr.s6_addr32[3], idev->dev->dev_addr + offset, 4);
+	memcpy(&addr.s6_addr32[3], idev->dev->dev_addr, 4);
 
 	if (!(idev->dev->flags & IFF_POINTOPOINT) && idev->dev->type == ARPHRD_SIT) {
 		scope = IPV6_ADDR_COMPATv4;
@@ -3529,7 +3526,13 @@ static void addrconf_gre_config(struct net_device *dev)
 		return;
 	}
 
-	if (dev->type == ARPHRD_ETHER) {
+	/* Generate the IPv6 link-local address using addrconf_addr_gen(),
+	 * unless we have an IPv4 GRE device not bound to an IP address and
+	 * which is in EUI64 mode (as __ipv6_isatap_ifid() would fail in this
+	 * case). Such devices fall back to add_v4_addrs() instead.
+	 */
+	if (!(dev->type == ARPHRD_IPGRE && *(__be32 *)dev->dev_addr == 0 &&
+	      idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64)) {
 		addrconf_addr_gen(idev, true);
 		return;
 	}
-- 
2.39.2

[PATCH net v2 2/2] selftests: Add IPv6 link-local address generation tests for GRE devices.

[Guillaume Nault]

GRE devices have their special code for IPv6 link-local address
generation that has been the source of several regressions in the past.

Add selftest to check that all gre, ip6gre, gretap and ip6gretap get an
IPv6 link-link local address in accordance with the
net.ipv6.conf.<dev>.addr_gen_mode sysctl.

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
v2: Add selftest to Makefile.

 tools/testing/selftests/net/Makefile          |   1 +
 .../testing/selftests/net/gre_ipv6_lladdr.sh  | 227 ++++++++++++++++++
 2 files changed, 228 insertions(+)
 create mode 100755 tools/testing/selftests/net/gre_ipv6_lladdr.sh

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 73ee88d6b043..5916f3b81c39 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -31,6 +31,7 @@ TEST_PROGS += veth.sh
 TEST_PROGS += ioam6.sh
 TEST_PROGS += gro.sh
 TEST_PROGS += gre_gso.sh
+TEST_PROGS += gre_ipv6_lladdr.sh
 TEST_PROGS += cmsg_so_mark.sh
 TEST_PROGS += cmsg_so_priority.sh
 TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
diff --git a/tools/testing/selftests/net/gre_ipv6_lladdr.sh b/tools/testing/selftests/net/gre_ipv6_lladdr.sh
new file mode 100755
index 000000000000..85e40b6df55e
--- /dev/null
+++ b/tools/testing/selftests/net/gre_ipv6_lladdr.sh
@@ -0,0 +1,227 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+ERR=4 # Return 4 by default, which is the SKIP code for kselftest
+PAUSE_ON_FAIL="no"
+
+readonly NS0=$(mktemp -u ns0-XXXXXXXX)
+
+# Exit the script after having removed the network namespaces it created
+#
+# Parameters:
+#
+#   * The list of network namespaces to delete before exiting.
+#
+exit_cleanup()
+{
+	for ns in "$@"; do
+		ip netns delete "${ns}" 2>/dev/null || true
+	done
+
+	if [ "${ERR}" -eq 4 ]; then
+		echo "Error: Setting up the testing environment failed." >&2
+	fi
+
+	exit "${ERR}"
+}
+
+# Create the network namespaces used by the script (NS0)
+#
+create_namespaces()
+{
+	ip netns add "${NS0}" || exit_cleanup
+}
+
+# The trap function handler
+#
+exit_cleanup_all()
+{
+	exit_cleanup "${NS0}"
+}
+
+# Add fake IPv4 and IPv6 networks on the loopback device, to be used as
+# underlay by future GRE devices.
+#
+setup_basenet()
+{
+	ip -netns "${NS0}" link set dev lo up
+	ip -netns "${NS0}" address add dev lo 192.0.2.10/24
+	ip -netns "${NS0}" address add dev lo 2001:db8::10/64 nodad
+}
+
+# Check if network device has an IPv6 link-local address assigned.
+#
+# Parameters:
+#
+#   * $1: The network device to test
+#   * $2: An extra regular expression that should be matched (to verify the
+#         presence of extra attributes)
+#   * $3: The expected return code from grep (to allow checking the abscence of
+#         a link-local address)
+#   * $4: The user visible name for the scenario being tested
+#
+check_ipv6_ll_addr()
+{
+	local DEV="$1"
+	local EXTRA_MATCH="$2"
+	local XRET="$3"
+	local MSG="$4"
+	local RET
+
+	printf "%-75s  " "${MSG}"
+
+	set +e
+	ip -netns "${NS0}" -6 address show dev "${DEV}" scope link | grep "fe80::" | grep -q "${EXTRA_MATCH}"
+	RET=$?
+	set -e
+
+	if [ "${RET}" -eq "${XRET}" ]; then
+		printf "[ OK ]\n"
+	else
+		ERR=1
+		printf "[FAIL]\n"
+		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
+			printf "\nHit enter to continue, 'q' to quit\n"
+			read -r a
+			if [ "$a" = "q" ]; then
+				exit 1
+			fi
+		fi
+	fi
+}
+
+
+# Create a GRE device and verify that it gets an IPv6 link-local address as
+# expected.
+#
+# Parameters:
+#
+#   * $1: The device type (gre, ip6gre, gretap or ip6gretap)
+#   * $2: The local underlay IP address (can be an IPv4, an IPv6 or "any")
+#   * $3: The remote underlay IP address (can be an IPv4, an IPv6 or "any")
+#   * $4: The IPv6 interface identifier generation mode to use for the GRE
+#         device (eui64, none, stable-privacy or random).
+#
+test_gre_device()
+{
+	local GRE_TYPE="$1"
+	local LOCAL_IP="$2"
+	local REMOTE_IP="$3"
+	local MODE="$4"
+	local ADDR_GEN_MODE
+	local MATCH_REGEXP
+	local MSG
+
+	ip link add netns "${NS0}" name gretest type "${GRE_TYPE}" local "${LOCAL_IP}" remote "${REMOTE_IP}"
+
+	case "${MODE}" in
+	    "eui64")
+		ADDR_GEN_MODE=0
+		MATCH_REGEXP=""
+		MSG="${GRE_TYPE}, mode: 0 (EUI64), ${LOCAL_IP} -> ${REMOTE_IP}"
+		XRET=0
+		;;
+	    "none")
+		ADDR_GEN_MODE=1
+		MATCH_REGEXP=""
+		MSG="${GRE_TYPE}, mode: 1 (none), ${LOCAL_IP} -> ${REMOTE_IP}"
+		XRET=1 # No link-local address should be generated
+		;;
+	    "stable-privacy")
+		ADDR_GEN_MODE=2
+		MATCH_REGEXP="stable-privacy"
+		MSG="${GRE_TYPE}, mode: 2 (stable privacy), ${LOCAL_IP} -> ${REMOTE_IP}"
+		XRET=0
+		# Initialise stable_secret (required for stable-privacy mode)
+		ip netns exec "${NS0}" sysctl -qw net.ipv6.conf.gretest.stable_secret="2001:db8::abcd"
+		;;
+	    "random")
+		ADDR_GEN_MODE=3
+		MATCH_REGEXP="stable-privacy"
+		MSG="${GRE_TYPE}, mode: 3 (random), ${LOCAL_IP} -> ${REMOTE_IP}"
+		XRET=0
+		;;
+	esac
+
+	# Check that IPv6 link-local address is generated when device goes up
+	ip netns exec "${NS0}" sysctl -qw net.ipv6.conf.gretest.addr_gen_mode="${ADDR_GEN_MODE}"
+	ip -netns "${NS0}" link set dev gretest up
+	check_ipv6_ll_addr gretest "${MATCH_REGEXP}" "${XRET}" "config: ${MSG}"
+
+	# Now disable link-local address generation
+	ip -netns "${NS0}" link set dev gretest down
+	ip netns exec "${NS0}" sysctl -qw net.ipv6.conf.gretest.addr_gen_mode=1
+	ip -netns "${NS0}" link set dev gretest up
+
+	# Check that link-local address generation works when re-enabled while
+	# the device is already up
+	ip netns exec "${NS0}" sysctl -qw net.ipv6.conf.gretest.addr_gen_mode="${ADDR_GEN_MODE}"
+	check_ipv6_ll_addr gretest "${MATCH_REGEXP}" "${XRET}" "update: ${MSG}"
+
+	ip -netns "${NS0}" link del dev gretest
+}
+
+test_gre4()
+{
+	local GRE_TYPE
+	local MODE
+
+	for GRE_TYPE in "gre" "gretap"; do
+		printf "\n####\nTesting IPv6 link-local address generation on ${GRE_TYPE} devices\n####\n\n"
+
+		for MODE in "eui64" "none" "stable-privacy" "random"; do
+			test_gre_device "${GRE_TYPE}" 192.0.2.10 192.0.2.11 "${MODE}"
+			test_gre_device "${GRE_TYPE}" any 192.0.2.11 "${MODE}"
+			test_gre_device "${GRE_TYPE}" 192.0.2.10 any "${MODE}"
+		done
+	done
+}
+
+test_gre6()
+{
+	local GRE_TYPE
+	local MODE
+
+	for GRE_TYPE in "ip6gre" "ip6gretap"; do
+		printf "\n####\nTesting IPv6 link-local address generation on ${GRE_TYPE} devices\n####\n\n"
+
+		for MODE in "eui64" "none" "stable-privacy" "random"; do
+			test_gre_device "${GRE_TYPE}" 2001:db8::10 2001:db8::11 "${MODE}"
+			test_gre_device "${GRE_TYPE}" any 2001:db8::11 "${MODE}"
+			test_gre_device "${GRE_TYPE}" 2001:db8::10 any "${MODE}"
+		done
+	done
+}
+
+usage()
+{
+	echo "Usage: $0 [-p]"
+	exit 1
+}
+
+while getopts :p o
+do
+	case $o in
+		p) PAUSE_ON_FAIL="yes";;
+		*) usage;;
+	esac
+done
+
+# Create namespaces before setting up the exit trap.
+# Otherwise, exit_cleanup_all() could delete namespaces that were not created
+# by this script.
+create_namespaces
+
+set -e
+trap exit_cleanup_all EXIT
+
+setup_basenet
+
+test_gre4
+test_gre6
+
+if [ "${ERR}" -eq 1 ]; then
+	echo "Some tests failed." >&2
+else
+	ERR=0
+fi
-- 
2.39.2

Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

[Ido Schimmel]

On Fri, Feb 21, 2025 at 10:24:04AM +0100, Guillaume Nault wrote:
> Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
> devices in most cases and fall back to using add_v4_addrs() only in
> case the GRE configuration is incompatible with addrconf_addr_gen().
> 
> GRE used to use addrconf_addr_gen() until commit e5dd729460ca
> ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
> address") restricted this use to gretap devices and created

It's not always clear throughout the commit message to which devices you
are referring to. For example, here, by "gretap" you mean both "gretap"
and "ip6gretap", no?

BTW, I believe the check against 'ARPHRD_ETHER' in addrconf_gre_config()
is dead code. addrconf_gre_config() is only called for ARPHRD_IP{,6}GRE
devices.

> add_v4_addrs() (borrowed from SIT) for non-Ethernet GRE ones.
> 
> The original problem came when commit 9af28511be10 ("addrconf: refuse
> isatap eui64 for INADDR_ANY") made __ipv6_isatap_ifid() fail when its
> addr parameter was 0. The commit says that this would create an invalid
> address, however, I couldn't find any RFC saying that the generated
> interface identifier would be wrong. Anyway, since plain gre devices
> pass their local tunnel address to __ipv6_isatap_ifid(), that commit
> broke their IPv6 link-local address generation when the local address
> was unspecified.

By "plain gre devices" you mean "ipgre"? Because addrconf_ifid_ip6tnl()
is called for "ip6gre" and it doesn't fail, unlike __ipv6_isatap_ifid().

> 
> Then commit e5dd729460ca ("ip/ip6_gre: use the same logic as SIT
> interfaces when computing v6LL address") tried to fix that case by
> defining add_v4_addrs() and calling it to generated the IPv6 link-local

s/generated/generate/

> address instead of using addrconf_addr_gen() (appart for gretap devices

s/appart/apart/

> which would still use the regular addrconf_addr_gen(), since they have
> a MAC address).

Assuming what I wrote is correct, I'm not sure why e5dd729460ca didn't
restrict the fix to "ipgre" and applied it to "ip6gre" as well.

> 
> That broke several use cases because add_v4_addrs() isn't properly
> integrated into the rest of IPv6 Neighbor Discovery code. Several of
> these shortcomings have been fixed over time, but add_v4_addrs()
> remains broken on several aspects. In particular, it doesn't send any
> Router Sollicitations, so the SLAAC process doesn't start until the
> interface receives a Router Advertisement. Also, add_v4_addrs() mostly
> ignores the address generation mode of the interface
> (/proc/sys/net/ipv6/conf/*/addr_gen_mode), thus breaking the
> IN6_ADDR_GEN_MODE_RANDOM and IN6_ADDR_GEN_MODE_STABLE_PRIVACY cases.
> 
> Fix all this by reverting to addrconf_addr_gen() in all cases but the
> very specific one that remains incompatible.
> 
> Fix the situation by using add_v4_addrs() only in the specific scenario
> where normal method would fail. That is, for interfaces that have all
> of the following characteristics:
> 
>   * transport IP packets directly, not Ethernet (that is, not gretap),
>   * run over IPv4,
>   * tunnel endpoint is INADDR_ANY (that is, 0),
>   * device address generation mode is EUI64.
> 
> In all other cases, revert back to the regular addrconf_addr_gen().
> 
> Also, remove the special case for ip6gre interfaces in add_v4_addrs(),
> since ip6gre devices now always use addrconf_addr_gen() instead.
> 
> Fixes: e5dd729460ca ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL address")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
>  net/ipv6/addrconf.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ac8cc1076536..8b6258819dad 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3209,16 +3209,13 @@ static void add_v4_addrs(struct inet6_dev *idev)
>  	struct in6_addr addr;
>  	struct net_device *dev;
>  	struct net *net = dev_net(idev->dev);
> -	int scope, plen, offset = 0;
> +	int scope, plen;
>  	u32 pflags = 0;
>  
>  	ASSERT_RTNL();
>  
>  	memset(&addr, 0, sizeof(struct in6_addr));
> -	/* in case of IP6GRE the dev_addr is an IPv6 and therefore we use only the last 4 bytes */
> -	if (idev->dev->addr_len == sizeof(struct in6_addr))
> -		offset = sizeof(struct in6_addr) - 4;
> -	memcpy(&addr.s6_addr32[3], idev->dev->dev_addr + offset, 4);
> +	memcpy(&addr.s6_addr32[3], idev->dev->dev_addr, 4);
>  
>  	if (!(idev->dev->flags & IFF_POINTOPOINT) && idev->dev->type == ARPHRD_SIT) {
>  		scope = IPV6_ADDR_COMPATv4;
> @@ -3529,7 +3526,13 @@ static void addrconf_gre_config(struct net_device *dev)
>  		return;
>  	}
>  
> -	if (dev->type == ARPHRD_ETHER) {
> +	/* Generate the IPv6 link-local address using addrconf_addr_gen(),
> +	 * unless we have an IPv4 GRE device not bound to an IP address and
> +	 * which is in EUI64 mode (as __ipv6_isatap_ifid() would fail in this
> +	 * case). Such devices fall back to add_v4_addrs() instead.
> +	 */
> +	if (!(dev->type == ARPHRD_IPGRE && *(__be32 *)dev->dev_addr == 0 &&

Doesn't this mean that the 'ARPHRD_IP6GRE' case (and the
'CONFIG_IPV6_GRE' checks) can be removed from
addrconf_init_auto_addrs()? That is, only call addrconf_gre_config() for
"ipgre", but not for "ip6gre".

> +	      idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64)) {
>  		addrconf_addr_gen(idev, true);
>  		return;
>  	}
> -- 
> 2.39.2
> 
> 

Re: [PATCH net v2 2/2] selftests: Add IPv6 link-local address generation tests for GRE devices.

[Ido Schimmel]

On Fri, Feb 21, 2025 at 10:24:10AM +0100, Guillaume Nault wrote:
> GRE devices have their special code for IPv6 link-local address
> generation that has been the source of several regressions in the past.
> 
> Add selftest to check that all gre, ip6gre, gretap and ip6gretap get an
> IPv6 link-link local address in accordance with the
> net.ipv6.conf.<dev>.addr_gen_mode sysctl.
> 
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

There are some helpers from lib.sh that could have been used, but the
test is very clean and easy to follow, so:

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Thanks!

Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

[Guillaume Nault]

On Sun, Feb 23, 2025 at 03:16:08PM +0200, Ido Schimmel wrote:
> On Fri, Feb 21, 2025 at 10:24:04AM +0100, Guillaume Nault wrote:
> > Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
> > devices in most cases and fall back to using add_v4_addrs() only in
> > case the GRE configuration is incompatible with addrconf_addr_gen().
> > 
> > GRE used to use addrconf_addr_gen() until commit e5dd729460ca
> > ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
> > address") restricted this use to gretap devices and created
> 
> It's not always clear throughout the commit message to which devices you
> are referring to.

Yes, that's a problem I had when writing the commit message: I couldn't
find a proper way to name the different GRE device types unambiguously.

By reusing the device types of "ip link" we don't know if "gre" refers
to all GRE types or if it's only for IPv4 encapsulation. But using the
ARPHRD_* types wouldn't help, as that wouldn't allow to distinguish
between gretap and ip6gretap.

Maybe the following terms would be clearer:
'ip4gre', 'ip4gretap', 'ip6gre', 'ip6gretap' (and potentially 'ipXgre'
and 'ipXgretap' when considering both the IPv4 and IPv6 tunnel
versions). Would you find these terms clearer?

> For example, here, by "gretap" you mean both "gretap"
> and "ip6gretap", no?

Yes.

> BTW, I believe the check against 'ARPHRD_ETHER' in addrconf_gre_config()
> is dead code. addrconf_gre_config() is only called for ARPHRD_IP{,6}GRE
> devices.

Yes, that was dead code. But I'm reusing that condition to minimise
code changes so to make the fix simpler. Do you mean I should write
explicitely, somewhere, that it was dead code but isn't anymore?

> > add_v4_addrs() (borrowed from SIT) for non-Ethernet GRE ones.
> > 
> > The original problem came when commit 9af28511be10 ("addrconf: refuse
> > isatap eui64 for INADDR_ANY") made __ipv6_isatap_ifid() fail when its
> > addr parameter was 0. The commit says that this would create an invalid
> > address, however, I couldn't find any RFC saying that the generated
> > interface identifier would be wrong. Anyway, since plain gre devices
> > pass their local tunnel address to __ipv6_isatap_ifid(), that commit
> > broke their IPv6 link-local address generation when the local address
> > was unspecified.
> 
> By "plain gre devices" you mean "ipgre"? Because addrconf_ifid_ip6tnl()
> is called for "ip6gre" and it doesn't fail, unlike __ipv6_isatap_ifid().

Exactly. I tried to use the "plain" adjective to say that's the kind of
device you get with the "gre" keyword in "ip link".

> > Then commit e5dd729460ca ("ip/ip6_gre: use the same logic as SIT
> > interfaces when computing v6LL address") tried to fix that case by
> > defining add_v4_addrs() and calling it to generated the IPv6 link-local
> 
> s/generated/generate/
> 
> > address instead of using addrconf_addr_gen() (appart for gretap devices
> 
> s/appart/apart/

Will fix both.

> > which would still use the regular addrconf_addr_gen(), since they have
> > a MAC address).
> 
> Assuming what I wrote is correct, I'm not sure why e5dd729460ca didn't
> restrict the fix to "ipgre" and applied it to "ip6gre" as well.

I asked myself the same question. Antonio might have an answer to this.
But in my understanding the changes brought by e5dd729460ca were much too
broad.

> > -	if (dev->type == ARPHRD_ETHER) {
> > +	/* Generate the IPv6 link-local address using addrconf_addr_gen(),
> > +	 * unless we have an IPv4 GRE device not bound to an IP address and
> > +	 * which is in EUI64 mode (as __ipv6_isatap_ifid() would fail in this
> > +	 * case). Such devices fall back to add_v4_addrs() instead.
> > +	 */
> > +	if (!(dev->type == ARPHRD_IPGRE && *(__be32 *)dev->dev_addr == 0 &&
> 
> Doesn't this mean that the 'ARPHRD_IP6GRE' case (and the
> 'CONFIG_IPV6_GRE' checks) can be removed from
> addrconf_init_auto_addrs()? That is, only call addrconf_gre_config() for
> "ipgre", but not for "ip6gre".

Yes. But I didn't want to do that here, to keep the fix as simple as
possible. Because that'd mean we'd also have to add a
"(dev->type != ARPHRD_IP6GRE)" condition in the test at the beginning
of addrconf_dev_config(), and I feel that'd be a distraction from the
core of the patch. So I prefer to do that in net-next.

> > +	      idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64)) {
> >  		addrconf_addr_gen(idev, true);
> >  		return;
> >  	}
> > -- 
> > 2.39.2
> > 
> > 
> 

Thanks a lot for your review.

Re: [PATCH net v2 2/2] selftests: Add IPv6 link-local address generation tests for GRE devices.

[Guillaume Nault]

On Sun, Feb 23, 2025 at 04:29:40PM +0200, Ido Schimmel wrote:
> On Fri, Feb 21, 2025 at 10:24:10AM +0100, Guillaume Nault wrote:
> > GRE devices have their special code for IPv6 link-local address
> > generation that has been the source of several regressions in the past.
> > 
> > Add selftest to check that all gre, ip6gre, gretap and ip6gretap get an
> > IPv6 link-link local address in accordance with the
> > net.ipv6.conf.<dev>.addr_gen_mode sysctl.
> > 
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> 
> There are some helpers from lib.sh that could have been used,

Yes, I reused a personnal template that predates lib.sh. Given how
simple is the setup of this selftest, I'm not sure if it's worth
including lib.sh. But I might consider doing that in v2.

> but the test is very clean and easy to follow, so:
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Tested-by: Ido Schimmel <idosch@nvidia.com>
> 
> Thanks!
> 

Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

[Ido Schimmel]

On Mon, Feb 24, 2025 at 06:27:56PM +0100, Guillaume Nault wrote:
> On Sun, Feb 23, 2025 at 03:16:08PM +0200, Ido Schimmel wrote:
> > On Fri, Feb 21, 2025 at 10:24:04AM +0100, Guillaume Nault wrote:
> > > Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
> > > devices in most cases and fall back to using add_v4_addrs() only in
> > > case the GRE configuration is incompatible with addrconf_addr_gen().
> > > 
> > > GRE used to use addrconf_addr_gen() until commit e5dd729460ca
> > > ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
> > > address") restricted this use to gretap devices and created
> > 
> > It's not always clear throughout the commit message to which devices you
> > are referring to.
> 
> Yes, that's a problem I had when writing the commit message: I couldn't
> find a proper way to name the different GRE device types unambiguously.
> 
> By reusing the device types of "ip link" we don't know if "gre" refers
> to all GRE types or if it's only for IPv4 encapsulation. But using the
> ARPHRD_* types wouldn't help, as that wouldn't allow to distinguish
> between gretap and ip6gretap.

Right.

> 
> Maybe the following terms would be clearer:
> 'ip4gre', 'ip4gretap', 'ip6gre', 'ip6gretap' (and potentially 'ipXgre'
> and 'ipXgretap' when considering both the IPv4 and IPv6 tunnel
> versions). Would you find these terms clearer?

I'm fine with the above, but I also think that as long as "ip link"
types (e.g., 'gre', 'ip6gre') are consistently used throughout the
commit message, it should be clear which devices the commit message
refers to. Whatever you prefer.

> 
> > For example, here, by "gretap" you mean both "gretap"
> > and "ip6gretap", no?
> 
> Yes.
> 
> > BTW, I believe the check against 'ARPHRD_ETHER' in addrconf_gre_config()
> > is dead code. addrconf_gre_config() is only called for ARPHRD_IP{,6}GRE
> > devices.
> 
> Yes, that was dead code. But I'm reusing that condition to minimise
> code changes so to make the fix simpler. Do you mean I should write
> explicitely, somewhere, that it was dead code but isn't anymore?

No, it's fine as-is. I made the comment while reading the commit message
and looking at the unpatched code and only later when I checked the
patch I noticed that you already took care of it :)

> 
> > > add_v4_addrs() (borrowed from SIT) for non-Ethernet GRE ones.
> > > 
> > > The original problem came when commit 9af28511be10 ("addrconf: refuse
> > > isatap eui64 for INADDR_ANY") made __ipv6_isatap_ifid() fail when its
> > > addr parameter was 0. The commit says that this would create an invalid
> > > address, however, I couldn't find any RFC saying that the generated
> > > interface identifier would be wrong. Anyway, since plain gre devices
> > > pass their local tunnel address to __ipv6_isatap_ifid(), that commit
> > > broke their IPv6 link-local address generation when the local address
> > > was unspecified.
> > 
> > By "plain gre devices" you mean "ipgre"? Because addrconf_ifid_ip6tnl()
> > is called for "ip6gre" and it doesn't fail, unlike __ipv6_isatap_ifid().
> 
> Exactly. I tried to use the "plain" adjective to say that's the kind of
> device you get with the "gre" keyword in "ip link".
> 
> > > Then commit e5dd729460ca ("ip/ip6_gre: use the same logic as SIT
> > > interfaces when computing v6LL address") tried to fix that case by
> > > defining add_v4_addrs() and calling it to generated the IPv6 link-local
> > 
> > s/generated/generate/
> > 
> > > address instead of using addrconf_addr_gen() (appart for gretap devices
> > 
> > s/appart/apart/
> 
> Will fix both.
> 
> > > which would still use the regular addrconf_addr_gen(), since they have
> > > a MAC address).
> > 
> > Assuming what I wrote is correct, I'm not sure why e5dd729460ca didn't
> > restrict the fix to "ipgre" and applied it to "ip6gre" as well.
> 
> I asked myself the same question. Antonio might have an answer to this.
> But in my understanding the changes brought by e5dd729460ca were much too
> broad.

I agree.

> 
> > > -	if (dev->type == ARPHRD_ETHER) {
> > > +	/* Generate the IPv6 link-local address using addrconf_addr_gen(),
> > > +	 * unless we have an IPv4 GRE device not bound to an IP address and
> > > +	 * which is in EUI64 mode (as __ipv6_isatap_ifid() would fail in this
> > > +	 * case). Such devices fall back to add_v4_addrs() instead.
> > > +	 */
> > > +	if (!(dev->type == ARPHRD_IPGRE && *(__be32 *)dev->dev_addr == 0 &&
> > 
> > Doesn't this mean that the 'ARPHRD_IP6GRE' case (and the
> > 'CONFIG_IPV6_GRE' checks) can be removed from
> > addrconf_init_auto_addrs()? That is, only call addrconf_gre_config() for
> > "ipgre", but not for "ip6gre".
> 
> Yes. But I didn't want to do that here, to keep the fix as simple as
> possible. Because that'd mean we'd also have to add a
> "(dev->type != ARPHRD_IP6GRE)" condition in the test at the beginning
> of addrconf_dev_config(), and I feel that'd be a distraction from the
> core of the patch. So I prefer to do that in net-next.

Fine by me. Thanks!

Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

[Guillaume Nault]

On Mon, Feb 24, 2025 at 08:21:14PM +0200, Ido Schimmel wrote:
> On Mon, Feb 24, 2025 at 06:27:56PM +0100, Guillaume Nault wrote:
> > On Sun, Feb 23, 2025 at 03:16:08PM +0200, Ido Schimmel wrote:
> > > On Fri, Feb 21, 2025 at 10:24:04AM +0100, Guillaume Nault wrote:
> > > > Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
> > > > devices in most cases and fall back to using add_v4_addrs() only in
> > > > case the GRE configuration is incompatible with addrconf_addr_gen().
> > > > 
> > > > GRE used to use addrconf_addr_gen() until commit e5dd729460ca
> > > > ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
> > > > address") restricted this use to gretap devices and created
> > > 
> > > It's not always clear throughout the commit message to which devices you
> > > are referring to.
> > 
> > Maybe the following terms would be clearer:
> > 'ip4gre', 'ip4gretap', 'ip6gre', 'ip6gretap' (and potentially 'ipXgre'
> > and 'ipXgretap' when considering both the IPv4 and IPv6 tunnel
> > versions). Would you find these terms clearer?
> 
> I'm fine with the above, but I also think that as long as "ip link"
> types (e.g., 'gre', 'ip6gre') are consistently used throughout the
> commit message, it should be clear which devices the commit message
> refers to. Whatever you prefer.

I've finally opted for reusing "ip link" types, plus a bit of rewording
to remove potential ambiguities.

Re: [PATCH net v2 2/2] selftests: Add IPv6 link-local address generation tests for GRE devices.

[Guillaume Nault]

On Mon, Feb 24, 2025 at 06:49:43PM +0100, Guillaume Nault wrote:
> On Sun, Feb 23, 2025 at 04:29:40PM +0200, Ido Schimmel wrote:
> > On Fri, Feb 21, 2025 at 10:24:10AM +0100, Guillaume Nault wrote:
> > > GRE devices have their special code for IPv6 link-local address
> > > generation that has been the source of several regressions in the past.
> > > 
> > > Add selftest to check that all gre, ip6gre, gretap and ip6gretap get an
> > > IPv6 link-link local address in accordance with the
> > > net.ipv6.conf.<dev>.addr_gen_mode sysctl.
> > > 
> > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > 
> > There are some helpers from lib.sh that could have been used,
> 
> Yes, I reused a personnal template that predates lib.sh. Given how
> simple is the setup of this selftest, I'm not sure if it's worth
> including lib.sh. But I might consider doing that in v2.

I've finally prefered to keep the script as-is. Reusing lib.sh wouldn't
simplify much, but would require to use bash. And just changing the
shebang to "#! /bin/bash" adds a 1.5 second penalty to the selftest
execution time. That's acceptable of course, but I'm getting a bit
tired of kselftests execution time, so I'd rather not contribute to
increasing it uselessly.

> > but the test is very clean and easy to follow, so:
> > 
> > Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> > Tested-by: Ido Schimmel <idosch@nvidia.com>
> > 
> > Thanks!
> >