Re: [PATCH nf v7 1/1] netfilter: nf_dup: preserve duplication guard across NFQUEUE

[zihan xi]

On Sun, Oct 11, 2026 at 10:58 PM Florian Westphal <fw@xxxxxxxxx> wrote:
>
> Zihan Xi <zihanx@xxxxxxxxxx> wrote:
> > nf_dup_ipv4() and nf_dup_ipv6() set current->in_nf_duplicate while the
> > cloned skb traverses output hooks. NFQUEUE can retain the clone and return
> > NF_STOLEN while that traversal is in progress. Once ip_local_out() or
> > ip6_local_out() returns, the task-local guard is cleared. NF_ACCEPT then
> > resumes hook traversal after the queued hook in the verdict task, where
> > the guard is no longer set. A later dup rule can clone the packet again,
> > causing repeated packet processing and resource consumption.
>
> I find this waaaaay too complex.
>
> >  include/linux/sched.h            |  2 ++
> >  include/net/netfilter/nf_queue.h |  1 +
> >  net/ipv4/netfilter/ip_tables.c   |  5 ++++-
> >  net/ipv4/netfilter/nf_dup_ipv4.c |  5 ++++-
> >  net/ipv6/netfilter/ip6_tables.c  |  5 ++++-
> >  net/ipv6/netfilter/nf_dup_ipv6.c |  6 +++++-
> >  net/netfilter/nf_dup_netdev.c    | 13 ++++++++++++-
> >  net/netfilter/nf_queue.c         |  4 ++++
> >  net/netfilter/nfnetlink_queue.c  |  8 +++++++-
> >  9 files changed, 43 insertions(+), 6 deletions(-)
>
> Holy cow...  This is waaaay to large/complex, sorry.
>
> >       unsigned                        in_nf_duplicate:1;
> > +     unsigned                        in_nf_dup_netdev:1;     /* netdev dup output is active */
> > +     unsigned                        in_nf_duplicate_reinject:1;     /* NFQUEUE resume */
>
> I don't understand this.
>
> >       struct nf_hook_state    state;
> > +     bool                    in_nf_duplicate;        /* skb is on a duplication path */
>
> OK.
>
> > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> > index 809441ced..7c2027ef8 100644
> > --- a/net/ipv4/netfilter/ip_tables.c
> > +++ b/net/ipv4/netfilter/ip_tables.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/vmalloc.h>
> >  #include <linux/netdevice.h>
> >  #include <linux/module.h>
> > +#include <linux/preempt.h>
> >  #include <net/ip.h>
> >  #include <net/compat.h>
> >  #include <linux/uaccess.h>
> > @@ -270,7 +271,9 @@ ipt_do_table(void *priv,
> >        * but it is no problem since absolute verdict is issued by these.
> >        */
> >       if (static_key_false(&xt_tee_enabled))
> > -             jumpstack += private->stacksize * current->in_nf_duplicate;
> > +             jumpstack += private->stacksize *
> > +                          (current->in_nf_duplicate ||
> > +                           (in_task() && current->in_nf_duplicate_reinject));
>
> ?  Aside from me not getting the need for in_nf_duplicate_reinject, why
> is the in_task() needed but for in_nf_duplicate its not needed...?
>
> As those are bits they even reside on same address...
>
> >  {
> >       struct iphdr *iph;
> > +     bool in_nf_duplicate_reinject = in_task() &&
> > +                                     current->in_nf_duplicate_reinject;
>
> Same question.
>
> >  #include <linux/slab.h>
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> > @@ -219,6 +220,9 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
> >       *entry = (struct nf_queue_entry) {
> >               .skb    = skb,
> >               .skb_dev = skb->dev,
> > +             .in_nf_duplicate = current->in_nf_duplicate ||
> > +                               (in_task() && current->in_nf_duplicate_reinject) ||
> > +                               current->in_nf_dup_netdev,
>
> Why not simply
>
>         .in_nf_dup_netdev = current->in_nf_duplicate ?
>
>
> > +     bool old_in_nf_duplicate_reinject;
> >
> >       net = entry->state.net;
> >       pf = entry->state.pf;
> > +     old_in_nf_duplicate_reinject = current->in_nf_duplicate_reinject;
> > +     current->in_nf_duplicate_reinject |= entry->in_nf_duplicate;
>
> Don't understand the need to store the old current state here.
> Nor the "|=".
>
> > @@ -378,7 +382,7 @@ static void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
> >       if (!hooks || i >= hooks->num_hook_entries) {
> >               kfree_skb_reason(skb, SKB_DROP_REASON_NETFILTER_DROP);
> >               nf_queue_entry_free(entry);
> > -             return;
> > +             goto out;
> >       }
> >
> >       hook_entry = &hooks->hooks[i];
> > @@ -417,6 +421,8 @@ static void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
> >       }
> >
> >       nf_queue_entry_free(entry);
> > +out:
> > +     current->in_nf_duplicate_reinject = old_in_nf_duplicate_reinject;
>
> ... nor this.  AFAICS the only change we really need is to move the
> local_bh_disable/enable pair to cover the hook iteration is well in
> nf_reinject() to avoid BH interference clearing current->in_nf_duplicate
> resp. acting on wron rect "current" task state.
>
> I think we either need to find a way to make this patch way simpler or
> go back to the "no duplication/TEE in userns".  As I already wrote
> earlier, I see no use case for dup/TEE is namespaces, except for CI
> maybe.
>
> Also, I think there are many many other ways to make kernel eat lots of
> RAM, so I don't think there is any need to ftarget nf tree either.
>
> Sorry, I know you've been spinning on this for a while, but this is too
> hard for me to comprehend.

Hi Florian,

Thanks again for the review. I tested the simpler approach you suggested:
extending the local_bh_disable() / local_bh_enable() scope in
nf_reinject() to cover hook lookup, hook traversal, and okfn execution.
The experiment changed only nfnetlink_queue.c and built successfully
against nf.git main at ab699d296e06.

I ran the IPv6 TEE/NFQUEUE reproducer in a 2-vCPU, 2-GiB QEMU guest
in a network namespace owned by a non-initial user namespace, with:

QUEUE_COUNT=1
PAYLOAD=1
TEE_CLONES=1
panic_on_oom=0

The bounded run reached 68,881 callback iterations in 6.700 seconds and
exited normally. No OOM, panic, Oops, BUG, or soft lockup was observed.
This was a bounded resource-consumption run, not an OOM or crash test.

The reproducer continued executing the callback loop throughout the
bounded run, so extending the BH-disabled region alone does not address
this case. It protects against BH interference during nf_reinject(), but
it does not preserve current->in_nf_duplicate across the asynchronous
NFQUEUE verdict path: the original ip_local_out() or ip6_local_out()
call has already returned and cleared the task-local guard before
reinjection starts.

I have also prepared an unsubmitted two-patch v8 candidate that moves the
duplication state into the skb, keeping the IP and netdev states separate.
It requires additional skb metadata handling, GRO compatibility checks,
and bounded asynchronous netdev duplication using NF_RECURSION_LIMIT.
Although this avoids the task-local and queue-entry state propagation in
v7, it introduces its own complexity.

The simpler alternative is the user-namespace restriction you suggested.
It would reject installation with -EPERM at these four entry points:

xt_TEE.c: tee_tg_check()
nft_dup_ipv4.c: nft_dup_ipv4_init()
nft_dup_ipv6.c: nft_dup_ipv6_init()
nft_dup_netdev.c: nft_dup_netdev_init()

The checks would reject installation of TEE targets and nft dup
expressions in network
namespaces whose net->user_ns is not init_user_ns. They would leave the
packet-processing fast paths and init_user_ns-owned network namespaces
unchanged, while making the restriction explicit at setup time.

This would be a policy mitigation rather than a general fix for
asynchronous duplication. It would disable these features in network
namespaces owned by non-initial user namespaces, including potential CI
and container use cases, and existing userspace configurations would
need to handle -EPERM. Any future duplication frontend would also need
equivalent coverage.

Given your concerns about complexity, I am inclined to pursue the
user-namespace restriction rather than send the skb-state v8 candidate
as-is. Would you agree that this is the more appropriate direction, or
do you think the skb-state approach is worth reviewing further?

Best regards,
Zihan