Re: [PATCH v4 net-next 5/5] geneve: Introduce IFLA_GENEVE_LOCAL and IFLA_GENEVE_LOCAL6.

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
geneve: Introduce IFLA_GENEVE_LOCAL and IFLA_GENEVE_LOCAL6.

This patch adds IFLA_GENEVE_LOCAL and IFLA_GENEVE_LOCAL6 attributes so a
GENEVE device's underlying UDP socket can be bound to a specific local
IPv4 or IPv6 address rather than the wildcard, and updates lookup paths
and xmit paths accordingly.

> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 3a62d132a8c4..c5372b33a305 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c

[ ... ]

> @@ -1972,6 +2043,55 @@ static int geneve_nl2info(struct nlattr *tb[], struct nlattr *data[],
>  #endif
>  	}
>  
> +	if (data[IFLA_GENEVE_LOCAL]) {
> +		if (changelink) {
> +			attrtype = IFLA_GENEVE_LOCAL;
> +			goto change_notsup;
> +		}
> +
> +		info->key.u.ipv4.src = nla_get_in_addr(data[IFLA_GENEVE_LOCAL]);
> +
> +		if (ipv4_is_multicast(info->key.u.ipv4.src)) {
> +			NL_SET_ERR_MSG_ATTR(extack, data[IFLA_GENEVE_LOCAL],
> +					    "Local IPv4 address cannot be Multicast");
> +			return -EINVAL;
> +		}
> +
> +		cfg->dualstack = false;
> +	}
> +
> +	if (data[IFLA_GENEVE_LOCAL6]) {
> +#if IS_ENABLED(CONFIG_IPV6)
> +		int addr_type;
> +
> +		if (changelink) {
> +			attrtype = IFLA_GENEVE_LOCAL6;
> +			goto change_notsup;
> +		}

[Medium]
Should the changelink rejection here mirror the IFLA_GENEVE_REMOTE
behavior in this same function? IFLA_GENEVE_REMOTE only fails the
changelink when the family changes:

	if (data[IFLA_GENEVE_REMOTE]) {
		if (changelink && (ip_tunnel_info_af(info) == AF_INET6)) {
			attrtype = IFLA_GENEVE_REMOTE;
			goto change_notsup;
		}

whereas IFLA_GENEVE_LOCAL and IFLA_GENEVE_LOCAL6 fail unconditionally
whenever the attribute is present, even when the value matches the
existing configuration.

Combined with the geneve_fill_info() change below, would this break the
common dump-then-changelink round trip used by some management tools on
pre-existing devices, returning -EOPNOTSUPP even though LOCAL was never
explicitly configured?

[ ... ]

> @@ -2320,6 +2441,20 @@ static int geneve_fill_info(struct sk_buff *skb, const struct net_device *dev)
>  #endif
>  	}
>  
> +	if (!geneve->cfg.dualstack) {
> +		if (ip_tunnel_info_af(info) == AF_INET) {
> +			if (nla_put_in_addr(skb, IFLA_GENEVE_LOCAL,
> +					    info->key.u.ipv4.src))
> +				goto nla_put_failure;
> +#if IS_ENABLED(CONFIG_IPV6)
> +		} else {
> +			if (nla_put_in6_addr(skb, IFLA_GENEVE_LOCAL6,
> +					     &info->key.u.ipv6.src))
> +				goto nla_put_failure;
> +#endif
> +		}
> +	}
> +

[Medium]
Does this emit IFLA_GENEVE_LOCAL or IFLA_GENEVE_LOCAL6 on every
non-collect_md device, including legacy devices created without any
local address?

cfg->dualstack is only set true for collect_md devices, so for any
device where the user never specified a local address, info->key.u.ipv4.src
is 0 (or info->key.u.ipv6.src is ::) but the attribute is still emitted.
This would change the visible output of ip -d link show and netlink
dumps for existing GENEVE devices, adding a synthetic local 0.0.0.0 or
local :: that was not present before.

Would gating this on a non-zero source address (or tracking whether the
user explicitly set LOCAL) avoid the dump-output change for unaffected
devices?

The commit message describes the new behavior on devices that set
LOCAL/LOCAL6 but does not mention that fill_info now emits these
attributes for every previously-created non-collect_md device, nor that
changelink will reject any presence of LOCAL/LOCAL6 even when the value
is unchanged. Could the commit message call out these compatibility
implications?