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 for the detailed review. I agree that v7 is too complex, and
that I haven't adequately justified the additional
task-local and queue-entry state. In particular, a single queue-entry
flag conflates the distinct IP and netdev
duplication states, and v7 does not establish why they can safely be
merged. The in_task() checks and the save/OR/
restore logic add further complexity. I will not resend v7 as-is.

I also considered your suggestion to extend the local_bh_disable() /
local_bh_enable() pair in nf_reinject() to cover
hook traversal. Based on the code, this alone does not appear to
preserve the duplication guard across the
asynchronous NFQUEUE verdict path: nf_dup_ipv4() and nf_dup_ipv6()
clear current->in_nf_duplicate when the original
output call returns, and the baseline NFQUEUE path does not otherwise
preserve that state. When a later userspace
NF_ACCEPT verdict resumes hook traversal in nf_reinject(), the guard
is not restored. Extending the BH-disabled region
could address a separate synchronous BH-interference case, but it
would not close this state-lifetime gap.

I'll revisit the queue and reinjection paths to look for a smaller,
correct fix that keeps the IP and netdev recursion
states separate. If I cannot find a sufficiently simple solution, I'll
report back with the analysis and ask whether
you prefer the user-namespace restriction you mentioned.

Best regards,
Zihan