[PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Niels Dossche]

No path towards dev_forward_change (common ancestor of paths is in
addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
We need to hold the lock during the whole loop in dev_forward_change.
__ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
their function body. Since addrconf_{join,leave}_anycast call to
__ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
the responsibility of locking upwards.

This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
place where the caller can directly acquire the idev->lock, that is in
ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
needs to be called under idev->lock, and thus it becomes the
responsibility of the callers of addrconf_leave_anycast to hold that
lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
ipv6_sock_ac_join, which can acquire the lock during the call to
__ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
needs to be called under idev->lock, and thus it becomes the
responsibility of the callers of addrconf_join_anycast to hold that
lock.

Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---

Changes in v2:
 - Move the locking upwards

 net/ipv6/addrconf.c | 21 ++++++++++++++++-----
 net/ipv6/anycast.c  | 37 ++++++++++++++++---------------------
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f908e2fd30b2..69e9f81e2045 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 		}
 	}
 
+	write_lock_bh(&idev->lock);
 	list_for_each_entry(ifa, &idev->addr_list, if_list) {
 		if (ifa->flags&IFA_F_TENTATIVE)
 			continue;
@@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 		else
 			addrconf_leave_anycast(ifa);
 	}
+	write_unlock_bh(&idev->lock);
 	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
 				     NETCONFA_FORWARDING,
 				     dev->ifindex, &idev->cnf);
@@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
 	__ipv6_dev_mc_dec(idev, &maddr);
 }
 
-/* caller must hold RTNL */
+/* caller must hold RTNL and write lock idev->lock */
 static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
@@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 	__ipv6_dev_ac_inc(ifp->idev, &addr);
 }
 
-/* caller must hold RTNL */
+/* caller must hold RTNL and write lock idev->lock */
 static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
@@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
 			__ipv6_ifa_notify(RTM_DELADDR, ifa);
 			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
 		} else {
-			if (idev->cnf.forwarding)
+			if (idev->cnf.forwarding) {
+				write_lock_bh(&idev->lock);
 				addrconf_leave_anycast(ifa);
+				write_unlock_bh(&idev->lock);
+			}
 			addrconf_leave_solict(ifa->idev, &ifa->addr);
 		}
 
@@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 				&ifp->addr, ifp->idev->dev->name);
 		}
 
-		if (ifp->idev->cnf.forwarding)
+		if (ifp->idev->cnf.forwarding) {
+			write_lock_bh(&ifp->idev->lock);
 			addrconf_join_anycast(ifp);
+			write_unlock_bh(&ifp->idev->lock);
+		}
 		if (!ipv6_addr_any(&ifp->peer_addr))
 			addrconf_prefix_route(&ifp->peer_addr, 128,
 					      ifp->rt_priority, ifp->idev->dev,
 					      0, 0, GFP_ATOMIC);
 		break;
 	case RTM_DELADDR:
-		if (ifp->idev->cnf.forwarding)
+		if (ifp->idev->cnf.forwarding) {
+			write_lock_bh(&ifp->idev->lock);
 			addrconf_leave_anycast(ifp);
+			write_unlock_bh(&ifp->idev->lock);
+		}
 		addrconf_leave_solict(ifp->idev, &ifp->addr);
 		if (!ipv6_addr_any(&ifp->peer_addr)) {
 			struct fib6_info *rt;
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index dacdea7fcb62..f3017ed6f005 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			goto error;
 	}
 
+	write_lock_bh(&idev->lock);
 	err = __ipv6_dev_ac_inc(idev, addr);
+	write_unlock_bh(&idev->lock);
 	if (!err) {
 		pac->acl_next = np->ipv6_ac_list;
 		np->ipv6_ac_list = pac;
@@ -280,41 +282,35 @@ static struct ifacaddr6 *aca_alloc(struct fib6_info *f6i,
 
 /*
  *	device anycast group inc (add if not found)
+ *	caller must hold write lock idev->lock
  */
 int __ipv6_dev_ac_inc(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifacaddr6 *aca;
 	struct fib6_info *f6i;
 	struct net *net;
-	int err;
 
 	ASSERT_RTNL();
 
-	write_lock_bh(&idev->lock);
-	if (idev->dead) {
-		err = -ENODEV;
-		goto out;
-	}
+	if (idev->dead)
+		return -ENODEV;
 
 	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
 		if (ipv6_addr_equal(&aca->aca_addr, addr)) {
 			aca->aca_users++;
-			err = 0;
-			goto out;
+			return 0;
 		}
 	}
 
 	net = dev_net(idev->dev);
 	f6i = addrconf_f6i_alloc(net, idev, addr, true, GFP_ATOMIC);
 	if (IS_ERR(f6i)) {
-		err = PTR_ERR(f6i);
-		goto out;
+		return PTR_ERR(f6i);
 	}
 	aca = aca_alloc(f6i, addr);
 	if (!aca) {
 		fib6_info_release(f6i);
-		err = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	aca->aca_next = idev->ac_list;
@@ -324,7 +320,6 @@ int __ipv6_dev_ac_inc(struct inet6_dev *idev, const struct in6_addr *addr)
 	 * it is already exposed via idev->ac_list.
 	 */
 	aca_get(aca);
-	write_unlock_bh(&idev->lock);
 
 	ipv6_add_acaddr_hash(net, aca);
 
@@ -334,13 +329,11 @@ int __ipv6_dev_ac_inc(struct inet6_dev *idev, const struct in6_addr *addr)
 
 	aca_put(aca);
 	return 0;
-out:
-	write_unlock_bh(&idev->lock);
-	return err;
 }
 
 /*
  *	device anycast group decrement
+ *	caller must hold write lock idev->lock
  */
 int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
@@ -348,7 +341,6 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 
 	ASSERT_RTNL();
 
-	write_lock_bh(&idev->lock);
 	prev_aca = NULL;
 	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
 		if (ipv6_addr_equal(&aca->aca_addr, addr))
@@ -356,18 +348,15 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 		prev_aca = aca;
 	}
 	if (!aca) {
-		write_unlock_bh(&idev->lock);
 		return -ENOENT;
 	}
 	if (--aca->aca_users > 0) {
-		write_unlock_bh(&idev->lock);
 		return 0;
 	}
 	if (prev_aca)
 		prev_aca->aca_next = aca->aca_next;
 	else
 		idev->ac_list = aca->aca_next;
-	write_unlock_bh(&idev->lock);
 	ipv6_del_acaddr_hash(aca);
 	addrconf_leave_solict(idev, &aca->aca_addr);
 
@@ -381,10 +370,16 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
 {
 	struct inet6_dev *idev = __in6_dev_get(dev);
+	int ret;
 
 	if (!idev)
 		return -ENODEV;
-	return __ipv6_dev_ac_dec(idev, addr);
+
+	write_lock_bh(&idev->lock);
+	ret = __ipv6_dev_ac_dec(idev, addr);
+	write_unlock_bh(&idev->lock);
+
+	return ret;
 }
 
 void ipv6_ac_destroy_dev(struct inet6_dev *idev)
-- 
2.35.1

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Paolo Abeni]

