netdev - Re: [PATCH] ipv6: fix locking issues with loops over idev->addr

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: <2ef6b0571179c75636830bd9810a777d197738f4.camel@redhat.com>
Date:   Wed, 23 Mar 2022 09:20:55 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Niels Dossche <dossche.niels@...il.com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] ipv6: fix locking issues with loops over idev->addr_list

On Tue, 2022-03-22 at 22:34 +0100, Niels Dossche wrote:
> idev->addr_list needs to be protected by idev->lock. However, it is not
> always possible to do so while iterating and performing actions on
> inet6_ifaddr instances. For example, multiple functions (like
> addrconf_{join,leave}_anycast) eventually call down to other functions
> that acquire the idev->lock. The current code temporarily unlocked the
> idev->lock during the loops, which can cause race conditions. Moving the
> locks up is also not an appropriate solution as the ordering of lock
> acquisition will be inconsistent with for example mc_lock.
> 
> This solution adds an additional field to inet6_ifaddr that is used
> to temporarily add the instances to a temporary list while holding
> idev->lock. The temporary list can then be traversed without holding
> idev->lock. This change was done in two places. In addrconf_ifdown, the
> list_for_each_entry_safe variant of the list loop is also no longer
> necessary as there is no deletion within that specific loop.
> 
> The remaining loop in addrconf_ifdown that unlocks idev->lock in its
> loop body is of no issue. This is because that loop always gets the
> first entry and performs the delete and condition check under the
> idev->lock.
> 
> Suggested-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Niels Dossche <dossche.niels@...il.com>
> ---
> 
> This was previously discussed in the mailing thread of
> [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change
> 
>  include/net/if_inet6.h |  7 +++++++
>  net/ipv6/addrconf.c    | 29 +++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
> index f026cf08a8e8..a17f29f75e9a 100644
> --- a/include/net/if_inet6.h
> +++ b/include/net/if_inet6.h
> @@ -64,6 +64,13 @@ struct inet6_ifaddr {
>  
>  	struct hlist_node	addr_lst;
>  	struct list_head	if_list;
> +	/*
> +	 * Used to safely traverse idev->addr_list in process context
> +	 * if the idev->lock needed to protect idev->addr_list cannot be held.
> +	 * In that case, add the items to this list temporarily and iterate
> +	 * without holding idev->lock. See addrconf_ifdown and dev_forward_change.
> +	 */
> +	struct list_head	if_list_aux;
>  
>  	struct list_head	tmp_list;
>  	struct inet6_ifaddr	*ifpub;
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f908e2fd30b2..72790d1934c7 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -800,6 +800,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>  {
>  	struct net_device *dev;
>  	struct inet6_ifaddr *ifa;
> +	LIST_HEAD(tmp_addr_list);
>  
>  	if (!idev)
>  		return;
> @@ -818,14 +819,23 @@ static void dev_forward_change(struct inet6_dev *idev)
>  		}
>  	}
>  
> +	read_lock_bh(&idev->lock);
>  	list_for_each_entry(ifa, &idev->addr_list, if_list) {
>  		if (ifa->flags&IFA_F_TENTATIVE)
>  			continue;
> +		list_add_tail(&ifa->if_list_aux, &tmp_addr_list);
> +	}
> +	read_unlock_bh(&idev->lock);
> +
> +	while (!list_empty(&tmp_addr_list)) {
> +		ifa = list_first_entry(&tmp_addr_list, struct inet6_ifaddr, if_list_aux);
> +		list_del(&ifa->if_list_aux);
>  		if (idev->cnf.forwarding)
>  			addrconf_join_anycast(ifa);
>  		else
>  			addrconf_leave_anycast(ifa);
>  	}
> +
>  	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
>  				     NETCONFA_FORWARDING,
>  				     dev->ifindex, &idev->cnf);
> @@ -3730,10 +3740,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  	unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_DOWN;
>  	struct net *net = dev_net(dev);
>  	struct inet6_dev *idev;
> -	struct inet6_ifaddr *ifa, *tmp;
> +	struct inet6_ifaddr *ifa;
>  	bool keep_addr = false;
>  	bool was_ready;
>  	int state, i;
> +	LIST_HEAD(tmp_addr_list);

Very minot nit: I guess it's better to try to enforce the reverse x-mas
tree order for newly added variables - that is: this declaration should
me moved up, just after 'ifa'.

>  	ASSERT_RTNL();
>  
> @@ -3822,16 +3833,23 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  		write_lock_bh(&idev->lock);
>  	}
>  
> -	list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list) {
> +	list_for_each_entry(ifa, &idev->addr_list, if_list) {
> +		list_add_tail(&ifa->if_list_aux, &tmp_addr_list);
> +	}


Other minor nit: the braces are not required here.

Otherwise LGTM:

Acked-by: Paolo Abeni <pabeni@...hat.com>

However this looks like net-next material, and we are in the merge
window right now. I think you should re-post in (slighly less) than 2w.
Please add the target tree into the subj.

Side note: AFAICS after this patch, there is still the suspicious
tempaddr_list usage in addrconf_ifdown to be handled.

Cheers,

Paolo