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

[Steffen Klassert <steffen.klassert@secunet.com>]

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!