[PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Fernando Fernandez Mancera]

nfqnl_mangle() replaces the skb with skb_copy_expand() when a verdict
grows a packet beyond its tailroom. skb_copy_expand() does not copy
skb->sk, so the replacement is unowned and freeing the original drops
its socket memory charge.

The expanded packet is no longer associated with the process that sent
it. Therefore xt_owner or xt_cgroup and other socket based matches in
later rules no longer matches and a later NFQUEUE reports no uid/gid. In
addition, the expanded packet's memory is not counted towards the
sender's SO_SNDBUF limit, so the sender can keep producing them while
they sit in a queue.

Fix this by transferring ownership before freeing the original as
skb_cow_data() an skb_expand_head() do.

Fixes: e2b58a67b91d ("[NETFILTER]: {ip,ip6,nfnetlink}_queue: fix SKB_LINEAR_ASSERT when mangling packet data")
Reported-by: VEGA <vega@nebusec.ai>
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/netfilter/nfnetlink_queue.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 90d887546f7d..fe3b9ce7acb0 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1459,6 +1459,8 @@ nfqnl_mangle(void *data, unsigned int data_len, struct nf_queue_entry *e, int di
 					       diff, GFP_ATOMIC);
 			if (!nskb)
 				return -ENOMEM;
+			if (e->skb->sk)
+				skb_set_owner_w(nskb, e->skb->sk);
 			kfree_skb(e->skb);
 			e->skb = nskb;
 		}
-- 
2.56.0

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> nfqnl_mangle() replaces the skb with skb_copy_expand() when a verdict
> grows a packet beyond its tailroom. skb_copy_expand() does not copy
> skb->sk, so the replacement is unowned and freeing the original drops
> its socket memory charge.
> 
> The expanded packet is no longer associated with the process that sent
> it. Therefore xt_owner or xt_cgroup and other socket based matches in
> later rules no longer matches and a later NFQUEUE reports no uid/gid. In
> addition, the expanded packet's memory is not counted towards the
> sender's SO_SNDBUF limit, so the sender can keep producing them while
> they sit in a queue.

A proper fix requires wmem/truesize fixups and will have to deal with
wmem-accounted vs. non-wmem accounted.

I dont think its worth fixing this, there are many other ways to
get OOM when you have CAP_NET_ADMIN.

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Fernando Fernandez Mancera]

On 10/30/26 3:35 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> nfqnl_mangle() replaces the skb with skb_copy_expand() when a verdict
>> grows a packet beyond its tailroom. skb_copy_expand() does not copy
>> skb->sk, so the replacement is unowned and freeing the original drops
>> its socket memory charge.
>>
>> The expanded packet is no longer associated with the process that sent
>> it. Therefore xt_owner or xt_cgroup and other socket based matches in
>> later rules no longer matches and a later NFQUEUE reports no uid/gid. In
>> addition, the expanded packet's memory is not counted towards the
>> sender's SO_SNDBUF limit, so the sender can keep producing them while
>> they sit in a queue.
> 
> A proper fix requires wmem/truesize fixups and will have to deal with
> wmem-accounted vs. non-wmem accounted.
> 
> I dont think its worth fixing this, there are many other ways to
> get OOM when you have CAP_NET_ADMIN.

That is fair but the keeping ownership of the skb seems worth it for 
other uses too? Anyway, I don't think the OOM aspect is too relevant.

This is just fixing a combination of queue + other matches. I can also 
drop the memory aspect of it from the commit and keep the combination thing.

But yeah, we can just ignore it. There are many combinations that are 
problematic anyway (?)

What do you think?

Thanks,
Fernando.

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 10/30/26 3:35 PM, Florian Westphal wrote:
> > Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> >> nfqnl_mangle() replaces the skb with skb_copy_expand() when a verdict
> >> grows a packet beyond its tailroom. skb_copy_expand() does not copy
> >> skb->sk, so the replacement is unowned and freeing the original drops
> >> its socket memory charge.
> >>
> >> The expanded packet is no longer associated with the process that sent
> >> it. Therefore xt_owner or xt_cgroup and other socket based matches in
> >> later rules no longer matches and a later NFQUEUE reports no uid/gid. In
> >> addition, the expanded packet's memory is not counted towards the
> >> sender's SO_SNDBUF limit, so the sender can keep producing them while
> >> they sit in a queue.
> > 
> > A proper fix requires wmem/truesize fixups and will have to deal with
> > wmem-accounted vs. non-wmem accounted.
> > 
> > I dont think its worth fixing this, there are many other ways to
> > get OOM when you have CAP_NET_ADMIN.
> 
> That is fair but the keeping ownership of the skb seems worth it for 
> other uses too? Anyway, I don't think the OOM aspect is too relevant.

