Re: [PATCH nf,v2] netfilter: conntrack: add dead flag to helpers

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, May 15, 2026 at 01:30:33AM +0200, Pablo Neira Ayuso wrote:
> On Thu, May 14, 2026 at 05:44:58PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Thu, May 14, 2026 at 04:43:17PM +0200, Florian Westphal wrote:
> > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > > Add a new NF_CT_HELPER_F_DEAD helper flag to notify the packet path that
> > > > > this helper is going away. Thus, helpers are effectively disabled and no
> > > > > new expectations are created while removing the expectations created by
> > > > > this helper as well as unhelping the existing conntrack entries.
> > > > > 
> > > > > Add the check for NF_CT_HELPER_F_DEAD in the packet path to:
> > > > > - Conntrack confirmation path which invokes the helper callback.
> > > > > - Propagation of helper to conntrack via expectation.
> > > > > - OVS ct helper invocation.
> > > > 
> > > > Not sure this is enough.  New conntracks are not in any hash table /
> > > > unreachable, and synchronize_rcu() doesn't guarantee they get confirmed
> > > > (can get queued).
> > > 
> > > nf_ct_iterate_destroy() calls nf_queue_nf_hook_drop() for each netns.
> > 
> > But is that enough?  Consider:
> > 
> > cpu0						cpu1
> > 						recieves verdict
> > 						unlink from nfqueue list
> > drop_queued_packets (misses unlinked)
> > 						... going on ..
> 
> This looks like a general problem with nf_queue_nf_hook_drop().
> 
> > I think to properly resolve this, there is a need to check
> > for this new dead flag after queueing to userspace (after its on list)
> > and again when receiving the verdict.
> >
> > Arguably this is kind of different bug, because this comment is wrong:
> > 
> > /* a skb w. unconfirmed conntrack could have been reinjected just
> >  * before we called nf_queue_nf_hook_drop().
> >  *
> >  * This makes sure its inserted into conntrack table.
> >  */
> >  synchronize_net();
> > 
> > (it could have been requeued).
> >
> > I think a more generic fix is to add a seqcnt to nf_queue_entry.
> > When queueing, record current seqcnt.
> >
> > On reinject, drop if unconfirmed and seqcnt_now != entry->seqcnt.
> > Not nice, but I don't see a better way ATM.
> 
> But you would need to check right before enqueueing (adding to the
> hashtable/list), so the race would still be there? 
> 
> > The seqcnt can be pernet and it can be restricted to nfnetlink_queue.
> > 
> > Any better idea?
> 
> Maybe add a helper_id which is set at helper registration time. Then
> nf_conn_help stores this helper_id field.  Unconfirmed conntrack on
> reinject use this helper_id to re-lookup the helper when reinjecting.
> This would force a slow path for unconfirmed conntracks, to
> re-validate if the helper is still there.
> 
> cttimeout would need this too, a lookup to check if the timeout policy
> is still around.

Hm.

struct nf_ct_ext {
        u8 offset[NF_CT_EXT_NUM];
        u8 len;
        unsigned int gen_id;   <---- There is already a gen_id here.

And nf_ct_ext_bump_genid() is called from nf_ct_iterate_destroy().

Maybe we could simply check if there is a mismatch in this generation
id from reinject path?