[PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[Qi Tang]

xfrm_trans_queue() queues transport-mode packets for async reinject via
xfrm_trans_reinject(). The queued skb may still be reinjected after the
originating device teardown has started.

Keep the device alive across the async reinject window by taking a netdev
reference when queueing the skb and dropping it after the reinject callback
completes.

Fixes: acf568ee859f ("xfrm: Reinject transport-mode packets through tasklet")
Cc: stable@vger.kernel.org
Signed-off-by: Qi Tang <tpluszz77@gmail.com>
---
 net/xfrm/xfrm_input.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 4ed346e682c7..4b5147cb44b7 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -40,6 +40,7 @@ struct xfrm_trans_cb {
 	} header;
 	int (*finish)(struct net *net, struct sock *sk, struct sk_buff *skb);
 	struct net *net;
+	struct net_device *dev;
 };
 
 #define XFRM_TRANS_SKB_CB(__skb) ((struct xfrm_trans_cb *)&((__skb)->cb[0]))
@@ -784,9 +785,13 @@ static void xfrm_trans_reinject(struct work_struct *work)
 	spin_unlock_bh(&trans->queue_lock);
 
 	local_bh_disable();
-	while ((skb = __skb_dequeue(&queue)))
-		XFRM_TRANS_SKB_CB(skb)->finish(XFRM_TRANS_SKB_CB(skb)->net,
-					       NULL, skb);
+	while ((skb = __skb_dequeue(&queue))) {
+		struct xfrm_trans_cb *cb = XFRM_TRANS_SKB_CB(skb);
+		struct net_device *dev = cb->dev;
+
+		cb->finish(cb->net, NULL, skb);
+		dev_put(dev);
+	}
 	local_bh_enable();
 }
 
@@ -805,6 +810,8 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
 
 	XFRM_TRANS_SKB_CB(skb)->finish = finish;
 	XFRM_TRANS_SKB_CB(skb)->net = net;
+	XFRM_TRANS_SKB_CB(skb)->dev = skb->dev;
+	dev_hold(XFRM_TRANS_SKB_CB(skb)->dev);
 	spin_lock_bh(&trans->queue_lock);
 	__skb_queue_tail(&trans->queue, skb);
 	spin_unlock_bh(&trans->queue_lock);
-- 
2.43.0

Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[Steffen Klassert]

