Re: [nf PATCH v3] netfilter: nfnetlink: Fix for interrupted hook dumps

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

On Tue, Sep 08, 2026 at 03:57:03PM +0200, Phil Sutter wrote:
> On Tue, Sep 08, 2026 at 02:41:05PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Sep 08, 2026 at 11:23:03AM +0200, Phil Sutter wrote:
> > > Hi Pablo,
> > > 
> > > On Mon, Sep 07, 2026 at 08:19:47PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Sep 04, 2026 at 02:52:15PM +0200, Phil Sutter wrote:
> > > > > @@ -338,27 +337,47 @@ nfnl_hook_entries_head(u8 pf, unsigned int hook, struct net *net, const char *de
> > > > >  }
> > > > >  
> > > > >  static int nfnl_hook_dump_nat(struct sk_buff *nlskb,
> > > > > -			      const struct nfnl_dump_hook_data *ctx,
> > > > > -			      const struct nf_hook_ops *ops,
> > > > > -			      int family, unsigned int seq)
> > > > > +			      struct netlink_callback *cb,
> > > > > +			      const struct nf_hook_ops *ops, int family)
> > > > >  {
> > > > >  	struct nf_nat_lookup_hook_priv *priv = ops->priv;
> > > > > -	struct nf_hook_entries *e = rcu_dereference(priv->entries);
> > > > > +	struct nfnl_dump_hook_data *ctx = cb->data;
> > > > > +	struct net *net = sock_net(nlskb->sk);
> > > > >  	struct nf_hook_ops **nat_ops;
> > > > > -	int i, err;
> > > > > +	unsigned int i = cb->args[1];
> > > > > +	struct nf_hook_entries *e;
> > > > > +	unsigned int base_seq;
> > > > > +	int err = 0;
> > > > >  
> > > > > +	base_seq = smp_load_acquire(&net->nf.nat_hook_base_seq);
> > > > 
> > > > Maybe annnotate this base sequence in the .start via:
> > > > 
> > > > struct netlink_dump_control c = {
> > > >         .start = ...;
> > > > 
> > > > We should probably start doing this in other nfnetlink subsystems too.
> > > > 
> > > > This will help catch an interference between two netlink recv() calls
> > > > which results in calling netlink_dump() which calls this function.
> > > 
> > > I do not comprehend, sorry. The concurrent hook dumps have distinct cb
> > > buffers and net->nf.{nat_,}hook_base_seq is shared but read-only. How
> > > does the problematic interference happen?
> > 
> > See nf_tables_dump_rules_start() for instance. It allocates the struct
> > nft_rule_dump_ctx, which is reachable through .start, .dump and .done
> > callbacks. I think it should be possible to annotate the current
> > base_seq at the beginning of the netlink dump from .start in a ctx
> > object. Then, use it from .dump to check if dump is consistent (ie.
> > turn on the NLM_F_DUMP_INTR flag).
> > 
> > So, instead of fetching the current sequence from .dump like this:
> > 
> >         cb->seq = nft_base_seq(net);
> > 
> > Use the sequence available in the new ctx object, ie. from .dump path
> > you do this:
> > 
> >         cb->seq = ctx->seq;
> 
> But to detect a concurrent hook update between to .dump callback calls
> (if skb space was exceeded), the current value in per-net data has to
> be fetched, no? So this would have to look like this in .dump callback:
> 
> | ctx->seq = nft_base_seq(net);
> | cb->seq = ctx->seq;
> 
> Then I don't get the detour via struct nfnl_dump_hook_data. Or is it
> possible we get multiple concurrent dump requests on the same netlink
> socket and thus cb->seq (and cb->prev_seq) gets shared between them?
> 
> > Because netlink_dump() is called for each userspace recv() call (netlink
> > delivers the chunked listing in several messages), this would allow
> > userspace to know that the listing is inconsistent, then optionally
> > retry.
> > 
> > Makes sense to you?
> 
> Not quite, sorry.

Right, I got confused by the LLM report.

Should be postpone bumping the seq also after array has been shrunk as
LLM suggest in the remove path? For consistency with the insert
operations, just bump sequence _after_ the datastructure update.