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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Wed, Mar 11, 2026 at 02:27:39PM +0100, Sabrina Dubroca wrote:
> 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.

Thank you for the review, Sabrina. I'll keep the current patch as-is for now.

> 
> 
> 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.

Got it, will keep that in mind for future submissions.


Best regards,
Hyunwoo Kim