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

[netdev-bot+sashiko@kernel.org]

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