[PATCH net v4] ipv6: addrconf: skip ERRDAD transition when address already DEAD

Linmao Li posted 1 patch 1 month, 2 weeks ago
net/ipv6/addrconf.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
[PATCH net v4] ipv6: addrconf: skip ERRDAD transition when address already DEAD
Posted by Linmao Li 1 month, 2 weeks ago
addrconf_dad_failure() transitions ifp->state from DAD to POSTDAD
via addrconf_dad_end(), which drops ifp->lock on return.  The lock
is re-acquired after net_info_ratelimited().  A concurrent
ipv6_del_addr() can take the lock in that window, set ifp->state
to DEAD and run list_del_rcu(&ifp->if_list).

addrconf_dad_failure() then overwrites DEAD with ERRDAD at errdad:
and schedules a new dad_work.  The work calls ipv6_del_addr()
again, hitting the already-poisoned list entry:

  general protection fault: 0000 [#1] SMP NOPTI
  CPU: 4 PID: 217 Comm: kworker/4:1
  Workqueue: ipv6_addrconf addrconf_dad_work
  RIP: 0010:ipv6_del_addr+0xe9/0x280
  RAX: dead000000000122
  Call Trace:
   addrconf_dad_stop+0x113/0x140
   addrconf_dad_work+0x28c/0x430
   process_one_work+0x1eb/0x3b0
   worker_thread+0x4d/0x400
   kthread+0x104/0x140
   ret_from_fork+0x35/0x40

Fold the addrconf_dad_end() logic into addrconf_dad_failure()
under a single ifp->lock critical section.  The STABLE_PRIVACY
branch temporarily drops ifp->lock around address regeneration,
so add a state-is-DEAD bail-out right after the lock is re-taken
at lock_errdad: for that remaining window.

Fixes: c15b1ccadb32 ("ipv6: move DAD and addrconf_verify processing to workqueue")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 net/ipv6/addrconf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 5476b6536eb7..b58bd9f11606 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2166,16 +2166,18 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
 	struct net *net = dev_net(idev->dev);
 	int max_addresses;
 
-	if (addrconf_dad_end(ifp)) {
+	spin_lock_bh(&ifp->lock);
+
+	if (ifp->state != INET6_IFADDR_STATE_DAD) {
+		spin_unlock_bh(&ifp->lock);
 		in6_ifa_put(ifp);
 		return;
 	}
+	ifp->state = INET6_IFADDR_STATE_POSTDAD;
 
 	net_info_ratelimited("%s: IPv6 duplicate address %pI6c used by %pM detected!\n",
 			     ifp->idev->dev->name, &ifp->addr, eth_hdr(skb)->h_source);
 
-	spin_lock_bh(&ifp->lock);
-
 	if (ifp->flags & IFA_F_STABLE_PRIVACY) {
 		struct in6_addr new_addr;
 		struct inet6_ifaddr *ifp2;
@@ -2223,6 +2225,11 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
 		in6_ifa_put(ifp2);
 lock_errdad:
 		spin_lock_bh(&ifp->lock);
+		if (ifp->state == INET6_IFADDR_STATE_DEAD) {
+			spin_unlock_bh(&ifp->lock);
+			in6_ifa_put(ifp);
+			return;
+		}
 	}
 
 errdad:
-- 
2.25.1
Re: [PATCH net v4] ipv6: addrconf: skip ERRDAD transition when address already DEAD
Posted by Ido Schimmel 1 month, 2 weeks ago
On Wed, Apr 29, 2026 at 09:26:51AM +0800, Linmao Li wrote:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 5476b6536eb7..b58bd9f11606 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2166,16 +2166,18 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
>  	struct net *net = dev_net(idev->dev);
>  	int max_addresses;
>  
> -	if (addrconf_dad_end(ifp)) {
> +	spin_lock_bh(&ifp->lock);
> +
> +	if (ifp->state != INET6_IFADDR_STATE_DAD) {
> +		spin_unlock_bh(&ifp->lock);
>  		in6_ifa_put(ifp);
>  		return;
>  	}
> +	ifp->state = INET6_IFADDR_STATE_POSTDAD;
>  
>  	net_info_ratelimited("%s: IPv6 duplicate address %pI6c used by %pM detected!\n",
>  			     ifp->idev->dev->name, &ifp->addr, eth_hdr(skb)->h_source);
>  
> -	spin_lock_bh(&ifp->lock);
> -
>  	if (ifp->flags & IFA_F_STABLE_PRIVACY) {
>  		struct in6_addr new_addr;
>  		struct inet6_ifaddr *ifp2;
> @@ -2223,6 +2225,11 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
>  		in6_ifa_put(ifp2);
>  lock_errdad:
>  		spin_lock_bh(&ifp->lock);
> +		if (ifp->state == INET6_IFADDR_STATE_DEAD) {

The code below expects the state to be POSTDAD, so wouldn't it be more
robust to check for anything but POSTDAD rather than specifically
checking for DEAD? Otherwise we risk overwriting a state that was set
while we weren't holding the lock.

Also:

1. Please post new versions in a new thread:

https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review

2. Please include a change log:

https://docs.kernel.org/process/maintainer-netdev.html#changes-requested

> +			spin_unlock_bh(&ifp->lock);
> +			in6_ifa_put(ifp);
> +			return;
> +		}
>  	}
>  
>  errdad:
> -- 
> 2.25.1
>