Message-ID: <Z7sfmLG4V_kHKRfy@shredder>
Date: Sun, 23 Feb 2025 15:16:08 +0200
From: Ido Schimmel <idosch@...sch.org>
To: Guillaume Nault <gnault@...hat.com>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
	David Ahern <dsahern@...nel.org>,
	Antonio Quartulli <antonio@...delbit.com>
Subject: Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

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@...hat.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
> 
> 

Powered by blists - more mailing lists