[PATCH 0/1] xfrm: avoid lock inversion in nat keepalive work

[PATCH 0/1] xfrm: avoid lock inversion in nat keepalive work

[Ren Wei]

From: Zihan Xi <xizh2024@lzu.edu.cn>

Hi Linux kernel maintainers,

We found and validated a lock inversion issue in
net/xfrm/xfrm_nat_keepalive.c. The bug is reachable when an outbound
ESP-in-UDP state enables NAT keepalives and races with SA deletion.
We've tested the fix, and it does not affect the normal delete path.

This series contains one patch:

  1/1 xfrm: avoid lock inversion in nat keepalive work

We provide bug details, reproducer steps, and a crash log below.

---- details below ----

Bug details:

nat_keepalive_work() walks the state table through xfrm_state_walk()
while xfrm_state_walk() holds net->xfrm.xfrm_state_lock. In the buggy
code, the walk callback nat_keepalive_work_single() then acquires
x->lock. The delete path takes the reverse order:
xfrm_state_delete() acquires x->lock first, and __xfrm_state_delete()
later acquires net->xfrm.xfrm_state_lock. That creates an AB-BA lock
inversion between the keepalive worker and the delete path, and lockdep
reports it as a circular dependency.

The fix keeps xfrm_state_lock out of the per-state processing phase.
The worker first collects matching states under the walk with an extra
reference, then processes them after the walk finishes and only then
takes x->lock. This preserves the original logic while avoiding the
reverse lock nesting.

We first tried to validate this with the original userspace
NETLINK_XFRM reproducer in the bug directory. On this validation kernel,
that reproducer was rejected during strict attribute validation with
"attribute type 34 has an invalid length" before it could reach the
buggy path. To validate the same root cause and the same keepalive
worker versus SA delete ordering in-kernel, we used a temporary local
in-kernel reproducer that creates an outbound ESP-in-UDP state with
NAT keepalive enabled, flushes the keepalive worker, and then deletes
the state. This temporary reproducer was used only for local validation
and is not part of the patch series.

Reproducer:

    # Build kernels with the temporary local in-kernel reproducer linked in
    make -C /var/cache/linux-patch/xfrm-nat-keepalive-src \
        O=/var/cache/linux-patch/bt-parent-uaf-net-main-build -j$(nproc) bzImage

    # Boot the unfixed kernel
    qemu-system-x86_64 -m 2G -cpu max -smp 2 -machine accel=tcg \
        -kernel verify/bzImage-unfixed-kpoc \
        -append 'root=/dev/sda rw console=ttyS0 earlyprintk=serial \
        net.ifnames=0 biosdevname=0 panic_on_warn=1 oops=panic \
        slub_debug=FZPU page_poison=1 init_on_alloc=1 init_on_free=1' \
        -drive file=/tmp/qemu-unfixed-realboot.qcow2,format=qcow2

    # Boot the fixed kernel
    qemu-system-x86_64 -m 2G -cpu max -smp 2 -machine accel=tcg \
        -kernel verify/bzImage-fixed-kpoc \
        -append 'root=/dev/sda rw console=ttyS0 earlyprintk=serial \
        net.ifnames=0 biosdevname=0 panic_on_warn=1 oops=panic \
        slub_debug=FZPU page_poison=1 init_on_alloc=1 init_on_free=1' \
        -drive file=/tmp/qemu-fixed-realboot.qcow2,format=qcow2

We run the validation in a 2 vCPU, 2 GB RAM x86 QEMU environment.

------BEGIN xfrm_nat_keepalive_repro.c------

// SPDX-License-Identifier: GPL-2.0
#include <linux/init.h>
#include <linux/in.h>
#include <linux/ip.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <net/net_namespace.h>
#include <net/xfrm.h>

