Re: [PATCH net] xfrm: Fix work re-schedule after cancel in xfrm_nat_keepalive_net_fini()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-03-11, 06:00:03 -0700, Eyal Birger wrote:
> On Wed, Mar 11, 2026 at 2:26 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2026-03-10, 17:14:19 -0700, Eyal Birger wrote:
> > > Hi,
> > >
> > > On Tue, Mar 10, 2026 at 3:57 PM Sabrina Dubroca <sd@queasysnail.net> wrote:
> > > >
> > > > Please also CC the author, and maybe additional contributors, of the
> > > > patch that introduced the problem you're fixing.
> > > >
> > > > 2026-03-11, 03:16:29 +0900, Hyunwoo Kim wrote:
> > > > > After cancel_delayed_work_sync() is called from
> > > > > xfrm_nat_keepalive_net_fini(), xfrm_state_fini() flushes remaining
> > > > > states via __xfrm_state_delete(), which calls
> > > > > xfrm_nat_keepalive_state_updated() to re-schedule nat_keepalive_work.
> > > >
> > > > Eyal, I'm wondering why __xfrm_state_delete() calls
> > > > xfrm_nat_keepalive_state_updated(). At this point the state has been
> > > > removed from the walk list so nat_keepalive_work() won't do
> > > > anything. Am I missing something?
> > >
> > > I don't remember for sure, but I think the idea was to have the work
> > > run "now" so that when deleting the last nat-keepalive state it
> > > won't run in the future, and in general to refresh the interval and
> > > not wait for the next iteration.
> > >
> > > Eyal.
> >
> > Ok. I thought about this, but I'm not seeing the benefit of doing
> > that. Assuming we're deleting just this one state, the next run will
> > process all the remaining states in the same way, whether it happens
> > right now or at the previously scheduled time:
> >
> >  - if the next run was needed by the peer we're deleting, not much
> >    changes except that we're recomputing the delay earlier than
> >    otherwise (right now instead of when deleted_state's interval runs
> >    out)
> >
> >  - if some other state was the first to need a keepalive, we do a run
> >    for nothing
> >
> > So I think we could drop the xfrm_nat_keepalive_state_updated call
> > from __xfrm_state_delete.
> 
> Right. I think at the time I didn't think it was "nice" to have the
> work running long after all states have been deleted, and also it
> decoupled the implementation a little - i.e. if instead of a global
> work we had per-state timers (which one of the original versions had).

Ok, I see. Thanks.

> > @Hyunwoo here again I'm not opposed to s/cancel/disable/, it makes
> > sense to use disable_ in a "destruct" operation where we don't plan to
> > need the work again. But AFAICT this schedule_delayed_work isn't
> > really useful.
> 
> I'm fine with both approaches.

Ok, so:
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Hyunwoo, if you want to send a v2 that does disable_ + remove the
xfrm_nat_keepalive_state_updated call, or keep this one as-is (and
then we can remove the unnecessary xfrm_nat_keepalive_state_updated
call in -next later), that's ok for me either way. Thanks.


BTW, patches for "NETWORKING [IPSEC]" should be tagged as [PATCH ipsec]
or [PATCH ipsec-next] rather than [PATCH net] or [PATCH net-next].
They go through Steffen Klassert's ipsec/ipsec-next trees, and get
pulled into net/net-next a bit later.

-- 
Sabrina