On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
> No path towards dev_forward_change (common ancestor of paths is in
> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
> We need to hold the lock during the whole loop in dev_forward_change.
> __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
> their function body. Since addrconf_{join,leave}_anycast call to
> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
> the responsibility of locking upwards.
> 
> This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
> place where the caller can directly acquire the idev->lock, that is in
> ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
> needs to be called under idev->lock, and thus it becomes the
> responsibility of the callers of addrconf_leave_anycast to hold that
> lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
> ipv6_sock_ac_join, which can acquire the lock during the call to
> __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
> needs to be called under idev->lock, and thus it becomes the
> responsibility of the callers of addrconf_join_anycast to hold that
> lock.
> 
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> 
> Changes in v2:
>  - Move the locking upwards
> 
>  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
>  net/ipv6/anycast.c  | 37 ++++++++++++++++---------------------
>  2 files changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f908e2fd30b2..69e9f81e2045 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>  		}
>  	}
>  
> +	write_lock_bh(&idev->lock);
>  	list_for_each_entry(ifa, &idev->addr_list, if_list) {
>  		if (ifa->flags&IFA_F_TENTATIVE)
>  			continue;
> @@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>  		else
>  			addrconf_leave_anycast(ifa);
>  	}
> +	write_unlock_bh(&idev->lock);
>  	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
>  				     NETCONFA_FORWARDING,
>  				     dev->ifindex, &idev->cnf);
> @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
>  	__ipv6_dev_mc_dec(idev, &maddr);
>  }
>  
> -/* caller must hold RTNL */
> +/* caller must hold RTNL and write lock idev->lock */
>  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>  {
>  	struct in6_addr addr;
> @@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>  	__ipv6_dev_ac_inc(ifp->idev, &addr);
>  }
>  
> -/* caller must hold RTNL */
> +/* caller must hold RTNL and write lock idev->lock */
>  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
>  {
>  	struct in6_addr addr;
> @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
>  			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
>  		} else {
> -			if (idev->cnf.forwarding)
> +			if (idev->cnf.forwarding) {
> +				write_lock_bh(&idev->lock);
>  				addrconf_leave_anycast(ifa);
> +				write_unlock_bh(&idev->lock);
> +			}
>  			addrconf_leave_solict(ifa->idev, &ifa->addr);
>  		}
>  
> @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
>  				&ifp->addr, ifp->idev->dev->name);
>  		}
>  
> -		if (ifp->idev->cnf.forwarding)
> +		if (ifp->idev->cnf.forwarding) {
> +			write_lock_bh(&ifp->idev->lock);
>  			addrconf_join_anycast(ifp);
> +			write_unlock_bh(&ifp->idev->lock);
> +		}
>  		if (!ipv6_addr_any(&ifp->peer_addr))
>  			addrconf_prefix_route(&ifp->peer_addr, 128,
>  					      ifp->rt_priority, ifp->idev->dev,
>  					      0, 0, GFP_ATOMIC);
>  		break;
>  	case RTM_DELADDR:
> -		if (ifp->idev->cnf.forwarding)
> +		if (ifp->idev->cnf.forwarding) {
> +			write_lock_bh(&ifp->idev->lock);
>  			addrconf_leave_anycast(ifp);
> +			write_unlock_bh(&ifp->idev->lock);
> +		}
>  		addrconf_leave_solict(ifp->idev, &ifp->addr);
>  		if (!ipv6_addr_any(&ifp->peer_addr)) {
>  			struct fib6_info *rt;
> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index dacdea7fcb62..f3017ed6f005 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
> @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  			goto error;
>  	}
>  
> +	write_lock_bh(&idev->lock);
>  	err = __ipv6_dev_ac_inc(idev, addr);
> +	write_unlock_bh(&idev->lock);

I feat this is problematic, due this call chain:

 __ipv6_dev_ac_inc() -> addrconf_join_solict() -> ipv6_dev_mc_inc ->
__ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);

The latter call requires process context.

One alternarive (likely very hackish way) to solve this could be:
- adding another list entry  into struct inet6_dev, rtnl protected.
- traverse addr_list under idev->lock and add each entry with
forwarding on to into a tmp list (e.g. tmp_join) using the field above;
add the entries with forwarding off into another tmp list (e.g.
tmp_leave), still using the same field.
- traverse the tmp_join list under rtnl lock only and do the
addrconf_join_anycast
- traverse the tmp_leave list under rtnl lock only and do
addrconf_leave_anycast

Side note: I'm wondering if we should complete the RCU conversion (e.g.
moving tempaddr_list to RCU and changing idev->lock). The current mixed
schema looks a bit confusing to me.

Cheers,

Paolo

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Paolo Abeni]

On Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
> On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
> > No path towards dev_forward_change (common ancestor of paths is in
> > addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
> > We need to hold the lock during the whole loop in dev_forward_change.
> > __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
> > their function body. Since addrconf_{join,leave}_anycast call to
> > __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
> > the responsibility of locking upwards.
> > 
> > This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
> > place where the caller can directly acquire the idev->lock, that is in
> > ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
> > needs to be called under idev->lock, and thus it becomes the
> > responsibility of the callers of addrconf_leave_anycast to hold that
> > lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
> > ipv6_sock_ac_join, which can acquire the lock during the call to
> > __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
> > needs to be called under idev->lock, and thus it becomes the
> > responsibility of the callers of addrconf_join_anycast to hold that
> > lock.
> > 
> > Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> > ---
> > 
> > Changes in v2:
> >  - Move the locking upwards
> > 
> >  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
> >  net/ipv6/anycast.c  | 37 ++++++++++++++++---------------------
> >  2 files changed, 32 insertions(+), 26 deletions(-)
> > 
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index f908e2fd30b2..69e9f81e2045 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> >  		}
> >  	}
> >  
> > +	write_lock_bh(&idev->lock);
> >  	list_for_each_entry(ifa, &idev->addr_list, if_list) {
> >  		if (ifa->flags&IFA_F_TENTATIVE)
> >  			continue;
> > @@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> >  		else
> >  			addrconf_leave_anycast(ifa);
> >  	}
> > +	write_unlock_bh(&idev->lock);
> >  	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
> >  				     NETCONFA_FORWARDING,
> >  				     dev->ifindex, &idev->cnf);
> > @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
> >  	__ipv6_dev_mc_dec(idev, &maddr);
> >  }
> >  
> > -/* caller must hold RTNL */
> > +/* caller must hold RTNL and write lock idev->lock */
> >  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> >  {
> >  	struct in6_addr addr;
> > @@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> >  	__ipv6_dev_ac_inc(ifp->idev, &addr);
> >  }
> >  
> > -/* caller must hold RTNL */
> > +/* caller must hold RTNL and write lock idev->lock */
> >  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
> >  {
> >  	struct in6_addr addr;
> > @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
> >  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
> >  			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
> >  		} else {
> > -			if (idev->cnf.forwarding)
> > +			if (idev->cnf.forwarding) {
> > +				write_lock_bh(&idev->lock);
> >  				addrconf_leave_anycast(ifa);
> > +				write_unlock_bh(&idev->lock);
> > +			}
> >  			addrconf_leave_solict(ifa->idev, &ifa->addr);
> >  		}
> >  
> > @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
> >  				&ifp->addr, ifp->idev->dev->name);
> >  		}
> >  
> > -		if (ifp->idev->cnf.forwarding)
> > +		if (ifp->idev->cnf.forwarding) {
> > +			write_lock_bh(&ifp->idev->lock);
> >  			addrconf_join_anycast(ifp);
> > +			write_unlock_bh(&ifp->idev->lock);
> > +		}
> >  		if (!ipv6_addr_any(&ifp->peer_addr))
> >  			addrconf_prefix_route(&ifp->peer_addr, 128,
> >  					      ifp->rt_priority, ifp->idev->dev,
> >  					      0, 0, GFP_ATOMIC);
> >  		break;
> >  	case RTM_DELADDR:
> > -		if (ifp->idev->cnf.forwarding)
> > +		if (ifp->idev->cnf.forwarding) {
> > +			write_lock_bh(&ifp->idev->lock);
> >  			addrconf_leave_anycast(ifp);
> > +			write_unlock_bh(&ifp->idev->lock);
> > +		}
> >  		addrconf_leave_solict(ifp->idev, &ifp->addr);
> >  		if (!ipv6_addr_any(&ifp->peer_addr)) {
> >  			struct fib6_info *rt;
> > diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> > index dacdea7fcb62..f3017ed6f005 100644
> > --- a/net/ipv6/anycast.c
> > +++ b/net/ipv6/anycast.c
> > @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
> >  			goto error;
> >  	}
> >  
> > +	write_lock_bh(&idev->lock);
> >  	err = __ipv6_dev_ac_inc(idev, addr);
> > +	write_unlock_bh(&idev->lock);
> 
> I feat this is problematic, due this call chain:
> 
>  __ipv6_dev_ac_inc() -> addrconf_join_solict() -> ipv6_dev_mc_inc ->
> __ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
> 
> The latter call requires process context.
> 
> One alternarive (likely very hackish way) to solve this could be:
> - adding another list entry  into struct inet6_dev, rtnl protected.

Typo above: the new field should be added to 'struct inet6_ifaddr'.

> - traverse addr_list under idev->lock and add each entry with
> forwarding on to into a tmp list (e.g. tmp_join) using the field above;
> add the entries with forwarding off into another tmp list (e.g.
> tmp_leave), still using the same field.

Again confusing text above, sorry. As the forwarding flag is per
device, all the addr entries will land into the same tmp list.

It's probably better if I sketch up some code...

/P

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Paolo Abeni]

On Fri, 2022-03-18 at 13:48 +0100, Paolo Abeni wrote:
> On Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
> > On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
> > > No path towards dev_forward_change (common ancestor of paths is in
> > > addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
> > > We need to hold the lock during the whole loop in dev_forward_change.
> > > __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
> > > their function body. Since addrconf_{join,leave}_anycast call to
> > > __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
> > > the responsibility of locking upwards.
> > > 
> > > This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
> > > place where the caller can directly acquire the idev->lock, that is in
> > > ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
> > > needs to be called under idev->lock, and thus it becomes the
> > > responsibility of the callers of addrconf_leave_anycast to hold that
> > > lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
> > > ipv6_sock_ac_join, which can acquire the lock during the call to
> > > __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
> > > needs to be called under idev->lock, and thus it becomes the
> > > responsibility of the callers of addrconf_join_anycast to hold that
> > > lock.
> > > 
> > > Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> > > ---
> > > 
> > > Changes in v2:
> > >  - Move the locking upwards
> > > 
> > >  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
> > >  net/ipv6/anycast.c  | 37 ++++++++++++++++---------------------
> > >  2 files changed, 32 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > > index f908e2fd30b2..69e9f81e2045 100644
> > > --- a/net/ipv6/addrconf.c
> > > +++ b/net/ipv6/addrconf.c
> > > @@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> > >  		}
> > >  	}
> > >  
> > > +	write_lock_bh(&idev->lock);
> > >  	list_for_each_entry(ifa, &idev->addr_list, if_list) {
> > >  		if (ifa->flags&IFA_F_TENTATIVE)
> > >  			continue;
> > > @@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> > >  		else
> > >  			addrconf_leave_anycast(ifa);
> > >  	}
> > > +	write_unlock_bh(&idev->lock);
> > >  	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
> > >  				     NETCONFA_FORWARDING,
> > >  				     dev->ifindex, &idev->cnf);
> > > @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
> > >  	__ipv6_dev_mc_dec(idev, &maddr);
> > >  }
> > >  
> > > -/* caller must hold RTNL */
> > > +/* caller must hold RTNL and write lock idev->lock */
> > >  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> > >  {
> > >  	struct in6_addr addr;
> > > @@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> > >  	__ipv6_dev_ac_inc(ifp->idev, &addr);
> > >  }
> > >  
> > > -/* caller must hold RTNL */
> > > +/* caller must hold RTNL and write lock idev->lock */
> > >  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
> > >  {
> > >  	struct in6_addr addr;
> > > @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
> > >  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
> > >  			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
> > >  		} else {
> > > -			if (idev->cnf.forwarding)
> > > +			if (idev->cnf.forwarding) {
> > > +				write_lock_bh(&idev->lock);
> > >  				addrconf_leave_anycast(ifa);
> > > +				write_unlock_bh(&idev->lock);
> > > +			}
> > >  			addrconf_leave_solict(ifa->idev, &ifa->addr);
> > >  		}
> > >  
> > > @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
> > >  				&ifp->addr, ifp->idev->dev->name);
> > >  		}
> > >  
> > > -		if (ifp->idev->cnf.forwarding)
> > > +		if (ifp->idev->cnf.forwarding) {
> > > +			write_lock_bh(&ifp->idev->lock);
> > >  			addrconf_join_anycast(ifp);
> > > +			write_unlock_bh(&ifp->idev->lock);
> > > +		}
> > >  		if (!ipv6_addr_any(&ifp->peer_addr))
> > >  			addrconf_prefix_route(&ifp->peer_addr, 128,
> > >  					      ifp->rt_priority, ifp->idev->dev,
> > >  					      0, 0, GFP_ATOMIC);
> > >  		break;
> > >  	case RTM_DELADDR:
> > > -		if (ifp->idev->cnf.forwarding)
> > > +		if (ifp->idev->cnf.forwarding) {
> > > +			write_lock_bh(&ifp->idev->lock);
> > >  			addrconf_leave_anycast(ifp);
> > > +			write_unlock_bh(&ifp->idev->lock);
> > > +		}
> > >  		addrconf_leave_solict(ifp->idev, &ifp->addr);
> > >  		if (!ipv6_addr_any(&ifp->peer_addr)) {
> > >  			struct fib6_info *rt;
> > > diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> > > index dacdea7fcb62..f3017ed6f005 100644
> > > --- a/net/ipv6/anycast.c
> > > +++ b/net/ipv6/anycast.c
> > > @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
> > >  			goto error;
> > >  	}
> > >  
> > > +	write_lock_bh(&idev->lock);
> > >  	err = __ipv6_dev_ac_inc(idev, addr);
> > > +	write_unlock_bh(&idev->lock);
> > 
> > I feat this is problematic, due this call chain:
> > 
> >  __ipv6_dev_ac_inc() -> addrconf_join_solict() -> ipv6_dev_mc_inc ->
> > __ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
> > 
> > The latter call requires process context.
> > 
> > One alternarive (likely very hackish way) to solve this could be:
> > - adding another list entry  into struct inet6_dev, rtnl protected.
> 
> Typo above: the new field should be added to 'struct inet6_ifaddr'.
> 
> > - traverse addr_list under idev->lock and add each entry with
> > forwarding on to into a tmp list (e.g. tmp_join) using the field above;
> > add the entries with forwarding off into another tmp list (e.g.
> > tmp_leave), still using the same field.
> 
> Again confusing text above, sorry. As the forwarding flag is per
> device, all the addr entries will land into the same tmp list.
> 
> It's probably better if I sketch up some code...