static int __init xfrm_nat_keepalive_repro_init(void)
{
	struct xfrm_state *x;
	struct xfrm_encap_tmpl *encap;
	int err;

	pr_info("xfrm_nat_keepalive_repro: start\n");

	x = xfrm_state_alloc(&init_net);
	if (!x)
		return 0;

	encap = kzalloc(sizeof(*encap), GFP_KERNEL);
	if (!encap) {
		xfrm_state_put(x);
		return 0;
	}

	encap->encap_type = UDP_ENCAP_ESPINUDP;
	encap->encap_sport = htons(4500);
	encap->encap_dport = htons(4500);

	x->id.proto = IPPROTO_ESP;
	x->id.spi = htonl(0x100);
	x->id.daddr.a4 = htonl(INADDR_LOOPBACK);
	x->props.saddr.a4 = htonl(INADDR_LOOPBACK);
	x->props.family = AF_INET;
	x->props.mode = XFRM_MODE_TRANSPORT;
	x->props.reqid = 1;
	x->sel.family = AF_INET;
	x->sel.daddr.a4 = htonl(INADDR_LOOPBACK);
	x->sel.saddr.a4 = htonl(INADDR_LOOPBACK);
	x->sel.prefixlen_d = 32;
	x->sel.prefixlen_s = 32;
	x->encap = encap;
	x->dir = XFRM_SA_DIR_OUT;
	x->nat_keepalive_interval = 1;
	x->lastused = ktime_get_real_seconds();
	x->km.state = XFRM_STATE_VALID;

	xfrm_state_insert(x);
	flush_delayed_work(&init_net.xfrm.nat_keepalive_work);
	err = xfrm_state_delete(x);
	xfrm_flush_gc();
	pr_info("xfrm_nat_keepalive_repro: delete err=%d\n", err);
	return 0;
}

late_initcall_sync(xfrm_nat_keepalive_repro_init);

------END xfrm_nat_keepalive_repro.c--------

----BEGIN crash log----

[  129.325779][    T1] xfrm_nat_keepalive_repro: start
[  129.348170][    T1] WARNING: possible circular locking dependency detected
[  129.348170][    T1] 7.2.0-rc2+ #5 Not tainted
[  129.348170][    T1] ------------------------------------------------------
[  129.348170][    T1] swapper/0/1 is trying to acquire lock:
[  129.348170][    T1] ffffffff989f9fd8 (&net->xfrm.xfrm_state_lock){+...}-{3:3}, at: __xfrm_state_delete+0xa4/0x9d0
[  129.460853][    T1] but task is already holding lock:
[  129.460853][    T1] ff1100001bbfc0c8 (&x->lock){+...}-{3:3}, at: xfrm_state_delete+0x1b/0x40
[  129.460853][    T1] -> #1 (&x->lock){+...}-{3:3}:
[  129.460853][    T1]        _raw_spin_lock+0x2d/0x40
[  129.460853][    T1]        nat_keepalive_work_single+0x15c/0x1c40
[  129.460853][    T1]        xfrm_state_walk+0x4ed/0xb70
[  129.460853][    T1]        nat_keepalive_work+0xe8/0x1b0
[  129.460853][    T1] -> #0 (&net->xfrm.xfrm_state_lock){+...}-{3:3}:
[  129.460853][    T1]        __xfrm_state_delete+0xa4/0x9d0
[  129.460853][    T1]        xfrm_state_delete+0x23/0x40
[  129.460853][    T1]        xfrm_nat_keepalive_repro_init+0x4ba/0x640
[  129.460853][    T1]  *** DEADLOCK ***
[  130.799594][    T1] xfrm_nat_keepalive_repro: delete err=0

-----END crash log-----

Best regards,
Zihan Xi

Zihan Xi (1):
  xfrm: avoid lock inversion in nat keepalive work

 net/xfrm/xfrm_nat_keepalive.c | 57 +++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/1] xfrm: avoid lock inversion in nat keepalive work

[Ren Wei]

From: Zihan Xi <xizh2024@lzu.edu.cn>

nat_keepalive_work() walks the state table while xfrm_state_walk()
holds net->xfrm.xfrm_state_lock. Its callback then acquires x->lock,
which conflicts with the delete path taking the same locks in reverse
order via xfrm_state_delete() and __xfrm_state_delete(). This creates
an AB-BA deadlock that is reported by lockdep when a NAT keepalive
worker races with SA deletion.

