Re: [PATCH net v3 1/3] net/ipv6: add ipv6_chk_addr_namespace

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the patch clarifies how ipv6_chk_addr()
  and related functions behave, but two of the new comments describe the…
- [Low] ipv6_chk_addr_namespace() is exported with EXPORT_SYMBOL, but no
  module uses it.

--

net/ipv6: add ipv6_chk_addr_namespace

This patch adds a skip_l3mdev_check argument to ipv6_chk_addr_and_flags()
and a new ipv6_chk_addr_namespace() helper. The helper looks for an address
in every L3 domain of the namespace. The patch also rewrites the comments
that describe how ipv6_chk_addr() and related functions behave when dev is
NULL.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index c8c0dd04982d9..b13a5846ec95a 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c

[ ... ]

> @@ -2026,7 +2013,7 @@ __ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>  			    : ifp->flags;
>  		if (ipv6_addr_equal(&ifp->addr, addr) &&
>  		    !(ifp_flags&banned_flags) &&
> -		    (!dev || ndev == dev ||
> +		    (skip_dev_check || ndev == dev ||
>  		     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))) {
>  			rcu_read_unlock();
>  			return ndev;
> @@ -2037,15 +2024,46 @@ __ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>  	return NULL;
>  }
>  
> +/* The 'dev' argument is used to match the exact device or find the L3 domain of
> + * interest.
> + * The 3 cases for addresses checks are:
> + *   1. With 'skip_dev_check == false': check if 'addr' exists on 'dev'.
> + *   2. With 'skip_dev_check == true && skip_l3mdev_check == false': check if
> + *      'addr' exists on the same L3 domain as 'dev'.
> + *   3. With 'skip_dev_check == true && skip_l3mdev_check == true': check if
> + *      'addr' exists in the entire namespace.
> + * For the purpose of searching in a specific L3 domain, 'dev == NULL' means
> + * "search in the main L3 domain".
> + */

[Severity: Low]
Is the description of case 1 accurate? The commit message says the patch
clarifies the behaviour, but the match condition in
__ipv6_chk_addr_and_flags() quoted above is looser:

    (skip_dev_check || ndev == dev ||
     !(ifp->scope&(IFA_LINK|IFA_HOST) || strict))

With skip_dev_check == false and strict == 0, a global scope address on any
other device in the same L3 domain also matches. Only link-local or host
scope addresses, or strict == 1, are tied to the exact device.

Existing callers depend on this. ip6_route_info_create() checks prefsrc
with ipv6_chk_addr(net, &cfg->fc_prefsrc, dev, 0). ip6_tnl_rcv_ctl() and
ip6_tnl_xmit_ctl() pass (ldev, false, false, 0, ...).

The new comment on ipv6_chk_addr() below has the same problem ("Check if
'addr' is configured on 'dev'"), because ipv6_chk_addr() passes the
caller's strict value through.

The ipv6_chk_addr_namespace() comment also says "exists in the entire
namespace". It leaves out that tentative addresses are skipped because of
IFA_F_TENTATIVE, while optimistic ones are accepted.

Could these comments mention the strict and scope exception, and the
tentative filtering?

>  int ipv6_chk_addr_and_flags(struct net *net, const struct in6_addr *addr,
>  			    const struct net_device *dev, bool skip_dev_check,

[ ... ]

> +/* Check if 'addr' is configured on 'dev'. If 'dev' is NULL, check the entire
> + * main L3 domain
> + */
> +int ipv6_chk_addr(struct net *net, const struct in6_addr *addr,
> +		  const struct net_device *dev, int strict)
> +{
> +	return ipv6_chk_addr_and_flags(net, addr, dev, !dev, false,
> +				       strict, IFA_F_TENTATIVE);
> +}
> +EXPORT_SYMBOL(ipv6_chk_addr);
> +
> +/* Check if 'addr' exists in the entire namespace, in any L3 domain */
> +int ipv6_chk_addr_namespace(struct net *net, const struct in6_addr *addr)
> +{
> +	return ipv6_chk_addr_and_flags(net, addr, NULL, true, true, 0,
> +				       IFA_F_TENTATIVE);
> +}
> +EXPORT_SYMBOL(ipv6_chk_addr_namespace);

[Severity: Low]
Is this export needed? CONFIG_IPV6 is bool in net/ipv6/Kconfig, so
addrconf.c is always built in.

The only user in the series is ip6_is_local_saddr() in
net/ipv6/ip6_output.c. It is added by the follow-up patch "net/ipv6: don't
route packets with unknown source address", and that file is also always
built in.

Could the EXPORT_SYMBOL be dropped? If a modular user is planned, could it
be EXPORT_SYMBOL_GPL instead?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028191103.623156-1-ihuguet%40riseup.net