For the records, I mean something alongside the following - completely
not tested:
---
diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
index 4cfdef6ca4f6..2df3c98b9e55 100644
--- a/include/net/if_inet6.h
+++ b/include/net/if_inet6.h
@@ -64,6 +64,7 @@ struct inet6_ifaddr {
 
 	struct hlist_node	addr_lst;
 	struct list_head	if_list;
+	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 b22504176588..27d1081b693e 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -797,6 +797,7 @@ static void dev_forward_change(struct inet6_dev *idev)
 {
 	struct net_device *dev;
 	struct inet6_ifaddr *ifa;
+	LIST_HEAD(tmp);
 
 	if (!idev)
 		return;
@@ -815,9 +816,17 @@ static void dev_forward_change(struct inet6_dev *idev)
 		}
 	}
 
+	rcu_read_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);
+	}
+	rcu_read_unlock();
+
+	while (!list_empty(&tmp)) {
+		ifa = list_first_entry(&tmp, struct inet6_ifaddr, if_list_aux);
+		list_del(&ifa->if_list_aux);
 		if (idev->cnf.forwarding)
 			addrconf_join_anycast(ifa);
 		else

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Niels Dossche]

On 18/03/2022 16:42, Paolo Abeni wrote:
> On Fri, 2022-03-18 at 13:48 +0100, Paolo Abeni wrote:
>> On Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
>>> On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
>>>> No path towards dev_forward_change (common ancestor of paths is in
>>>> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
>>>> We need to hold the lock during the whole loop in dev_forward_change.
>>>> __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
>>>> their function body. Since addrconf_{join,leave}_anycast call to
>>>> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
>>>> the responsibility of locking upwards.
>>>>
>>>> This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
>>>> place where the caller can directly acquire the idev->lock, that is in
>>>> ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
>>>> needs to be called under idev->lock, and thus it becomes the
>>>> responsibility of the callers of addrconf_leave_anycast to hold that
>>>> lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
>>>> ipv6_sock_ac_join, which can acquire the lock during the call to
>>>> __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
>>>> needs to be called under idev->lock, and thus it becomes the
>>>> responsibility of the callers of addrconf_join_anycast to hold that
>>>> lock.
>>>>
>>>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>  - Move the locking upwards
>>>>
>>>>  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
>>>>  net/ipv6/anycast.c  | 37 ++++++++++++++++---------------------
>>>>  2 files changed, 32 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>>> index f908e2fd30b2..69e9f81e2045 100644
>>>> --- a/net/ipv6/addrconf.c
>>>> +++ b/net/ipv6/addrconf.c
>>>> @@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>>>>  		}
>>>>  	}
>>>>  
>>>> +	write_lock_bh(&idev->lock);
>>>>  	list_for_each_entry(ifa, &idev->addr_list, if_list) {
>>>>  		if (ifa->flags&IFA_F_TENTATIVE)
>>>>  			continue;
>>>> @@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>>>>  		else
>>>>  			addrconf_leave_anycast(ifa);
>>>>  	}
>>>> +	write_unlock_bh(&idev->lock);
>>>>  	inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
>>>>  				     NETCONFA_FORWARDING,
>>>>  				     dev->ifindex, &idev->cnf);
>>>> @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
>>>>  	__ipv6_dev_mc_dec(idev, &maddr);
>>>>  }
>>>>  
>>>> -/* caller must hold RTNL */
>>>> +/* caller must hold RTNL and write lock idev->lock */
>>>>  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>>>>  {
>>>>  	struct in6_addr addr;
>>>> @@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>>>>  	__ipv6_dev_ac_inc(ifp->idev, &addr);
>>>>  }
>>>>  
>>>> -/* caller must hold RTNL */
>>>> +/* caller must hold RTNL and write lock idev->lock */
>>>>  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
>>>>  {
>>>>  	struct in6_addr addr;
>>>> @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>>>>  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
>>>>  			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
>>>>  		} else {
>>>> -			if (idev->cnf.forwarding)
>>>> +			if (idev->cnf.forwarding) {
>>>> +				write_lock_bh(&idev->lock);
>>>>  				addrconf_leave_anycast(ifa);
>>>> +				write_unlock_bh(&idev->lock);
>>>> +			}
>>>>  			addrconf_leave_solict(ifa->idev, &ifa->addr);
>>>>  		}
>>>>  
>>>> @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
>>>>  				&ifp->addr, ifp->idev->dev->name);
>>>>  		}
>>>>  
>>>> -		if (ifp->idev->cnf.forwarding)
>>>> +		if (ifp->idev->cnf.forwarding) {
>>>> +			write_lock_bh(&ifp->idev->lock);
>>>>  			addrconf_join_anycast(ifp);
>>>> +			write_unlock_bh(&ifp->idev->lock);
>>>> +		}
>>>>  		if (!ipv6_addr_any(&ifp->peer_addr))
>>>>  			addrconf_prefix_route(&ifp->peer_addr, 128,
>>>>  					      ifp->rt_priority, ifp->idev->dev,
>>>>  					      0, 0, GFP_ATOMIC);
>>>>  		break;
>>>>  	case RTM_DELADDR:
>>>> -		if (ifp->idev->cnf.forwarding)
>>>> +		if (ifp->idev->cnf.forwarding) {
>>>> +			write_lock_bh(&ifp->idev->lock);
>>>>  			addrconf_leave_anycast(ifp);
>>>> +			write_unlock_bh(&ifp->idev->lock);
>>>> +		}
>>>>  		addrconf_leave_solict(ifp->idev, &ifp->addr);
>>>>  		if (!ipv6_addr_any(&ifp->peer_addr)) {
>>>>  			struct fib6_info *rt;
>>>> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
>>>> index dacdea7fcb62..f3017ed6f005 100644
>>>> --- a/net/ipv6/anycast.c
>>>> +++ b/net/ipv6/anycast.c
>>>> @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>>>>  			goto error;
>>>>  	}
>>>>  
>>>> +	write_lock_bh(&idev->lock);
>>>>  	err = __ipv6_dev_ac_inc(idev, addr);
>>>> +	write_unlock_bh(&idev->lock);
>>>
>>> I feat this is problematic, due this call chain:
>>>
>>>  __ipv6_dev_ac_inc() -> addrconf_join_solict() -> ipv6_dev_mc_inc ->
>>> __ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
>>>
>>> The latter call requires process context.
>>>
>>> One alternarive (likely very hackish way) to solve this could be:
>>> - adding another list entry  into struct inet6_dev, rtnl protected.
>>
>> Typo above: the new field should be added to 'struct inet6_ifaddr'.
>>
>>> - traverse addr_list under idev->lock and add each entry with
>>> forwarding on to into a tmp list (e.g. tmp_join) using the field above;
>>> add the entries with forwarding off into another tmp list (e.g.
>>> tmp_leave), still using the same field.
>>
>> Again confusing text above, sorry. As the forwarding flag is per
>> device, all the addr entries will land into the same tmp list.
>>
>> It's probably better if I sketch up some code...
> 
> For the records, I mean something alongside the following - completely
> not tested:
> ---
> diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
> index 4cfdef6ca4f6..2df3c98b9e55 100644
> --- a/include/net/if_inet6.h
> +++ b/include/net/if_inet6.h
> @@ -64,6 +64,7 @@ struct inet6_ifaddr {
>  
>  	struct hlist_node	addr_lst;
>  	struct list_head	if_list;
> +	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 b22504176588..27d1081b693e 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -797,6 +797,7 @@ static void dev_forward_change(struct inet6_dev *idev)
>  {
>  	struct net_device *dev;
>  	struct inet6_ifaddr *ifa;
> +	LIST_HEAD(tmp);
>  
>  	if (!idev)
>  		return;
> @@ -815,9 +816,17 @@ static void dev_forward_change(struct inet6_dev *idev)
>  		}
>  	}
>  
> +	rcu_read_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);
> +	}
> +	rcu_read_unlock();
> +
> +	while (!list_empty(&tmp)) {
> +		ifa = list_first_entry(&tmp, struct inet6_ifaddr, if_list_aux);
> +		list_del(&ifa->if_list_aux);
>  		if (idev->cnf.forwarding)
>  			addrconf_join_anycast(ifa);
>  		else
> 

I see, nice small change.
Only thing I notice is that list_for_each_entry_rcu should be used instead of list_for_each_entry inside the rcu lock, right?

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Paolo Abeni]

