[PATCH ipsec] xfrm: do not take x->lock under xfrm_state_lock in the NAT keepalive walk

[PATCH ipsec] xfrm: do not take x->lock under xfrm_state_lock in the NAT keepalive walk

[Yuejie Shi]

nat_keepalive_work() walks the SA list with xfrm_state_walk(), which
holds net->xfrm.xfrm_state_lock across the callback, and
nat_keepalive_work_single() then takes x->lock underneath it.

The rest of xfrm uses the opposite order.  xfrm_timer_handler() takes
x->lock and calls __xfrm_state_delete(), which takes xfrm_state_lock,
and xfrm_state_flush() deliberately drops xfrm_state_lock before calling
xfrm_state_delete() for that very reason.  The NAT keepalive walk is the
only place that inverts it.

lockdep reports it as soon as both paths have run, which needs nothing
more than a few outbound ESP-in-UDP SAs carrying
XFRMA_NAT_KEEPALIVE_INTERVAL and a hard lifetime -- no traffic and no
peer are required:

  WARNING: possible circular locking dependency detected
  swapper/3/0 is trying to acquire lock:
   (&net->xfrm.xfrm_state_lock){+.-.}, at: __xfrm_state_delete+0x60/0x324
  but task is already holding lock:
   (&x->lock){+.-.}, at: xfrm_timer_handler+0x94/0x50c

  -> #1 (&x->lock){+.-.}:
         _raw_spin_lock+0x48/0x60
         nat_keepalive_work_single+0x120/0xb60
         xfrm_state_walk+0x284/0x468
         nat_keepalive_work+0xc4/0x15c
         process_one_work+0x450/0xc78

  -> #0 (&net->xfrm.xfrm_state_lock){+.-.}:
         _raw_spin_lock+0x48/0x60
         __xfrm_state_delete+0x60/0x324
         xfrm_timer_handler+0x35c/0x50c
         __hrtimer_run_queues+0x1c4/0x5b8

   Possible unsafe locking scenario:
         CPU0                    CPU1
         ----                    ----
    lock(&x->lock);
                                 lock(&net->xfrm.xfrm_state_lock);
                                 lock(&x->lock);
    lock(&net->xfrm.xfrm_state_lock);

xfrm_user_rcv_msg() gates every operation on netlink_net_capable(skb,
CAP_NET_ADMIN), which is CAP_NET_ADMIN over the netns' user namespace,
so an unprivileged user who unshares a user and network namespace can
set this up.  When the two orders do collide, both CPUs spin forever --
one of them inside hrtimer softirq context, which also stops softirq
processing on that CPU.

x->lock is needed here because the data path updates x->lastused under
it.  x->nat_keepalive_expiration is only ever touched by this worker,
which is a per-netns delayed_work and so never runs concurrently with
itself.

Restructuring the walk so the state list lock can be dropped is not
worth it for a once-per-second housekeeping pass.  Use spin_trylock()
instead: it cannot block, so the inversion is gone, and a state whose
lock happens to be held by the data path at that instant is simply
revisited on the next run.  Deferring one keepalive by a second is
harmless -- the configured intervals are in seconds, and next_run is
recomputed from scratch on every pass.

Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
Cc: stable@vger.kernel.org
Signed-off-by: Yuejie Shi <syjcnss@gmail.com>
---
 net/xfrm/xfrm_nat_keepalive.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
index eb1b6f67739e..7143c7a11c18 100644
--- a/net/xfrm/xfrm_nat_keepalive.c
+++ b/net/xfrm/xfrm_nat_keepalive.c
@@ -173,7 +173,19 @@ static int nat_keepalive_work_single(struct xfrm_state *x, int count, void *ptr)
 	if (!interval)
 		return 0;
 
-	spin_lock(&x->lock);
+	/* This runs from xfrm_state_walk() with net->xfrm.xfrm_state_lock
+	 * held, while the rest of xfrm takes x->lock first and
+	 * xfrm_state_lock second (see xfrm_timer_handler() ->
+	 * __xfrm_state_delete(), and xfrm_state_flush(), which drops
+	 * xfrm_state_lock before deleting a state).  Blocking on x->lock
+	 * here would invert that order and deadlock.  A keepalive is
+	 * housekeeping, so if the state is busy just look at it again on
+	 * the next run.
+	 */
+	if (!spin_trylock(&x->lock)) {
+		next_run = ctx->now + 1;
+		goto out;
+	}
 
 	delta = (int)(ctx->now - x->lastused);
 	if (delta < interval) {
@@ -192,6 +204,7 @@ static int nat_keepalive_work_single(struct xfrm_state *x, int count, void *ptr)
 	if (send_keepalive)
 		nat_keepalive_send(&ka);
 
+out:
 	if (!ctx->next_run || next_run < ctx->next_run)
 		ctx->next_run = next_run;
 	return 0;
--
2.51.0