Re: [PATCH 5/8] xfrm: avoid lock inversion in nat keepalive work

[Steffen Klassert <steffen.klassert@secunet.com>]

Zihan, please looks at this review of your patch.

Thanks!

On Thu, Jul 30, 2026 at 07:15:46PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> --- Patch [5]: [PATCH 5/8] xfrm: avoid lock inversion in nat keepalive work ---
> commit 550991d4355a9993b144d945cc62ff754d14ede8
> Author: Zihan Xi <xizh2024@lzu.edu.cn>
> 
> xfrm: avoid lock inversion in nat keepalive work
> 
> This splits nat_keepalive_work() into two phases: the xfrm_state_walk()
> callback now only collects candidate states and takes a reference on each,
> and the actual keepalive processing (which takes x->lock) happens after the
> walk has dropped net->xfrm.xfrm_state_lock.
> 
> > diff --git a/net/xfrm/xfrm_nat_keepalive.c b/net/xfrm/xfrm_nat_keepalive.c
> > index eb1b6f67739e1..8679c68c10a16 100644
> > --- a/net/xfrm/xfrm_nat_keepalive.c
> > +++ b/net/xfrm/xfrm_nat_keepalive.c
> > @@ -156,24 +156,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;
> 
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5]
> Does the allocation footprint here scale with the number of SAs in a way
> that is a concern?  nat_keepalive_work_collect() runs as the
> xfrm_state_walk() callback, so it executes under
> spin_lock_bh(&net->xfrm.xfrm_state_lock), which forces GFP_ATOMIC.
> 
> One node is allocated from the __GFP_HIGH reserves, and one
> xfrm_state_hold() is taken, for every ESP SA with nat_keepalive_interval
> set.  On a gateway with a large number of keepalive SAs that is a single
> burst of tens or hundreds of thousands of GFP_ATOMIC allocations per
> round, and a single failure aborts the entire round (see below).
> 
> For reference, the lock hold time itself is not a regression here: the
> baseline callback already did alloc_skb(GFP_ATOMIC) and transmitted under
> the same lock, so that part is if anything improved.  The concerns are the
> new per-SA reserve draw, the new abort-on-ENOMEM failure mode, and the
> batch-wide reference pinning.
> 
> Was a non-allocating variant considered, for example READ_ONCE/WRITE_ONCE
> or a dedicated lock for the three shared fields x->lastused,
> x->nat_keepalive_interval and x->nat_keepalive_expiration, a persistent
> struct xfrm_state_walk using the walk's stop/resume support, a per-net list
> of keepalive-enabled SAs, or a bounded pre-allocated chunk?
> 
> [Severity: Low]
> [Finding: main-1-2]
> [Sources: opus-5]
> This isn't a bug, but the patch carries:
> 
>     Fixes: f531d13bdfe3 ("xfrm: support sending NAT keepalives in ESP in UDP states")
>     Cc: stable@vger.kernel.org
> 
> while using kmalloc_obj(), which is a recent addition in include/linux/slab.h:
> 
>     #define kmalloc_obj(VAR_OR_TYPE, ...) \
>         __alloc_objs(kmalloc, default_gfp(__VA_ARGS__), typeof(VAR_OR_TYPE), 1)
> 
> Stable branches containing f531d13bdfe3 do not have that macro, so the
> backport will not build without manual conversion.  Would plain
> kmalloc(sizeof(*state), GFP_ATOMIC) be preferable, since the macro buys
> nothing here?
> 
> > +
> > +	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) {
> > @@ -187,29 +214,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);
> 
> [Severity: High]
> [Finding: main-3-5]
> [Sources: opus-5]
> Is nat_keepalive_send() still called from a softirq/BH-disabled context
> after this change?  spin_unlock_bh() re-enables softirqs, and the phase-two
> drain loop in nat_keepalive_work() has no local_bh_disable(), so this call
> now runs from fully preemptible process context.
> 
> nat_keepalive_send() ends up in nat_keepalive_send_ipv4() and
> nat_keepalive_send_ipv6(), both of which do:
> 
>     local_lock_nested_bh(&nat_keepalive_sk_ipv4.bh_lock);
>     sk = this_cpu_read(nat_keepalive_sk_ipv4.sock);
>     sock_net_set(sk, net);
>     ...
>     sock_net_set(sk, &init_net);
>     local_unlock_nested_bh(&nat_keepalive_sk_ipv4.bh_lock);
> 
> and local_lock_nested_bh() starts with an assertion:
> 
>     #define __local_lock_nested_bh(lock)                    \
>         do {                                                \
>             lockdep_assert_in_softirq();                    \
>             local_lock_acquire((lock));                     \
> 
> Before this patch nat_keepalive_work_single() was itself the
> xfrm_state_walk() callback and therefore always ran with
> spin_lock_bh(&net->xfrm.xfrm_state_lock) held, so in_softirq() was true.
> 
> Does every keepalive transmission now hit that WARN_ON_ONCE, replacing the
> lockdep splat the changelog aims to fix with a different one?
> 
> Beyond the warning, is the exclusion for the per-CPU socket still there?
> On !PREEMPT_RT __local_lock_nested_bh() is lockdep bookkeeping only, so
> with BHs enabled nothing serializes the
> this_cpu_read(sock)/sock_net_set(sk, net)/xmit/sock_net_set(sk, &init_net)
> window.  Can a preempted or migrated worker let another netns' keepalive
> work clobber the same per-CPU sk, so a keepalive is emitted with the
> socket's net pointing at init_net or another netns?
> 
> On PREEMPT_RT, local_lock_nested_bh() and local_unlock_nested_bh() each
> resolve __this_cpu_local_lock() and take/release a real spinlock with no
> migrate_disable().  Without BHs disabled, can the task migrate between
> acquire and release and unlock a different CPU's lock, leaving the original
> one held forever?
> 
> Would wrapping the nat_keepalive_send(&ka) call, or the whole phase-two
> loop, in local_bh_disable()/local_bh_enable() be enough?
> 
> >  
> > -	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;
> > +	}
> 
> [Severity: High]
> [Finding: main-1-0]
> [Sources: opus-5]
> Can this retry path make forward progress?
> 
> xfrm_state_walk() aborts the walk on any non-zero callback return and only
> records a resume cursor in walk:
> 
>     err = func(state, walk->seq, data);
>     if (err) {
>         list_move_tail(&walk->all, &x->all);
>         goto out;
>     }
> 
> but xfrm_state_walk_done(&walk, net) is called unconditionally right after,
> and it does list_del(&walk->all), throwing that cursor away.  walk is a
> stack variable that xfrm_state_walk_init() re-initialises on every
> invocation.
> 
> Since WORK_STRUCT_PENDING is cleared before the handler runs, the
> schedule_delayed_work(..., 0) here succeeds and the work re-executes
> essentially immediately, restarting from the head of net->xfrm.state_all
> and failing at the same point while the atomic reserves are still
> depleted.  There is no backoff and no retry bound, and nothing is retired
> between attempts.
> 
> Does this also duplicate keepalive packets?  The send branch in
> nat_keepalive_work_single():
> 
>     } else {
>         next_run = ctx->now + interval;
>         nat_keepalive_init(&ka, x);
>         send_keepalive = true;
>     }
> 
> computes a local next_run but never updates x->nat_keepalive_expiration, so
> on each immediate retry within the same second every SA before the failure
> point still satisfies delta >= interval and
> x->nat_keepalive_expiration <= ctx->now, and sends again.
> 
> And do SAs positioned after the failure point ever get serviced, given the
> walk always restarts from the head and the ctx.next_run schedule computed
> for the states that were processed is discarded by the early return?
> 
> Would returning 0 from nat_keepalive_work_collect() to skip the
> unallocatable entry (keeping the normal ctx.next_run schedule), or resuming
> the saved walk cursor with a bounded backoff, be preferable?
> 
> The changelog describes this as a locking-order fix only and does not
> mention the new retry behaviour.  Could it be documented there?
> 
> >  	if (ctx.next_run)
> >  		schedule_delayed_work(&net->xfrm.nat_keepalive_work,
> >  				      (ctx.next_run - ctx.now) * HZ);