netdev - Re: [PATCH net-next v2] Do not invoke addrconf_verify

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: <c67e10cf-ae33-4974-93c6-aaa111171635@redhat.com>
Date: Tue, 10 Dec 2024 11:04:43 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Gilad Naaman <gnaaman@...venets.com>,
 "David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2] Do not invoke addrconf_verify_rtnl
 unnecessarily

On 12/5/24 18:12, Gilad Naaman wrote:
> Do not invoke costly `addrconf_verify_rtnl` if the added address
> wouldn't need it, or affect the delayed_work timer.
> 
> For new/modified addresses, call "verify" only if added address has an
> expiration, or if any temporary address was created.
> 
> This is done to account for a case where the new expiration time might
> be sooner than the current delayed_work's expiration.
> 
> For deleted addresses, avoid calling verify at all:
> 
> If the address being deleted is not perishable, and thus does not affect
> the delayed_work expiration, there is not point in going over the entire
> table.
> 
> If the address IS perishable, but is not the soonest-to-be-expired
> address, calling "verify" would not change the expiration, and would be
> a very expensive nop.
> 
> If the address IS perishable, and IS the soonest-to-be-expired address,
> calling or not-calling "verify" for a single address deletion is
> equivalent in cost.

This last statement is not obvious to me, could you please expand the
reasoning?

> 
> But calling "verify" immediately will result in a performance hit when
> deleting many addresses.

Since this is about (control plane) performances, please include the
relevant test details (or even better, please add a small/fast self-test
covering the use-case).

> 
> Signed-off-by: Gilad Naaman <gnaaman@...venets.com>
> ---
>  net/ipv6/addrconf.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index c489a1e6aec9..893502787554 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -310,6 +310,16 @@ static inline bool addrconf_link_ready(const struct net_device *dev)
>  	return netif_oper_up(dev) && !qdisc_tx_is_noop(dev);
>  }
>  
> +static inline bool addrconf_perishable(int ifa_flags, u32 prefered_lft)

Please, do not use 'inline' in c files.

> +{
> +	/* When setting preferred_lft to a value not zero or
> +	 * infinity, while valid_lft is infinity
> +	 * IFA_F_PERMANENT has a non-infinity life time.
> +	 */
> +	return !((ifa_flags & IFA_F_PERMANENT) &&
> +		(prefered_lft == INFINITY_LIFE_TIME));

Minor nit: the intentation is wrong should be:
		
	return !((ifa_flags & IFA_F_PERMANENT) &&
		 (prefered_lft == INFINITY_LIFE_TIME));
> +}
> +
>  static void addrconf_del_rs_timer(struct inet6_dev *idev)
>  {
>  	if (del_timer(&idev->rs_timer))
> @@ -3090,8 +3100,7 @@ static int inet6_addr_add(struct net *net, int ifindex,
>  		 */
>  		if (!(ifp->flags & (IFA_F_OPTIMISTIC | IFA_F_NODAD)))
>  			ipv6_ifa_notify(0, ifp);
> -		/*
> -		 * Note that section 3.1 of RFC 4429 indicates
> +		/* Note that section 3.1 of RFC 4429 indicates
>  		 * that the Optimistic flag should not be set for
>  		 * manually configured addresses
>  		 */
> @@ -3100,7 +3109,14 @@ static int inet6_addr_add(struct net *net, int ifindex,
>  			manage_tempaddrs(idev, ifp, cfg->valid_lft,
>  					 cfg->preferred_lft, true, jiffies);
>  		in6_ifa_put(ifp);
> -		addrconf_verify_rtnl(net);
> +
> +		/* Verify only if it's possible that adding this address
> +		 * may modify the worker expiration time.
> +		 */
> +		if ((cfg->ifa_flags & IFA_F_MANAGETEMPADDR) ||
> +		    addrconf_perishable(cfg->ifa_flags, cfg->preferred_lft))
> +			addrconf_verify_rtnl(net);
> +
>  		return 0;
>  	} else if (cfg->ifa_flags & IFA_F_MCAUTOJOIN) {
>  		ipv6_mc_config(net->ipv6.mc_autojoin_sk, false,
> @@ -3148,7 +3164,6 @@ static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
>  			    (ifp->flags & IFA_F_MANAGETEMPADDR))
>  				delete_tempaddrs(idev, ifp);
>  
> -			addrconf_verify_rtnl(net);

With an additional 'addrconf_perishable' check here protecting the (here
removed) addrconf_verify_rtnl(), the patch will be IMHO much less prone
to unintended side-effects.

Thanks,

Paolo