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 10:26:27AM +0100, Sabrina Dubroca 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.
> 
> 
> @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.

Thank you for the review. 
Should I submit a v2 patch that removes the 
xfrm_nat_keepalive_state_updated() call from __xfrm_state_delete()?


Best regards,
Hyunwoo Kim