netdev - Re: [PATCH net-next] ipv6: Avoid invoking 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] [day] [month] [year] [list]

Message-Id: <20241125093054.3014390-1-gnaaman@drivenets.com>
Date: Mon, 25 Nov 2024 09:30:54 +0000
From: Gilad Naaman <gnaaman@...venets.com>
To: kuba@...nel.org
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next] ipv6: Avoid invoking addrconf_verify_rtnl unnecessarily

> On Mon, 11 Nov 2024 17:16:07 +0000 Gilad Naaman wrote:
> > Do not invoke costly `addrconf_verify_rtnl` if the added address
> > wouldn't need it, or affect the delayed_work timer.
> > 
> > Signed-off-by: Gilad Naaman <gnaaman@...venets.com>
> > ---
> > addrconf_verify_rtnl() deals with either management/temporary (Security)
> > addresses, or with addresses that have some kind of lifetime.
> > 
> > This patches makes it so that ops on addresses that are permanent would
> > not trigger this function.
> > 
> > This does wonders in our use-case of modifying a lot of (~24K) static
> > addresses, since it turns the addition or deletion of addresses to an
> > amortized O(1), instead of O(N).
> > 
> > Modification of management addresses or "non-permanent" (not sure what
> > is the correct jargon) addresses are still slow.
> > 
> > We can improve those in the future, depending on the case:
> > 
> > If the function is called only to handle cases where the scheduled work should
> > be called earlier, I think this would be better served by saving the next
> > expiration and equating to it, since it would save iteration of the
> > table.
> > 
> > If some upkeep *is* needed (e.g. creating a temporary address)
> > I Think it is possible in theory make these modifications faster as
> > well, if we only iterate `idev->if_addrs` as a response for a
> > modification, since it doesn't seem to me like there are any
> > cross-device effects.
> > 
> > I opted to keep this patch simple and not solve this, on the assumption
> > that there aren't many users that need this scale.
> 
> I'd rather you put too much in the commit message than too little.
> Move more (all?) of this above the --- please.

No problem, will do :)
I thought that most of the text is a bit speculative and describes
changes I didn't end up making, but I see the value in including it.

> 
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index d0a99710d65d..12fdabb1deba 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -3072,8 +3072,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
> >  		 */
> > @@ -3082,7 +3081,15 @@ 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 this address is perishable or has temporary
> > +		 * offshoots, as this function is too expansive.
> > +		 */
> > +		if ((cfg->ifa_flags & IFA_F_MANAGETEMPADDR) ||
> > +		    !(cfg->ifa_flags & IFA_F_PERMANENT) ||
> > +		    cfg->preferred_lft != INFINITY_LIFE_TIME)
> 
> Would be very useful for readability to extract the condition into 
> some helper. If addrconf_verify_rtnl() also used that same helper
> reviewing this patch would be trivial..

Good idea.

> > +			addrconf_verify_rtnl(net);
> > +
> >  		return 0;
> >  	} else if (cfg->ifa_flags & IFA_F_MCAUTOJOIN) {
> >  		ipv6_mc_config(net->ipv6.mc_autojoin_sk, false,
> > @@ -3099,6 +3106,7 @@ static int inet6_addr_del(struct net *net, int ifindex, u32 ifa_flags,
> >  	struct inet6_ifaddr *ifp;
> >  	struct inet6_dev *idev;
> >  	struct net_device *dev;
> > +	int is_mgmt_tmp;
> 
> The flag naming isn't super clear, but it's manageD, not manageMENT,
> as in "managed by the kernel".

Oh, whoopse, MANAGEDTEMP tricked me.
Thank you.

> >  
> >  	if (plen > 128) {
> >  		NL_SET_ERR_MSG_MOD(extack, "Invalid prefix length");
> 
> I think this change will need to wait until after the merge window
> (Dec 2nd), sorry nobody reviewed it in time for 6.13 :(

No problem, thank you for time!
I'll resend a polished patch next week.