I don't think many people to packet payload rewrite for locally
originating packets.

> This is just fixing a combination of queue + other matches. I can also 
> drop the memory aspect of it from the commit and keep the combination thing.

I think that problem isn't too serious in practice, LLM is overselling.

You just need to structure the ruleset so that the nfqueue stuff comes
after the sk identity checks :-)

> But yeah, we can just ignore it. There are many combinations that are 
> problematic anyway (?)
>
> What do you think?

If you think its worth doing, then by all means, go for it!

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Fernando Fernandez Mancera]

On 10/30/26 3:53 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 10/30/26 3:35 PM, Florian Westphal wrote:
>>> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>>>> nfqnl_mangle() replaces the skb with skb_copy_expand() when a verdict
>>>> grows a packet beyond its tailroom. skb_copy_expand() does not copy
>>>> skb->sk, so the replacement is unowned and freeing the original drops
>>>> its socket memory charge.
>>>>
>>>> The expanded packet is no longer associated with the process that sent
>>>> it. Therefore xt_owner or xt_cgroup and other socket based matches in
>>>> later rules no longer matches and a later NFQUEUE reports no uid/gid. In
>>>> addition, the expanded packet's memory is not counted towards the
>>>> sender's SO_SNDBUF limit, so the sender can keep producing them while
>>>> they sit in a queue.
>>>
>>> A proper fix requires wmem/truesize fixups and will have to deal with
>>> wmem-accounted vs. non-wmem accounted.
>>>
>>> I dont think its worth fixing this, there are many other ways to
>>> get OOM when you have CAP_NET_ADMIN.
>>
>> That is fair but the keeping ownership of the skb seems worth it for
>> other uses too? Anyway, I don't think the OOM aspect is too relevant.
> 
> I don't think many people to packet payload rewrite for locally
> originating packets.
> 

Oh that is for sure. This is a corner case.

>> This is just fixing a combination of queue + other matches. I can also
>> drop the memory aspect of it from the commit and keep the combination thing.
> 
> I think that problem isn't too serious in practice, LLM is overselling.
> 
> You just need to structure the ruleset so that the nfqueue stuff comes
> after the sk identity checks :-)
> 
>> But yeah, we can just ignore it. There are many combinations that are
>> problematic anyway (?)
>>
>> What do you think?
> 
> If you think its worth doing, then by all means, go for it!

The ownership fix is trivial and already done so why not? Anyway, let's 
see what does Pablo think and decide based on that. FWIW, I won't go for 
the whole memory issue fix you mentioned before.

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 10/30/26 3:53 PM, Florian Westphal wrote:
> > If you think its worth doing, then by all means, go for it!
> 
> The ownership fix is trivial and already done so why not? Anyway, let's 
> see what does Pablo think and decide based on that. FWIW, I won't go for 
> the whole memory issue fix you mentioned before.

Its not trivial, unfortunately, this needs wmem adjustments depending
on destructor type, else accouting is off and sk cannot be released
anymore (or we release to early if truesize was reduced).

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Fernando Fernandez Mancera]

On 10/30/26 4:07 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 10/30/26 3:53 PM, Florian Westphal wrote:
>>> If you think its worth doing, then by all means, go for it!
>>
>> The ownership fix is trivial and already done so why not? Anyway, let's
>> see what does Pablo think and decide based on that. FWIW, I won't go for
>> the whole memory issue fix you mentioned before.
> 
> Its not trivial, unfortunately, this needs wmem adjustments depending
> on destructor type, else accouting is off and sk cannot be released
> anymore (or we release to early if truesize was reduced).

Ah that is right. I missed it. Thanks Florian, let me take a look if 
this does not take too much effort I might fix it. Otherwise let's drop 
it. Although we might hear about this in the future due to LLMs.

Re: [PATCH nf] netfilter: nfnetlink_queue: keep socket ownership when expanding skb

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> Ah that is right. I missed it. Thanks Florian, let me take a look if 
> this does not take too much effort I might fix it.

Thanks Fernando!

> Otherwise let's drop
> it. Although we might hear about this in the future due to LLMs.

Undoubtedly so...