Re: nf_queue: hook changes drop or re-queue packets waiting in any nfqueue

[Florian Westphal <fw@strlen.de>]

Antonio Ojea <antonio.ojea.garcia@gmail.com> wrote:
> deleting an nftables base chain drops every packet that is waiting for
> a verdict in any nfqueue of the network namespace, including packets
> queued by hooks of other tables and other families. Adding a base chain
> in front of the one that queued a packet makes the packet traverse the
> queuing hook again after the verdict, so userspace sees it twice.
> 
> We found the first problem in kube-network-policies and kindnet
> (Kubernetes network policy and CNI agents built on nfqueue) [1]. Their
> nftables sync did "add table; delete table; add table" to replace the
> ruleset, and every sync lost the connections being evaluated at that
> moment. The userspace symptom is the verdict for the dropped packet
> failing asynchronously with -ENOENT:
> 
>   "Could not receive message"
>   error="netlink receive: no such file or directory"
> 
> The add/delete/add idiom was my mistake. The documented way to replace
> a ruleset atomically is "flush table" (or "flush chain") in the same
> transaction, which keeps the base chains and their hooks, and we need
> to change that. However, it does not cover upgrades. A table written by an
> older version may have a chain and/or sets the new version
> no longer uses. Removing them still requires deleting the table, or
> the chain, at least once at startup, with the same effect on every
> nfqueue in the namespace. Suggestions on how to handle that case are
> welcome.
> 
> Independently of fixing it in our projects, any other component will
> trigger the same problem that applies to any nfqueue consumer.

I don't think this is fixable.  You could tell your coding assistant
to pass const struct nf_hook_ops *reg down into nf_queue_nf_hook_drop().

If thats not NULL, then pass it to a new nfqnl_cmpfn() that only
drops skbs when the struct nf_queue_entry shares same pf and
same hooknum as the one in nf_hook_ops arg.

That would limit the impact a bit.