[PATCH net] xfrm: reschedule NAT keepalive work for new states

[PATCH net] xfrm: reschedule NAT keepalive work for new states

[Baek Kyu, Kim]

The per-net NAT keepalive work can already be pending for a state with a
long interval when a new state with a shorter interval is added.
schedule_delayed_work(..., 0) does not move the pending deadline, so the
new state's keepalive waits for the older state's deadline.

Use mod_delayed_work() for a newly added state to run the worker
immediately and recalculate the next deadline. Keep the existing
schedule_delayed_work() behavior when a state is deleted. States without
a keepalive interval still return early.

Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
An arm64 QEMU/KVM test of the immediate-reschedule change showed that a
new 2-second SA sent a keepalive immediately while the original kernel
sent none for five seconds. With a virtual NAT gateway, the original
mapping expired and a 9-second inbound UDP packet was lost; the changed
kernel retained the mapping and received it. A boot test of the final
patch confirmed an immediate keepalive for the new SA and no immediate
workqueue event or later keepalive for the deleted SA. An arm64 W=1
object build passed. No physical NAT device or ESP payload was tested.

 net/xfrm/xfrm_nat_keepalive.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
index 8ed30e6c263b66..6814f9063969ef 100644
--- a/net/xfrm/xfrm_nat_keepalive.c
+++ b/net/xfrm/xfrm_nat_keepalive.c
@@ -288,7 +288,10 @@ void xfrm_nat_keepalive_state_updated(struct xfrm_state *x)
 		return;
 
 	net = xs_net(x);
-	schedule_delayed_work(&net->xfrm.nat_keepalive_work, 0);
+	if (x->km.state == XFRM_STATE_DEAD)
+		schedule_delayed_work(&net->xfrm.nat_keepalive_work, 0);
+	else
+		mod_delayed_work(system_wq, &net->xfrm.nat_keepalive_work, 0);
 }
 
 int __net_init xfrm_nat_keepalive_net_init(struct net *net)

Re: [PATCH net] xfrm: reschedule NAT keepalive work for new states

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] xfrm: reschedule NAT keepalive work for new states

[Baek Kyu, Kim]

Hi,

The issue was identified while reviewing commit daf8e3b253aa and
inspecting the XFRM NAT keepalive work scheduling path. It was not
reported from production.

I reproduced the delayed keepalive on the original kernel, then verified
the change with arm64 QEMU/KVM and a virtual NAT gateway.

Thanks,
Baek Kyu, Kim

Re: [PATCH net] xfrm: reschedule NAT keepalive work for new states

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In xfrm_nat_keepalive_state_updated()
  (net/xfrm/xfrm_nat_keepalive.c:294), the new insert path queues the…
- [Medium] Before this patch, inserting or deleting a keepalive SA while
  the per-netns nat_keepalive_work timer was pending did nothing…

--

xfrm: reschedule NAT keepalive work for new states

When a non-dead state is added, xfrm_nat_keepalive_state_updated() now
calls mod_delayed_work() instead of schedule_delayed_work(), so the
per-netns NAT keepalive worker runs right away. Before, it waited for an
older, longer pending deadline. Deleted states still use
schedule_delayed_work().

> diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
> index 8ed30e6c263b6..6814f9063969e 100644
> --- a/net/xfrm/xfrm_nat_keepalive.c
> +++ b/net/xfrm/xfrm_nat_keepalive.c
> @@ -288,7 +288,10 @@ void xfrm_nat_keepalive_state_updated(struct xfrm_state *x)
>  		return;
>  
>  	net = xs_net(x);
> -	schedule_delayed_work(&net->xfrm.nat_keepalive_work, 0);
> +	if (x->km.state == XFRM_STATE_DEAD)
> +		schedule_delayed_work(&net->xfrm.nat_keepalive_work, 0);
> +	else
> +		mod_delayed_work(system_wq, &net->xfrm.nat_keepalive_work, 0);

[Severity: High]
Should this be system_percpu_wq instead of system_wq?

