[PATCH] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

[PATCH] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

[Tristan Madani]

iptfs_destroy_state() calls hrtimer_cancel() while holding a spinlock
that the timer callback also acquires, leading to an ABBA deadlock on
SMP systems.

For the output timer (iptfs_timer):
  - iptfs_destroy_state() holds x->lock, calls hrtimer_cancel()
  - iptfs_delay_timer() callback takes x->lock

For the drop timer (drop_timer):
  - iptfs_destroy_state() holds drop_lock, calls hrtimer_cancel()
  - iptfs_drop_timer() callback takes drop_lock

Both timers use HRTIMER_MODE_REL_SOFT, so their callbacks run in softirq
context.  When hrtimer_cancel() is called for a soft timer that is
currently executing on another CPU, hrtimer_cancel_wait_running() spins
on softirq_expiry_lock -- the same lock held by the softirq running the
callback.  If the callback is blocked waiting for the spinlock held by
the caller of hrtimer_cancel(), a circular dependency forms:

  CPU 0: holds lock_A -> waits for softirq_expiry_lock
  CPU 1: holds softirq_expiry_lock -> waits for lock_A

Fix this by cancelling both timers before acquiring their respective
locks.  hrtimer_cancel() is safe to call without holding any lock and
will wait for any in-progress callback to complete.  The locks are still
acquired afterwards to synchronize with any in-flight packet processing
before tearing down the state.

Found by source code audit.

Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
Cc: Christian Hopps <chopps@labn.net>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
 net/xfrm/xfrm_iptfs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 97bc979e55baf..fd25b2b230793 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -2708,8 +2708,9 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	if (!xtfs)
 		return;
 
-	spin_lock_bh(&xtfs->x->lock);
 	hrtimer_cancel(&xtfs->iptfs_timer);
+
+	spin_lock_bh(&xtfs->x->lock);
 	__skb_queue_head_init(&list);
 	skb_queue_splice_init(&xtfs->queue, &list);
 	spin_unlock_bh(&xtfs->x->lock);
@@ -2717,8 +2718,9 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	while ((skb = __skb_dequeue(&list)))
 		kfree_skb(skb);
 
-	spin_lock_bh(&xtfs->drop_lock);
 	hrtimer_cancel(&xtfs->drop_timer);
+
+	spin_lock_bh(&xtfs->drop_lock);
 	spin_unlock_bh(&xtfs->drop_lock);
 
 	if (xtfs->ra_newskb)
-- 
2.47.3

Re: [PATCH] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

[Steffen Klassert]

On Thu, May 28, 2026 at 04:03:18PM +0000, Tristan Madani wrote:
> iptfs_destroy_state() calls hrtimer_cancel() while holding a spinlock
> that the timer callback also acquires, leading to an ABBA deadlock on
> SMP systems.
> 
> For the output timer (iptfs_timer):
>   - iptfs_destroy_state() holds x->lock, calls hrtimer_cancel()
>   - iptfs_delay_timer() callback takes x->lock
> 
> For the drop timer (drop_timer):
>   - iptfs_destroy_state() holds drop_lock, calls hrtimer_cancel()
>   - iptfs_drop_timer() callback takes drop_lock
> 
> Both timers use HRTIMER_MODE_REL_SOFT, so their callbacks run in softirq
> context.  When hrtimer_cancel() is called for a soft timer that is
> currently executing on another CPU, hrtimer_cancel_wait_running() spins
> on softirq_expiry_lock -- the same lock held by the softirq running the
> callback.  If the callback is blocked waiting for the spinlock held by
> the caller of hrtimer_cancel(), a circular dependency forms:
> 
>   CPU 0: holds lock_A -> waits for softirq_expiry_lock
>   CPU 1: holds softirq_expiry_lock -> waits for lock_A
> 
> Fix this by cancelling both timers before acquiring their respective
> locks.  hrtimer_cancel() is safe to call without holding any lock and
> will wait for any in-progress callback to complete.  The locks are still
> acquired afterwards to synchronize with any in-flight packet processing
> before tearing down the state.
> 
> Found by source code audit.
> 
> Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
> Cc: Christian Hopps <chopps@labn.net>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
> ---
>  net/xfrm/xfrm_iptfs.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 97bc979e55baf..fd25b2b230793 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -2708,8 +2708,9 @@ static void iptfs_destroy_state(struct xfrm_state *x)
>  	if (!xtfs)
>  		return;
>  
> -	spin_lock_bh(&xtfs->x->lock);
>  	hrtimer_cancel(&xtfs->iptfs_timer);
> +
> +	spin_lock_bh(&xtfs->x->lock);
>  	__skb_queue_head_init(&list);
>  	skb_queue_splice_init(&xtfs->queue, &list);
>  	spin_unlock_bh(&xtfs->x->lock);
> @@ -2717,8 +2718,9 @@ static void iptfs_destroy_state(struct xfrm_state *x)
>  	while ((skb = __skb_dequeue(&list)))
>  		kfree_skb(skb);
>  
> -	spin_lock_bh(&xtfs->drop_lock);
>  	hrtimer_cancel(&xtfs->drop_timer);
> +
> +	spin_lock_bh(&xtfs->drop_lock);
>  	spin_unlock_bh(&xtfs->drop_lock);

What is this? You take the drop_lock just to release it in the next
line.

[PATCH v2] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

[Tristan Madani]

Hi Steffen,

You are right - the lock/unlock pair around the drop_timer cancel was
only needed to serialize with the timer callback, which hrtimer_cancel()
already handles. Since the xfrm state refcount has reached zero by the
time the destructor runs, no concurrent iptfs_input() can be accessing
drop_lock-protected state either. The empty lock/unlock is dead code.

