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>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index aea63a000d1dfcc0cc7e4035d3a4a37d8348e480..ad810d1f97c067d0680a1a4a6b7a038997aa206e 100644 (file)
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -2730,8 +2730,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);
@@ -2739,9 +2740,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);