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@xxxxxxxx>
> Cc: Steffen Klassert <steffen.klassert@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Tristan Madani <tristan@xxxxxxxxxxxxxxxxxxx>
> ---
>  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.