On Fri, 2022-03-18 at 16:45 +0100, Niels Dossche wrote:
> On 18/03/2022 16:42, Paolo Abeni wrote:
> > On Fri, 2022-03-18 at 13:48 +0100, Paolo Abeni wrote:
> > > On Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
> > > > On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
> > > > > No path towards dev_forward_change (common ancestor of paths
> > > > > is in
> > > > > addrconf_fixup_forwarding) acquires idev->lock for idev-
> > > > > >addr_list.
> > > > > We need to hold the lock during the whole loop in
> > > > > dev_forward_change.
> > > > > __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev-
> > > > > >lock in
> > > > > their function body. Since addrconf_{join,leave}_anycast call
> > > > > to
> > > > > __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need
> > > > > to move
> > > > > the responsibility of locking upwards.
> > > > > 
> > > > > This patch moves the locking up. For __ipv6_dev_ac_dec, there
> > > > > is one
> > > > > place where the caller can directly acquire the idev->lock,
> > > > > that is in
> > > > > ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast,
> > > > > which now
> > > > > needs to be called under idev->lock, and thus it becomes the
> > > > > responsibility of the callers of addrconf_leave_anycast to
> > > > > hold that
> > > > > lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
> > > > > ipv6_sock_ac_join, which can acquire the lock during the call
> > > > > to
> > > > > __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast,
> > > > > which now
> > > > > needs to be called under idev->lock, and thus it becomes the
> > > > > responsibility of the callers of addrconf_join_anycast to
> > > > > hold that
> > > > > lock.
> > > > > 
> > > > > Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> > > > > ---
> > > > > 
> > > > > Changes in v2:
> > > > >  - Move the locking upwards
> > > > > 
> > > > >  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
> > > > >  net/ipv6/anycast.c  | 37 ++++++++++++++++-------------------
> > > > > --
> > > > >  2 files changed, 32 insertions(+), 26 deletions(-)
> > > > > 
> > > > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > > > > index f908e2fd30b2..69e9f81e2045 100644
> > > > > --- a/net/ipv6/addrconf.c
> > > > > +++ b/net/ipv6/addrconf.c
> > > > > @@ -818,6 +818,7 @@ static void dev_forward_change(struct
> > > > > inet6_dev *idev)
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	write_lock_bh(&idev->lock);
> > > > >  	list_for_each_entry(ifa, &idev->addr_list, if_list)
> > > > > {
> > > > >  		if (ifa->flags&IFA_F_TENTATIVE)
> > > > >  			continue;
> > > > > @@ -826,6 +827,7 @@ static void dev_forward_change(struct
> > > > > inet6_dev *idev)
> > > > >  		else
> > > > >  			addrconf_leave_anycast(ifa);
> > > > >  	}
> > > > > +	write_unlock_bh(&idev->lock);
> > > > >  	inet6_netconf_notify_devconf(dev_net(dev),
> > > > > RTM_NEWNETCONF,
> > > > >  				     NETCONFA_FORWARDING,
> > > > >  				     dev->ifindex, &idev-
> > > > > >cnf);
> > > > > @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct
> > > > > inet6_dev *idev, const struct in6_addr *addr)
> > > > >  	__ipv6_dev_mc_dec(idev, &maddr);
> > > > >  }
> > > > >  
> > > > > -/* caller must hold RTNL */
> > > > > +/* caller must hold RTNL and write lock idev->lock */
> > > > >  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> > > > >  {
> > > > >  	struct in6_addr addr;
> > > > > @@ -2204,7 +2206,7 @@ static void
> > > > > addrconf_join_anycast(struct inet6_ifaddr *ifp)
> > > > >  	__ipv6_dev_ac_inc(ifp->idev, &addr);
> > > > >  }
> > > > >  
> > > > > -/* caller must hold RTNL */
> > > > > +/* caller must hold RTNL and write lock idev->lock */
> > > > >  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
> > > > >  {
> > > > >  	struct in6_addr addr;
> > > > > @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct
> > > > > net_device *dev, bool unregister)
> > > > >  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
> > > > > 			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
> > > > >  		} else {
> > > > > -			if (idev->cnf.forwarding)
> > > > > +			if (idev->cnf.forwarding) {
> > > > > +				write_lock_bh(&idev->lock);
> > > > >  				addrconf_leave_anycast(ifa);
> > > > > +				write_unlock_bh(&idev-
> > > > > >lock);
> > > > > +			}
> > > > >  			addrconf_leave_solict(ifa->idev,
> > > > > &ifa->addr);
> > > > >  		}
> > > > >  
> > > > > @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int
> > > > > event, struct inet6_ifaddr *ifp)
> > > > >  				&ifp->addr, ifp->idev->dev-
> > > > > >name);
> > > > >  		}
> > > > >  
> > > > > -		if (ifp->idev->cnf.forwarding)
> > > > > +		if (ifp->idev->cnf.forwarding) {
> > > > > +			write_lock_bh(&ifp->idev->lock);
> > > > >  			addrconf_join_anycast(ifp);
> > > > > +			write_unlock_bh(&ifp->idev->lock);
> > > > > +		}
> > > > >  		if (!ipv6_addr_any(&ifp->peer_addr))
> > > > >  			addrconf_prefix_route(&ifp-
> > > > > >peer_addr, 128,
> > > > >  					      ifp-
> > > > > >rt_priority, ifp->idev->dev,
> > > > >  					      0, 0,
> > > > > GFP_ATOMIC);
> > > > >  		break;
> > > > >  	case RTM_DELADDR:
> > > > > -		if (ifp->idev->cnf.forwarding)
> > > > > +		if (ifp->idev->cnf.forwarding) {
> > > > > +			write_lock_bh(&ifp->idev->lock);
> > > > >  			addrconf_leave_anycast(ifp);
> > > > > +			write_unlock_bh(&ifp->idev->lock);
> > > > > +		}
> > > > >  		addrconf_leave_solict(ifp->idev, &ifp-
> > > > > >addr);
> > > > >  		if (!ipv6_addr_any(&ifp->peer_addr)) {
> > > > >  			struct fib6_info *rt;
> > > > > diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> > > > > index dacdea7fcb62..f3017ed6f005 100644
> > > > > --- a/net/ipv6/anycast.c
> > > > > +++ b/net/ipv6/anycast.c
> > > > > @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk,
> > > > > int ifindex, const struct in6_addr *addr)
> > > > >  			goto error;
> > > > >  	}
> > > > >  
> > > > > +	write_lock_bh(&idev->lock);
> > > > >  	err = __ipv6_dev_ac_inc(idev, addr);
> > > > > +	write_unlock_bh(&idev->lock);
> > > > 
> > > > I feat this is problematic, due this call chain:
> > > > 
> > > >  __ipv6_dev_ac_inc() -> addrconf_join_solict() ->
> > > > ipv6_dev_mc_inc ->
> > > > __ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
> > > > 
> > > > The latter call requires process context.
> > > > 
> > > > One alternarive (likely very hackish way) to solve this could
> > > > be:
> > > > - adding another list entry  into struct inet6_dev, rtnl
> > > > protected.
> > > 
> > > Typo above: the new field should be added to 'struct
> > > inet6_ifaddr'.
> > > 
> > > > - traverse addr_list under idev->lock and add each entry with
> > > > forwarding on to into a tmp list (e.g. tmp_join) using the
> > > > field above;
> > > > add the entries with forwarding off into another tmp list (e.g.
> > > > tmp_leave), still using the same field.
> > > 
> > > Again confusing text above, sorry. As the forwarding flag is per
> > > device, all the addr entries will land into the same tmp list.
> > > 
> > > It's probably better if I sketch up some code...
> > 
> > For the records, I mean something alongside the following -
> > completely
> > not tested:
> > ---
> > diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
> > index 4cfdef6ca4f6..2df3c98b9e55 100644
> > --- a/include/net/if_inet6.h
> > +++ b/include/net/if_inet6.h
> > @@ -64,6 +64,7 @@ struct inet6_ifaddr {
> >  
> >  	struct hlist_node	addr_lst;
> >  	struct list_head	if_list;
> > +	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 b22504176588..27d1081b693e 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -797,6 +797,7 @@ static void dev_forward_change(struct inet6_dev
> > *idev)
> >  {
> >  	struct net_device *dev;
> >  	struct inet6_ifaddr *ifa;
> > +	LIST_HEAD(tmp);
> >  
> >  	if (!idev)
> >  		return;
> > @@ -815,9 +816,17 @@ static void dev_forward_change(struct
> > inet6_dev *idev)
> >  		}
> >  	}
> >  
> > +	rcu_read_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);
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	while (!list_empty(&tmp)) {
> > +		ifa = list_first_entry(&tmp, struct inet6_ifaddr,
> > if_list_aux);
> > +		list_del(&ifa->if_list_aux);
> >  		if (idev->cnf.forwarding)
> >  			addrconf_join_anycast(ifa);
> >  		else
> > 
> 
> I see, nice small change.
> Only thing I notice is that list_for_each_entry_rcu should be used
> instead of list_for_each_entry inside the rcu lock, right?

Yes you are right. Or you can replace rcu_read_lock() with
write_lock_bh(&idev->lock).

Probably we need some commend nearby if_list_aux definition alike "used
to safely traverse the idev address list in process context, see
dev_forward_change"

Cheers,

Paolo

 

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Niels Dossche]