The DEAD branch above and the re-arm at the end of nat_keepalive_work()
both use schedule_delayed_work(), which queues on system_percpu_wq:

include/linux/workqueue.h:
static inline bool schedule_delayed_work(struct delayed_work *dwork,
					 unsigned long delay)
{
	return queue_delayed_work(system_percpu_wq, dwork, delay);
}

workqueue_init_early() allocates these as two separate workqueues, and
system_wq is marked deprecated:

	system_wq = alloc_workqueue("events", WQ_PERCPU | __WQ_DEPRECATED, 0);
	system_percpu_wq = alloc_workqueue("events", WQ_PERCPU, 0);

So the first keepalive SA insert will trigger the "enqueued on deprecated
workqueue" pr_warn_once() in __queue_work().

The bigger concern is non-reentrancy. In __queue_work(), the check that
stops the same work item running on two CPUs only applies when it is
requeued on the same workqueue:

		if (worker && worker->current_pwq->wq == wq) {

Say nat_keepalive_work() is already running from system_percpu_wq on one
CPU, so PENDING is clear. An SA insert on another CPU then calls
mod_delayed_work(system_wq, ..., 0). Can a second instance start and run
at the same time as the first?

If it can, the first instance computed its deadline without the new
state. If it finishes after the second one has started, its re-arm wins:

CPU X (instance A)                   CPU Y (instance B)
nat_keepalive_work()
                                     nat_keepalive_work()
  schedule_delayed_work(long)
    sets PENDING
                                       schedule_delayed_work(short)
                                         PENDING already set, no-op

Wouldn't the new SA then wait for the old long deadline, which is the
problem this patch is trying to fix? Before this patch the requeue went
to the same workqueue, so B was queued behind A on A's pool and B's
deadline won.

Two concurrent instances could also both reach the send branch in
nat_keepalive_work_single() for the same state, because that branch does
not update nat_keepalive_expiration:

	} else {
		next_run = ctx->now + interval;
		nat_keepalive_init(&ka, x);
		send_keepalive = true;
	}

That would send duplicate keepalives.

Netns teardown is also affected. xfrm_net_exit()->
xfrm_nat_keepalive_net_fini() does:

	disable_delayed_work_sync(&net->xfrm.nat_keepalive_work);

start_flush_work() only looks for an executing worker on the pool
recorded in work->data, which belongs to the instance that started
last. Could an earlier instance still be walking net->xfrm.state_all
under xfrm_state_lock while xfrm_state_fini() runs and struct net is
freed?

Using mod_delayed_work(system_percpu_wq, ...) here would keep every
enqueue on the same workqueue and would still move the pending deadline.

[Severity: Medium]
Does this remove the rate limit on full state walks?

Before this patch, inserting a keepalive SA while the timer was pending
did nothing. Full walks were limited to one per computed next_run.

Now every non-DEAD call from __xfrm_state_insert() forces an immediate
run, or queues another run if one is already executing. This covers add,
ACQ replacement in update, migrate and rekey.

Each run calls xfrm_state_walk(), which holds net->xfrm.xfrm_state_lock
with BH disabled while it steps over every ESP state that has no
keepalive interval:

nat_keepalive_work_collect() {
	...
	if (!READ_ONCE(x->nat_keepalive_interval))
		return 0;
	...
}

The lock is only dropped after a batch of 16 keepalive states is
collected, so each run costs O(N) in the number of ESP states.

A user with CAP_NET_ADMIN in its own user and net namespace, and many
ESP SAs, could repeatedly send XFRM_MSG_NEWSA and XFRM_MSG_DELSA for a
single keepalive SA:

xfrm_add_sa()
  xfrm_state_add()
    __xfrm_state_insert()
      xfrm_nat_keepalive_state_updated()
        mod_delayed_work(..., 0)
          nat_keepalive_work()
            xfrm_state_walk()

Would each pair of messages then force another full walk in a shared
system kworker, with the CPU time not charged to the caller?

The commit message doesn't cover this. Would it work to move the timer
only when it is set to fire later than the new state's interval allows?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110144321.94096-1-pocj8ur4in%40gmail.com