Re: [PATCH net 2/3] ipv6: mcast: Fix potential UAF in MLD delayed work

[Ido Schimmel <idosch@nvidia.com>]

On Sat, Jul 04, 2026 at 07:43:45PM +0000, Eric Dumazet wrote:
> A race condition exists between device teardown and incoming MLD query
> processing, leading to a Use-After-Free in the MLD delayed work.
> 
> During device destruction, the primary reference to inet6_dev is dropped,
> which can drop its refcount to 0. The actual freeing of inet6_dev memory
> is deferred via RCU.
> 
> Concurrently, the packet receive path runs under RCU read lock and obtains
> the inet6_dev pointer. Because the memory is RCU-protected, CPU-0 can
> safely dereference inet6_dev even if its refcount has hit 0.
> 
> However, if CPU-0 calls igmp6_event_query() and schedules delayed work, it
> attempts to acquire a reference using in6_dev_hold(). This increments the
> refcount from 0 to 1, triggering a "refcount_t: addition on 0" warning.
> Since the inet6_dev memory is still scheduled to be freed after the RCU
> grace period, the device is freed while the work is still scheduled.
> When the work runs, it accesses the freed memory, causing a kernel panic.
> 
> Fix this by using refcount_inc_not_zero() (via a new helper
> in6_dev_hold_safe()) to prevent acquiring a reference if the device is
> already being destroyed. If the refcount is 0, we do not schedule the work.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Eric, thanks for taking care of this!

> ---
>  include/net/addrconf.h |  5 +++++
>  net/ipv6/mcast.c       | 38 ++++++++++++++++++++++++++++----------
>  2 files changed, 33 insertions(+), 10 deletions(-)

[...]

> @@ -1395,6 +1401,7 @@ static void mld_process_v2(struct inet6_dev *idev, struct mld2_query *mld,
>  void igmp6_event_query(struct sk_buff *skb)
>  {
>  	struct inet6_dev *idev = __in6_dev_get(skb->dev);
> +	bool put = false;
>  
>  	if (!idev || idev->dead)
>  		goto out;
> @@ -1402,11 +1409,16 @@ void igmp6_event_query(struct sk_buff *skb)
>  	spin_lock_bh(&idev->mc_query_lock);
>  	if (skb_queue_len(&idev->mc_query_queue) < MLD_MAX_SKBS) {
>  		__skb_queue_tail(&idev->mc_query_queue, skb);

Shouldn't we only enqueue the skb if we managed to take a reference?
Something like [1] (on top of this patch).

If we failed to take a reference, then ipv6_mc_destroy_dev() already ran
and the queue will not be purged, thereby leaking this skb.

> -		if (!mod_delayed_work(mld_wq, &idev->mc_query_work, 0))
> -			in6_dev_hold(idev);
> +		if (in6_dev_hold_safe(idev)) {
> +			if (mod_delayed_work(mld_wq, &idev->mc_query_work, 0))
> +				put = true;
> +		}
>  		skb = NULL;
>  	}
>  	spin_unlock_bh(&idev->mc_query_lock);
> +
> +	if (put)
> +		in6_dev_put(idev);
>  out:
>  	kfree_skb(skb);
>  }
> @@ -1570,6 +1582,7 @@ static void mld_query_work(struct work_struct *work)
>  void igmp6_event_report(struct sk_buff *skb)
>  {
>  	struct inet6_dev *idev = __in6_dev_get(skb->dev);
> +	bool put = false;
>  
>  	if (!idev || idev->dead)
>  		goto out;
> @@ -1577,11 +1590,16 @@ void igmp6_event_report(struct sk_buff *skb)
>  	spin_lock_bh(&idev->mc_report_lock);
>  	if (skb_queue_len(&idev->mc_report_queue) < MLD_MAX_SKBS) {
>  		__skb_queue_tail(&idev->mc_report_queue, skb);

Same here

> -		if (!mod_delayed_work(mld_wq, &idev->mc_report_work, 0))
> -			in6_dev_hold(idev);
> +		if (in6_dev_hold_safe(idev)) {
> +			if (mod_delayed_work(mld_wq, &idev->mc_report_work, 0))
> +				put = true;
> +		}
>  		skb = NULL;
>  	}
>  	spin_unlock_bh(&idev->mc_report_lock);
> +
> +	if (put)
> +		in6_dev_put(idev);
>  out:
>  	kfree_skb(skb);
>  }

[1]
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 12d22de6f496..aaba4c2aae23 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1408,12 +1408,11 @@ void igmp6_event_query(struct sk_buff *skb)
 		goto out;
 
 	spin_lock_bh(&idev->mc_query_lock);
-	if (skb_queue_len(&idev->mc_query_queue) < MLD_MAX_SKBS) {
+	if (skb_queue_len(&idev->mc_query_queue) < MLD_MAX_SKBS &&
+	    in6_dev_hold_safe(idev)) {
 		__skb_queue_tail(&idev->mc_query_queue, skb);
-		if (in6_dev_hold_safe(idev)) {
-			if (mod_delayed_work(mld_wq, &idev->mc_query_work, 0))
-				put = true;
-		}
+		if (mod_delayed_work(mld_wq, &idev->mc_query_work, 0))
+			put = true;
 		skb = NULL;
 	}
 	spin_unlock_bh(&idev->mc_query_lock);
@@ -1589,12 +1588,11 @@ void igmp6_event_report(struct sk_buff *skb)
 		goto out;
 
 	spin_lock_bh(&idev->mc_report_lock);
-	if (skb_queue_len(&idev->mc_report_queue) < MLD_MAX_SKBS) {
+	if (skb_queue_len(&idev->mc_report_queue) < MLD_MAX_SKBS &&
+	    in6_dev_hold_safe(idev)) {
 		__skb_queue_tail(&idev->mc_report_queue, skb);
-		if (in6_dev_hold_safe(idev)) {
-			if (mod_delayed_work(mld_wq, &idev->mc_report_work, 0))
-				put = true;
-		}
+		if (mod_delayed_work(mld_wq, &idev->mc_report_work, 0))
+			put = true;
 		skb = NULL;
 	}
 	spin_unlock_bh(&idev->mc_report_lock);