v2 below removes it entirely.

---

From: Tristan Madani <tristan@talencesecurity.com>
Subject: [PATCH v2] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

iptfs_destroy_state() calls hrtimer_cancel() while holding a spinlock
that the timer callback also acquires, leading to an ABBA deadlock on
SMP systems.

For the output timer (iptfs_timer):
  - iptfs_destroy_state() holds x->lock, calls hrtimer_cancel()
  - iptfs_delay_timer() callback takes x->lock

For the drop timer (drop_timer):
  - iptfs_destroy_state() holds drop_lock, calls hrtimer_cancel()
  - iptfs_drop_timer() callback takes drop_lock

Both timers use HRTIMER_MODE_REL_SOFT, so their callbacks run in softirq
context.  When hrtimer_cancel() is called for a soft timer that is
currently executing on another CPU, hrtimer_cancel_wait_running() spins
on softirq_expiry_lock -- the same lock held by the softirq running the
callback.  If the callback is blocked waiting for the spinlock held by
the caller of hrtimer_cancel(), a circular dependency forms:

  CPU 0: holds lock_A -> waits for softirq_expiry_lock
  CPU 1: holds softirq_expiry_lock -> waits for lock_A

Fix by calling hrtimer_cancel() before acquiring the respective locks.
hrtimer_cancel() is safe to call without holding any lock and will wait
for any in-progress callback to complete.  For the output timer, the
lock is still acquired afterwards to drain the packet queue.  For the
drop timer, the lock/unlock pair is removed entirely since it only
existed to serialize with the timer callback, which hrtimer_cancel()
already guarantees.

Found by source code audit.

Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
Cc: Christian Hopps <chopps@labn.net>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: stable@vger.kernel.org
Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
---
v2: remove the now-useless empty drop_lock/unlock pair (Steffen)
---
 net/xfrm/xfrm_iptfs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 97bc979e55baf..82c52bbf25e1a 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -2708,8 +2708,9 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	if (!xtfs)
 		return;
 
-	spin_lock_bh(&xtfs->x->lock);
 	hrtimer_cancel(&xtfs->iptfs_timer);
+
+	spin_lock_bh(&xtfs->x->lock);
 	__skb_queue_head_init(&list);
 	skb_queue_splice_init(&xtfs->queue, &list);
 	spin_unlock_bh(&xtfs->x->lock);
@@ -2717,9 +2718,7 @@ static void iptfs_destroy_state(struct xfrm_state *x)
 	while ((skb = __skb_dequeue(&list)))
 		kfree_skb(skb);
 
-	spin_lock_bh(&xtfs->drop_lock);
 	hrtimer_cancel(&xtfs->drop_timer);
-	spin_unlock_bh(&xtfs->drop_lock);
 
 	if (xtfs->ra_newskb)
 		kfree_skb(xtfs->ra_newskb);
-- 
2.47.3

Best,
Tristan

Re: [PATCH v2] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()

[Steffen Klassert]

On Tue, Jun 02, 2026 at 05:16:41PM -0000, Tristan Madani wrote:
> Hi Steffen,
> 
> You are right - the lock/unlock pair around the drop_timer cancel was
> only needed to serialize with the timer callback, which hrtimer_cancel()
> already handles. Since the xfrm state refcount has reached zero by the
> time the destructor runs, no concurrent iptfs_input() can be accessing
> drop_lock-protected state either. The empty lock/unlock is dead code.
> 
> v2 below removes it entirely.
> 
> ---
> 
> From: Tristan Madani <tristan@talencesecurity.com>
> Subject: [PATCH v2] xfrm: iptfs: fix ABBA deadlock in iptfs_destroy_state()
> 
> iptfs_destroy_state() calls hrtimer_cancel() while holding a spinlock
> that the timer callback also acquires, leading to an ABBA deadlock on
> SMP systems.
> 
> For the output timer (iptfs_timer):
>   - iptfs_destroy_state() holds x->lock, calls hrtimer_cancel()
>   - iptfs_delay_timer() callback takes x->lock
> 
> For the drop timer (drop_timer):
>   - iptfs_destroy_state() holds drop_lock, calls hrtimer_cancel()
>   - iptfs_drop_timer() callback takes drop_lock
> 
> Both timers use HRTIMER_MODE_REL_SOFT, so their callbacks run in softirq
> context.  When hrtimer_cancel() is called for a soft timer that is
> currently executing on another CPU, hrtimer_cancel_wait_running() spins
> on softirq_expiry_lock -- the same lock held by the softirq running the
> callback.  If the callback is blocked waiting for the spinlock held by
> the caller of hrtimer_cancel(), a circular dependency forms:
> 
>   CPU 0: holds lock_A -> waits for softirq_expiry_lock
>   CPU 1: holds softirq_expiry_lock -> waits for lock_A
> 
> Fix by calling hrtimer_cancel() before acquiring the respective locks.
> hrtimer_cancel() is safe to call without holding any lock and will wait
> for any in-progress callback to complete.  For the output timer, the
> lock is still acquired afterwards to drain the packet queue.  For the
> drop timer, the lock/unlock pair is removed entirely since it only
> existed to serialize with the timer callback, which hrtimer_cancel()
> already guarantees.
> 
> Found by source code audit.
> 
> Fixes: 4b3faf610cc6 ("xfrm: iptfs: add new iptfs xfrm mode impl")
> Cc: Christian Hopps <chopps@labn.net>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>

Applied, thanks a lot!