Fix this by splitting the keepalive walk into two phases. First,
collect the candidate states while the walk holds xfrm_state_lock and
take a reference on each state. Then, after the walk completes, process
each collected state and acquire x->lock without nesting it under
xfrm_state_lock.

Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Assisted-by: Codex:gpt-5.4
Signed-off-by: Zihan Xi <xizh2024@lzu.edu.cn>
Signed-off-by: Ren Wei <enjou1224z@gmail.com>
---
 net/xfrm/xfrm_nat_keepalive.c | 57 +++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 9 deletions(-)

diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
index 458931062a04..132de23e64c2 100644
--- a/net/xfrm/xfrm_nat_keepalive.c
+++ b/net/xfrm/xfrm_nat_keepalive.c
@@ -153,24 +153,51 @@ static void nat_keepalive_send(struct nat_keepalive *ka)
 }
 
 struct nat_keepalive_work_ctx {
+	struct list_head states;
 	time64_t next_run;
 	time64_t now;
 };
 
-static int nat_keepalive_work_single(struct xfrm_state *x, int count, void *ptr)
+struct nat_keepalive_state {
+	struct list_head list;
+	struct xfrm_state *x;
+};
+
+static int nat_keepalive_work_collect(struct xfrm_state *x, int count, void *ptr)
 {
 	struct nat_keepalive_work_ctx *ctx = ptr;
+	struct nat_keepalive_state *state;
+
+	if (!READ_ONCE(x->nat_keepalive_interval))
+		return 0;
+
+	state = kmalloc_obj(*state, GFP_ATOMIC);
+	if (!state)
+		return -ENOMEM;
+
+	xfrm_state_hold(x);
+	state->x = x;
+	list_add_tail(&state->list, &ctx->states);
+	return 0;
+}
+
+static void nat_keepalive_work_single(struct xfrm_state *x,
+				      struct nat_keepalive_work_ctx *ctx)
+{
 	bool send_keepalive = false;
 	struct nat_keepalive ka;
-	time64_t next_run;
+	time64_t next_run = 0;
 	u32 interval;
 	int delta;
 
+	spin_lock_bh(&x->lock);
+
+	if (x->km.state == XFRM_STATE_DEAD)
+		goto out;
+
 	interval = x->nat_keepalive_interval;
 	if (!interval)
-		return 0;
-
-	spin_lock(&x->lock);
+		goto out;
 
 	delta = (int)(ctx->now - x->lastused);
 	if (delta < interval) {
@@ -184,29 +211,41 @@ static int nat_keepalive_work_single(struct xfrm_state *x, int count, void *ptr)
 		send_keepalive = true;
 	}
 
-	spin_unlock(&x->lock);
+out:
+	spin_unlock_bh(&x->lock);
 
 	if (send_keepalive)
 		nat_keepalive_send(&ka);
 
-	if (!ctx->next_run || next_run < ctx->next_run)
+	if (next_run && (!ctx->next_run || next_run < ctx->next_run))
 		ctx->next_run = next_run;
-	return 0;
 }
 
 static void nat_keepalive_work(struct work_struct *work)
 {
+	struct nat_keepalive_state *state, *tmp;
 	struct nat_keepalive_work_ctx ctx;
 	struct xfrm_state_walk walk;
 	struct net *net;
+	int err;
 
+	INIT_LIST_HEAD(&ctx.states);
 	ctx.next_run = 0;
 	ctx.now = ktime_get_real_seconds();
 
 	net = container_of(work, struct net, xfrm.nat_keepalive_work.work);
 	xfrm_state_walk_init(&walk, IPPROTO_ESP, NULL);
-	xfrm_state_walk(net, &walk, nat_keepalive_work_single, &ctx);
+	err = xfrm_state_walk(net, &walk, nat_keepalive_work_collect, &ctx);
 	xfrm_state_walk_done(&walk, net);
+	list_for_each_entry_safe(state, tmp, &ctx.states, list) {
+		nat_keepalive_work_single(state->x, &ctx);
+		xfrm_state_put(state->x);
+		kfree(state);
+	}
+	if (err == -ENOMEM) {
+		schedule_delayed_work(&net->xfrm.nat_keepalive_work, 0);
+		return;
+	}
 	if (ctx.next_run)
 		schedule_delayed_work(&net->xfrm.nat_keepalive_work,
 				      (ctx.next_run - ctx.now) * HZ);
-- 
2.43.0