On Fri, Mar 20, 2026 at 03:30:23PM +0800, Qi Tang wrote:
> xfrm_trans_queue() queues transport-mode packets for async reinject via
> xfrm_trans_reinject(). The queued skb may still be reinjected after the
> originating device teardown has started.
> 
> Keep the device alive across the async reinject window by taking a netdev
> reference when queueing the skb and dropping it after the reinject callback
> completes.
> 
> Fixes: acf568ee859f ("xfrm: Reinject transport-mode packets through tasklet")
> Cc: stable@vger.kernel.org
> Signed-off-by: Qi Tang <tpluszz77@gmail.com>
> ---
>  net/xfrm/xfrm_input.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 4ed346e682c7..4b5147cb44b7 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -40,6 +40,7 @@ struct xfrm_trans_cb {
>  	} header;
>  	int (*finish)(struct net *net, struct sock *sk, struct sk_buff *skb);
>  	struct net *net;
> +	struct net_device *dev;
>  };
>  
>  #define XFRM_TRANS_SKB_CB(__skb) ((struct xfrm_trans_cb *)&((__skb)->cb[0]))
> @@ -784,9 +785,13 @@ static void xfrm_trans_reinject(struct work_struct *work)
>  	spin_unlock_bh(&trans->queue_lock);
>  
>  	local_bh_disable();
> -	while ((skb = __skb_dequeue(&queue)))
> -		XFRM_TRANS_SKB_CB(skb)->finish(XFRM_TRANS_SKB_CB(skb)->net,
> -					       NULL, skb);
> +	while ((skb = __skb_dequeue(&queue))) {
> +		struct xfrm_trans_cb *cb = XFRM_TRANS_SKB_CB(skb);
> +		struct net_device *dev = cb->dev;
> +
> +		cb->finish(cb->net, NULL, skb);
> +		dev_put(dev);
> +	}
>  	local_bh_enable();
>  }
>  
> @@ -805,6 +810,8 @@ int xfrm_trans_queue_net(struct net *net, struct sk_buff *skb,
>  
>  	XFRM_TRANS_SKB_CB(skb)->finish = finish;
>  	XFRM_TRANS_SKB_CB(skb)->net = net;
> +	XFRM_TRANS_SKB_CB(skb)->dev = skb->dev;
> +	dev_hold(XFRM_TRANS_SKB_CB(skb)->dev);

While this looks correct, we take and drop yet another reference
here. This is an expensive operation. We do dev_hold already
in xfrm_input and it seems we just drop it too early.

Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[steffen-ai]

Subject: Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject
In-Reply-To: <20260320073023.21873-1-tpluszz77@gmail.com>
To: Qi Tang <tpluszz77@gmail.com>
Cc: netdev@vger.kernel.org, steffen.klassert@secunet.com, herbert@gondor.apana.org.au, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, stable@vger.kernel.org

The fix looks correct. A few comments:

1. The approach of holding a netdev reference at enqueue time and
   releasing it after async reinject properly addresses the UAF window.
   The ordering in xfrm_trans_reinject() is key - caching the dev
   pointer before calling finish() avoids touching skb->cb after a
   callback that may consume/free the skb.

2. One minor note: the net tree generally prefers netdev_hold() /
   netdev_put() for reference tracking, but dev_hold() / dev_put()
   is functionally equivalent here.

3. The BUILD_BUG_ON() in xfrm_trans_queue_net() will catch any cb
   size overflow, so that's covered.

No functional issues observed.

Reviewed-by: Steffen Klassert <steffen.klassert@secunet.com>

Model: openai-codex/gpt-5.3-codex

Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[Steffen Klassert]

Please ignore this. It was sent accidentally to the list.

Sorry.

On Fri, Mar 20, 2026 at 08:32:12AM +0000, steffen-ai@klassert.de wrote:
> Subject: Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject
> In-Reply-To: <20260320073023.21873-1-tpluszz77@gmail.com>
> To: Qi Tang <tpluszz77@gmail.com>
> Cc: netdev@vger.kernel.org, steffen.klassert@secunet.com, herbert@gondor.apana.org.au, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, stable@vger.kernel.org

...

Re: [PATCH net] xfrm: hold skb->dev across async IPv6 transport reinject

[Qi Tang]

I reworked the fix direction to avoid taking another netdev reference.

The current idea is to reuse the existing dev_hold() in xfrm_input(),
carry that reference across the async transport reinject path, and drop
it only once the skb is either consumed earlier or the async reinject
callback has completed.

Very roughly, the change would look like this:

  - in xfrm_input(), do not drop the existing dev reference immediately
    on async resume
  - add a dedicated queueing path for skbs that already carry that
    input-side reference
  - in transport_finish(), switch from NF_HOOK() to nf_hook() so the
    ret != 1 path can drop the carried reference when the skb is consumed
    before okfn runs
  - in xfrm_trans_reinject(), drop the transferred reference after the
    callback completes

Something along these lines:

  diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
  @@
        if (encap_type == -1) {
                async = 1;
  -             dev_put(skb->dev);
                seq = XFRM_SKB_CB(skb)->seq.input.low;
                ...
        }

  @@
  +int xfrm_trans_queue_in(struct net *net, struct sk_buff *skb, ...)
  +{
  +       ...
  +       XFRM_TRANS_SKB_CB(skb)->dev_ref_held = true;
  +       ...
  +}

  diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
  @@
  -     NF_HOOK(..., xfrm6_transport_finish2);
  +     ret = nf_hook(..., async ? xfrm6_transport_finish2_async :
  +                            xfrm6_transport_finish2);
  +     if (ret != 1) {
  +             if (async)
  +                     dev_put(dev);
  +             return 0;
  +     }
  +     (async ? xfrm6_transport_finish2_async :
  +              xfrm6_transport_finish2)(net, NULL, skb);

  diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
  @@
        while ((skb = __skb_dequeue(&queue))) {
  +             bool dev_ref_held = cb->dev_ref_held;
  +             struct net_device *dev = dev_ref_held ? skb->dev : NULL;
                cb->finish(cb->net, NULL, skb);
  +             if (dev_ref_held)
  +                     dev_put(dev);
        }

Does this sound like a reasonable direction for a v2?