On 3/18/22 17:50, Paolo Abeni wrote:
> On Fri, 2022-03-18 at 16:45 +0100, Niels Dossche wrote:
>> On 18/03/2022 16:42, Paolo Abeni wrote:
>>> On Fri, 2022-03-18 at 13:48 +0100, Paolo Abeni wrote:
>>>> On Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
>>>>> On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
>>>>>> No path towards dev_forward_change (common ancestor of paths
>>>>>> is in
>>>>>> addrconf_fixup_forwarding) acquires idev->lock for idev-
>>>>>>> addr_list.
>>>>>> We need to hold the lock during the whole loop in
>>>>>> dev_forward_change.
>>>>>> __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev-
>>>>>>> lock in
>>>>>> their function body. Since addrconf_{join,leave}_anycast call
>>>>>> to
>>>>>> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need
>>>>>> to move
>>>>>> the responsibility of locking upwards.
>>>>>>
>>>>>> This patch moves the locking up. For __ipv6_dev_ac_dec, there
>>>>>> is one
>>>>>> place where the caller can directly acquire the idev->lock,
>>>>>> that is in
>>>>>> ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast,
>>>>>> which now
>>>>>> needs to be called under idev->lock, and thus it becomes the
>>>>>> responsibility of the callers of addrconf_leave_anycast to
>>>>>> hold that
>>>>>> lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
>>>>>> ipv6_sock_ac_join, which can acquire the lock during the call
>>>>>> to
>>>>>> __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast,
>>>>>> which now
>>>>>> needs to be called under idev->lock, and thus it becomes the
>>>>>> responsibility of the callers of addrconf_join_anycast to
>>>>>> hold that
>>>>>> lock.
>>>>>>
>>>>>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>>  - Move the locking upwards
>>>>>>
>>>>>>  net/ipv6/addrconf.c | 21 ++++++++++++++++-----
>>>>>>  net/ipv6/anycast.c  | 37 ++++++++++++++++-------------------
>>>>>> --
>>>>>>  2 files changed, 32 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>>>>> index f908e2fd30b2..69e9f81e2045 100644
>>>>>> --- a/net/ipv6/addrconf.c
>>>>>> +++ b/net/ipv6/addrconf.c
>>>>>> @@ -818,6 +818,7 @@ static void dev_forward_change(struct
>>>>>> inet6_dev *idev)
>>>>>>  		}
>>>>>>  	}
>>>>>>  
>>>>>> +	write_lock_bh(&idev->lock);
>>>>>>  	list_for_each_entry(ifa, &idev->addr_list, if_list)
>>>>>> {
>>>>>>  		if (ifa->flags&IFA_F_TENTATIVE)
>>>>>>  			continue;
>>>>>> @@ -826,6 +827,7 @@ static void dev_forward_change(struct
>>>>>> inet6_dev *idev)
>>>>>>  		else
>>>>>>  			addrconf_leave_anycast(ifa);
>>>>>>  	}
>>>>>> +	write_unlock_bh(&idev->lock);
>>>>>>  	inet6_netconf_notify_devconf(dev_net(dev),
>>>>>> RTM_NEWNETCONF,
>>>>>>  				     NETCONFA_FORWARDING,
>>>>>>  				     dev->ifindex, &idev-
>>>>>>> cnf);
>>>>>> @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct
>>>>>> inet6_dev *idev, const struct in6_addr *addr)
>>>>>>  	__ipv6_dev_mc_dec(idev, &maddr);
>>>>>>  }
>>>>>>  
>>>>>> -/* caller must hold RTNL */
>>>>>> +/* caller must hold RTNL and write lock idev->lock */
>>>>>>  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>>>>>>  {
>>>>>>  	struct in6_addr addr;
>>>>>> @@ -2204,7 +2206,7 @@ static void
>>>>>> addrconf_join_anycast(struct inet6_ifaddr *ifp)
>>>>>>  	__ipv6_dev_ac_inc(ifp->idev, &addr);
>>>>>>  }
>>>>>>  
>>>>>> -/* caller must hold RTNL */
>>>>>> +/* caller must hold RTNL and write lock idev->lock */
>>>>>>  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
>>>>>>  {
>>>>>>  	struct in6_addr addr;
>>>>>> @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct
>>>>>> net_device *dev, bool unregister)
>>>>>>  			__ipv6_ifa_notify(RTM_DELADDR, ifa);
>>>>>> 			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
>>>>>>  		} else {
>>>>>> -			if (idev->cnf.forwarding)
>>>>>> +			if (idev->cnf.forwarding) {
>>>>>> +				write_lock_bh(&idev->lock);
>>>>>>  				addrconf_leave_anycast(ifa);
>>>>>> +				write_unlock_bh(&idev-
>>>>>>> lock);
>>>>>> +			}
>>>>>>  			addrconf_leave_solict(ifa->idev,
>>>>>> &ifa->addr);
>>>>>>  		}
>>>>>>  
>>>>>> @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int
>>>>>> event, struct inet6_ifaddr *ifp)
>>>>>>  				&ifp->addr, ifp->idev->dev-
>>>>>>> name);
>>>>>>  		}
>>>>>>  
>>>>>> -		if (ifp->idev->cnf.forwarding)
>>>>>> +		if (ifp->idev->cnf.forwarding) {
>>>>>> +			write_lock_bh(&ifp->idev->lock);
>>>>>>  			addrconf_join_anycast(ifp);
>>>>>> +			write_unlock_bh(&ifp->idev->lock);
>>>>>> +		}
>>>>>>  		if (!ipv6_addr_any(&ifp->peer_addr))
>>>>>>  			addrconf_prefix_route(&ifp-
>>>>>>> peer_addr, 128,
>>>>>>  					      ifp-
>>>>>>> rt_priority, ifp->idev->dev,
>>>>>>  					      0, 0,
>>>>>> GFP_ATOMIC);
>>>>>>  		break;
>>>>>>  	case RTM_DELADDR:
>>>>>> -		if (ifp->idev->cnf.forwarding)
>>>>>> +		if (ifp->idev->cnf.forwarding) {
>>>>>> +			write_lock_bh(&ifp->idev->lock);
>>>>>>  			addrconf_leave_anycast(ifp);
>>>>>> +			write_unlock_bh(&ifp->idev->lock);
>>>>>> +		}
>>>>>>  		addrconf_leave_solict(ifp->idev, &ifp-
>>>>>>> addr);
>>>>>>  		if (!ipv6_addr_any(&ifp->peer_addr)) {
>>>>>>  			struct fib6_info *rt;
>>>>>> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
>>>>>> index dacdea7fcb62..f3017ed6f005 100644
>>>>>> --- a/net/ipv6/anycast.c
>>>>>> +++ b/net/ipv6/anycast.c
>>>>>> @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk,
>>>>>> int ifindex, const struct in6_addr *addr)
>>>>>>  			goto error;
>>>>>>  	}
>>>>>>  
>>>>>> +	write_lock_bh(&idev->lock);
>>>>>>  	err = __ipv6_dev_ac_inc(idev, addr);
>>>>>> +	write_unlock_bh(&idev->lock);
>>>>>
>>>>> I feat this is problematic, due this call chain:
>>>>>
>>>>>  __ipv6_dev_ac_inc() -> addrconf_join_solict() ->
>>>>> ipv6_dev_mc_inc ->
>>>>> __ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
>>>>>
>>>>> The latter call requires process context.
>>>>>
>>>>> One alternarive (likely very hackish way) to solve this could
>>>>> be:
>>>>> - adding another list entry  into struct inet6_dev, rtnl
>>>>> protected.
>>>>
>>>> Typo above: the new field should be added to 'struct
>>>> inet6_ifaddr'.
>>>>
>>>>> - traverse addr_list under idev->lock and add each entry with
>>>>> forwarding on to into a tmp list (e.g. tmp_join) using the
>>>>> field above;
>>>>> add the entries with forwarding off into another tmp list (e.g.
>>>>> tmp_leave), still using the same field.
>>>>
>>>> Again confusing text above, sorry. As the forwarding flag is per
>>>> device, all the addr entries will land into the same tmp list.
>>>>
>>>> It's probably better if I sketch up some code...
>>>
>>> For the records, I mean something alongside the following -
>>> completely
>>> not tested:
>>> ---
>>> diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
>>> index 4cfdef6ca4f6..2df3c98b9e55 100644
>>> --- a/include/net/if_inet6.h
>>> +++ b/include/net/if_inet6.h
>>> @@ -64,6 +64,7 @@ struct inet6_ifaddr {
>>>  
>>>  	struct hlist_node	addr_lst;
>>>  	struct list_head	if_list;
>>> +	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 b22504176588..27d1081b693e 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -797,6 +797,7 @@ static void dev_forward_change(struct inet6_dev
>>> *idev)
>>>  {
>>>  	struct net_device *dev;
>>>  	struct inet6_ifaddr *ifa;
>>> +	LIST_HEAD(tmp);
>>>  
>>>  	if (!idev)
>>>  		return;
>>> @@ -815,9 +816,17 @@ static void dev_forward_change(struct
>>> inet6_dev *idev)
>>>  		}
>>>  	}
>>>  
>>> +	rcu_read_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);
>>> +	}
>>> +	rcu_read_unlock();
>>> +
>>> +	while (!list_empty(&tmp)) {
>>> +		ifa = list_first_entry(&tmp, struct inet6_ifaddr,
>>> if_list_aux);
>>> +		list_del(&ifa->if_list_aux);
>>>  		if (idev->cnf.forwarding)
>>>  			addrconf_join_anycast(ifa);
>>>  		else
>>>
>>
>> I see, nice small change.
>> Only thing I notice is that list_for_each_entry_rcu should be used
>> instead of list_for_each_entry inside the rcu lock, right?
> 
> Yes you are right. Or you can replace rcu_read_lock() with
> write_lock_bh(&idev->lock).
> 
> Probably we need some commend nearby if_list_aux definition alike "used
> to safely traverse the idev address list in process context, see
> dev_forward_change"
> 
> Cheers,
> 
> Paolo
> 
>  
> 

Hi,
I have an additional question about the locks on the addr_list actually.
In addrconf_ifdown, there's a loop on addr_list within a write lock in idev->lock
> list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list)
The loop body unlocks the idev->lock and reacquires it later. I assume because of the lock dependency on ifa->lock and the calls that acquire the mc_lock? Shouldn't that list iteration also be protected during the whole iteration?

Thanks
Niels

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[David Ahern]

On 3/19/22 7:17 AM, Niels Dossche wrote:
> I have an additional question about the locks on the addr_list actually.
> In addrconf_ifdown, there's a loop on addr_list within a write lock in idev->lock
>> list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list)
> The loop body unlocks the idev->lock and reacquires it later. I assume because of the lock dependency on ifa->lock and the calls that acquire the mc_lock? Shouldn't that list iteration also be protected during the whole iteration?
> 


That loop needs to be improved as well. Locking in ipv6 code is a bit
hairy.

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Paolo Abeni]

On Mon, 2022-03-21 at 09:42 -0600, David Ahern wrote:
> On 3/19/22 7:17 AM, Niels Dossche wrote:
> > I have an additional question about the locks on the addr_list actually.
> > In addrconf_ifdown, there's a loop on addr_list within a write lock in idev->lock
> > > list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list)
> > The loop body unlocks the idev->lock and reacquires it later. I assume because of the lock dependency on ifa->lock and the calls that acquire the mc_lock? Shouldn't that list iteration also be protected during the whole iteration?
> > 
> 
> 
> That loop needs to be improved as well. Locking in ipv6 code is a bit
> hairy.

I *think* we could re-use the if_list_aux trick: create a tmp list
under idev->lock using ifa->if_list_aux and traverse (still using the
_safe variant) such list with no lock.

Still in addrconf_ifdown(), there is a similar loop for
'tempaddr_list'.

In the latter case I think we could splice the idev->lock protected
list into a tmp one and traverse the latter with no lock held.

@Niels: could you look at that, too?

Thanks!

Paolo

Re: [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change

[Niels Dossche]

On 21/03/2022 19:15, Paolo Abeni wrote:
> On Mon, 2022-03-21 at 09:42 -0600, David Ahern wrote:
>> On 3/19/22 7:17 AM, Niels Dossche wrote:
>>> I have an additional question about the locks on the addr_list actually.
>>> In addrconf_ifdown, there's a loop on addr_list within a write lock in idev->lock
>>>> list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list)
>>> The loop body unlocks the idev->lock and reacquires it later. I assume because of the lock dependency on ifa->lock and the calls that acquire the mc_lock? Shouldn't that list iteration also be protected during the whole iteration?
>>>
>>
>>
>> That loop needs to be improved as well. Locking in ipv6 code is a bit
>> hairy.
> 
> I *think* we could re-use the if_list_aux trick: create a tmp list
> under idev->lock using ifa->if_list_aux and traverse (still using the
> _safe variant) such list with no lock.
> 

This sounds like a good plan.

> Still in addrconf_ifdown(), there is a similar loop for
> 'tempaddr_list'.
> 
> In the latter case I think we could splice the idev->lock protected
> list into a tmp one and traverse the latter with no lock held.
> 
> @Niels: could you look at that, too?

I will be able to look into this tomorrow in more detail, I'll try then to create a patch series.

> 
> Thanks!
> 
> Paolo
> 

Cheers
Niels