[PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Norbert Szetei]

queue_userspace_packet() calls skb_tx_error() on the packet skb in its
error path, but it only borrows that skb: on the OVS_ACTION_ATTR_USERSPACE
action path do_execute_actions() ignores output_userspace()'s return value
and keeps forwarding the same skb through the flow's remaining actions.
skb_tx_error() completes the zerocopy uarg and clears SKBFL_ALL_ZEROCOPY,
and with it SKBFL_SHARED_FRAG.

For a MSG_ZEROCOPY skb carrying page-cache frags, SKBFL_SHARED_FRAG is
what makes esp_input() skb_cow_data() instead of taking the in-place AEAD
path. Once it is stripped, a later local ESP delivery decrypts in place
over pages the sender still shares with the page cache.

Patch 1 moves the skb_tx_error() into the one path that does drop the
packet, the "default" arm of ovs_dp_process_packet()'s switch(error).

Patch 2 removes a second such strip, in skb_zerocopy(), which calls
skb_tx_error() on its source when skb_orphan_frags() fails. A copy helper
should not perform a destructive action on its source, and both callers
already report the error on their own drop path. MSG_ZEROCOPY skbs cannot
reach that one -- SKBFL_DONT_ORPHAN makes skb_orphan_frags() return early
-- but producers that do not set that flag, such as vhost-net, can.
Patch 3 is new in v2. It stops skb_tx_error() from touching skb_shinfo()
state that is shared with clones, so patch 1's new call site cannot reach
a live skb either. For a non-last OVS_ACTION_ATTR_RECIRC action
clone_execute() sends a skb_clone() into ovs_dp_process_packet() while
do_execute_actions() keeps forwarding the original, and skb_clone() does
not privatise the frags for these skbs -- skb_orphan_frags() returns early
on SKBFL_DONT_ORPHAN -- so a flow miss on the clone strips
SKBFL_SHARED_FRAG from the packet still in flight. Confirmed on a KASAN
build with a flow matching recirc_id 0 and actions RECIRC(1),OUTPUT(0):
with patches 1 and 2 applied it still reproduces the page-cache write,
with patch 3 on top it no longer does (5/5 runs). A kprobe on
skb_tx_error() shows the datapath drop path is still reached in both
cases, so the difference is the guard and not the reproducer.

As Ilya noted, that makes patch 3 the general fix -- an skb can enter any
skb_tx_error() caller already cloned elsewhere in the stack -- while
patches 1 and 2 keep the callers from acting on an skb they do not own.
Removing skb_tx_error() altogether looks like the right long-term cleanup
and is planned as a net-next follow-up.

v3:
  - patch 3: Fixes tag corrected to 25121173f7b1 ("skb: api to report
    errors for zero copy skbs"), the commit that added skb_tx_error()
    (Ilya Maximets)
  - Tested-by from Jongmin Jang picked up on patches 1 and 3
  - v2: https://lore.kernel.org/netdev/AD1B7BEE-C04C-4A1B-982C-8385F1908911@doyensec.com/

v2:
  - new patch 3: skip the shared skb_shinfo() work in skb_tx_error() when
    the skb is cloned, which also covers the OVS_ACTION_ATTR_RECIRC path
    that patch 1 alone leaves open (suggested by Ilya Maximets)
  - patches 1 and 2 unchanged, Reviewed-by from Ilya Maximets picked up
  - v1: https://lore.kernel.org/netdev/8063260C-05C9-4997-B9B6-2135063C4858@doyensec.com/

Norbert Szetei (3):
  openvswitch: only skb_tx_error() a packet we are about to drop
  net: skbuff: don't skb_tx_error() the source skb in skb_zerocopy()
  net: skbuff: don't touch shared zerocopy state in skb_tx_error()

 net/core/skbuff.c          | 10 ++++++----
 net/openvswitch/datapath.c |  3 +--
 2 files changed, 7 insertions(+), 6 deletions(-)

-- 
2.55.0

[PATCH net v3 1/3] openvswitch: only skb_tx_error() a packet we are about to drop

[Norbert Szetei]

queue_userspace_packet() borrows the packet skb -- it only copies it into
a private netlink message (user_skb) and does not own it; on return
do_execute_actions() keeps forwarding it through the flow's remaining
actions. Its error path nevertheless calls skb_tx_error(skb), which via
skb_zcopy_clear() does skb_shinfo(skb)->flags &= ~SKBFL_ALL_ZEROCOPY,
stripping SKBFL_SHARED_FRAG from that live skb (skb_tx_error()'s kerneldoc
says "skb must be freed afterwards").

For a MSG_ZEROCOPY skb carrying page-cache frags, SKBFL_SHARED_FRAG is
what makes esp_input() skb_cow_data() before in-place AEAD; once it is
stripped a later local ESP-in-UDP delivery decrypts in place over pages
the sender does not own -- an unprivileged page-cache write (the
"Fragnesia" primitive).
do_execute_actions() ignores output_userspace()'s return value, so any
action after a failed USERSPACE upcall inherits the stripped skb.

Move the skb_tx_error() to the flow-miss drop path - the "default"
branch of ovs_dp_process_packet()'s switch(error), before kfree_skb().

The call has been here since commit 36d5fe6a0007 ("core, nfqueue,
openvswitch: Orphan frags in skb_zerocopy and handle errors") but was
harmless until esp_input() began relying on SKBFL_SHARED_FRAG to gate
in-place decrypt; only then did stripping it on a still-forwarded skb
become a page-cache write primitive.

Fixes: 36d5fe6a0007 ("core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors")
Fixes: f4c50a4034e6 ("xfrm: esp: avoid in-place decrypt on shared skb frags")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
Tested-by: Jongmin Jang <payload.jang@gmail.com>
---
 net/openvswitch/datapath.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ae69b2cabab9..fff75c3eed11 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -285,6 +285,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
 			consume_skb(skb);
 			break;
 		default:
+			skb_tx_error(skb);
 			kfree_skb(skb);
 			break;
 		}
@@ -601,8 +602,6 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 	err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb, upcall_info->portid);
 	user_skb = NULL;
 out:
-	if (err)
-		skb_tx_error(skb);
 	consume_skb(user_skb);
 	consume_skb(nskb);
 
-- 
2.55.0

[PATCH net v3 2/3] net: skbuff: don't skb_tx_error() the source skb in skb_zerocopy()

[Norbert Szetei]

skb_zerocopy() copies frags from @from into @to. On an
skb_orphan_frags() failure it calls skb_tx_error(@from), a destructive
operation on the source skb the copy helper does not own. That completes
@from's zerocopy uarg and clears SKBFL_ALL_ZEROCOPY, including the
SKBFL_SHARED_FRAG page-ownership marker.

Both callers already report the failure on their own drop path.
nfnetlink_queue does it at nla_put_failure, and Open vSwitch does it in
the flow-miss drop arm of ovs_dp_process_packet(), so nothing is lost by
dropping it here.

On Open vSwitch's OVS_ACTION_ATTR_USERSPACE path the skb is not freed on
this error: do_execute_actions() ignores output_userspace()'s return
value and, unless the upcall was the last action, keeps forwarding the
same skb through the flow's remaining actions. The uarg is completed
while that skb is still in flight, telling the producer its buffers are
free, and SKBFL_SHARED_FRAG is cleared on an skb the rest of the stack
still handles. That flag is what makes esp_input() call skb_cow_data()
instead of decrypting in place, so a later local ESP delivery can
decrypt over frags the skb does not own privately.

Leave error reporting to the callers.

Fixes: 36d5fe6a0007 ("core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors")
Cc: stable@vger.kernel.org
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
---
 net/core/skbuff.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ba3dbac80fb4..db62ed6e04b9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3907,10 +3907,9 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 
 	skb_len_add(to, len + plen);
 
-	if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
-		skb_tx_error(from);
+	if (unlikely(skb_orphan_frags(from, GFP_ATOMIC)))
 		return -ENOMEM;
-	}
+
 	skb_zerocopy_clone(to, from, GFP_ATOMIC);
 
 	for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
-- 
2.55.0

[PATCH net v3 3/3] net: skbuff: don't touch shared zerocopy state in skb_tx_error()

[Norbert Szetei]

skb_tx_error() completes the zerocopy uarg and clears
SKBFL_ALL_ZEROCOPY, and skb_zcopy_downgrade_managed() clears
SKBFL_MANAGED_FRAG_REFS. Both live in skb_shinfo(), which every clone
shares, while the caller only owns the reference it is about to drop.
Through a clone it tells the producer its pages are free and drops
SKBFL_SHARED_FRAG for an skb that is still in flight.

Open vSwitch reaches this with a non-last OVS_ACTION_ATTR_RECIRC:
clone_execute() sends a skb_clone() into ovs_dp_process_packet() while
do_execute_actions() keeps forwarding the original, and skb_clone()
does not privatise the frags here -- skb_orphan_frags() returns early
on SKBFL_DONT_ORPHAN. A flow miss on the clone then strips the marker
from the packet still being forwarded, and a later local ESP delivery
decrypts in place over frags it does not own privately.

Skip it for a cloned skb. Nothing is lost: skb_release_data() clears
the zerocopy state once the last reference to the shared data goes.

Fixes: 25121173f7b1 ("skb: api to report errors for zero copy skbs")
Cc: stable@vger.kernel.org
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
Tested-by: Jongmin Jang <payload.jang@gmail.com>
---
 net/core/skbuff.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index db62ed6e04b9..04776a112334 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1417,10 +1417,13 @@ EXPORT_SYMBOL(skb_dump);
  *
  *	Report xmit error if a device callback is tracking this skb.
  *	skb must be freed afterwards.
+ *
+ *	Does nothing for a cloned skb: the zerocopy state lives in
+ *	skb_shinfo(), which the clones share.
  */
 void skb_tx_error(struct sk_buff *skb)
 {
-	if (skb) {
+	if (skb && !skb_cloned(skb)) {
 		skb_zcopy_downgrade_managed(skb);
 		skb_zcopy_clear(skb, true);
 	}
-- 
2.55.0

Re: [PATCH net v3 3/3] net: skbuff: don't touch shared zerocopy state in skb_tx_error()

[Ilya Maximets]

On 8/18/26 10:47 AM, Norbert Szetei wrote:
> skb_tx_error() completes the zerocopy uarg and clears
> SKBFL_ALL_ZEROCOPY, and skb_zcopy_downgrade_managed() clears
> SKBFL_MANAGED_FRAG_REFS. Both live in skb_shinfo(), which every clone
> shares, while the caller only owns the reference it is about to drop.
> Through a clone it tells the producer its pages are free and drops
> SKBFL_SHARED_FRAG for an skb that is still in flight.
> 
> Open vSwitch reaches this with a non-last OVS_ACTION_ATTR_RECIRC:
> clone_execute() sends a skb_clone() into ovs_dp_process_packet() while
> do_execute_actions() keeps forwarding the original, and skb_clone()
> does not privatise the frags here -- skb_orphan_frags() returns early
> on SKBFL_DONT_ORPHAN. A flow miss on the clone then strips the marker
> from the packet still being forwarded, and a later local ESP delivery
> decrypts in place over frags it does not own privately.
> 
> Skip it for a cloned skb. Nothing is lost: skb_release_data() clears
> the zerocopy state once the last reference to the shared data goes.
> 
> Fixes: 25121173f7b1 ("skb: api to report errors for zero copy skbs")
> Cc: stable@vger.kernel.org
> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>
> Tested-by: Jongmin Jang <payload.jang@gmail.com>
> ---
>  net/core/skbuff.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index db62ed6e04b9..04776a112334 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1417,10 +1417,13 @@ EXPORT_SYMBOL(skb_dump);
>   *
>   *	Report xmit error if a device callback is tracking this skb.
>   *	skb must be freed afterwards.
> + *
> + *	Does nothing for a cloned skb: the zerocopy state lives in
> + *	skb_shinfo(), which the clones share.
>   */
>  void skb_tx_error(struct sk_buff *skb)
>  {
> -	if (skb) {
> +	if (skb && !skb_cloned(skb)) {
>  		skb_zcopy_downgrade_managed(skb);
>  		skb_zcopy_clear(skb, true);
>  	}

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Ilya Maximets]

On 8/18/26 10:43 AM, Norbert Szetei wrote:
> queue_userspace_packet() calls skb_tx_error() on the packet skb in its
> error path, but it only borrows that skb: on the OVS_ACTION_ATTR_USERSPACE
> action path do_execute_actions() ignores output_userspace()'s return value
> and keeps forwarding the same skb through the flow's remaining actions.
> skb_tx_error() completes the zerocopy uarg and clears SKBFL_ALL_ZEROCOPY,
> and with it SKBFL_SHARED_FRAG.
> 
> For a MSG_ZEROCOPY skb carrying page-cache frags, SKBFL_SHARED_FRAG is
> what makes esp_input() skb_cow_data() instead of taking the in-place AEAD
> path. Once it is stripped, a later local ESP delivery decrypts in place
> over pages the sender still shares with the page cache.
> 
> Patch 1 moves the skb_tx_error() into the one path that does drop the
> packet, the "default" arm of ovs_dp_process_packet()'s switch(error).
> 
> Patch 2 removes a second such strip, in skb_zerocopy(), which calls
> skb_tx_error() on its source when skb_orphan_frags() fails. A copy helper
> should not perform a destructive action on its source, and both callers
> already report the error on their own drop path. MSG_ZEROCOPY skbs cannot
> reach that one -- SKBFL_DONT_ORPHAN makes skb_orphan_frags() return early
> -- but producers that do not set that flag, such as vhost-net, can.
> Patch 3 is new in v2. It stops skb_tx_error() from touching skb_shinfo()
> state that is shared with clones, so patch 1's new call site cannot reach
> a live skb either. For a non-last OVS_ACTION_ATTR_RECIRC action
> clone_execute() sends a skb_clone() into ovs_dp_process_packet() while
> do_execute_actions() keeps forwarding the original, and skb_clone() does
> not privatise the frags for these skbs -- skb_orphan_frags() returns early
> on SKBFL_DONT_ORPHAN -- so a flow miss on the clone strips
> SKBFL_SHARED_FRAG from the packet still in flight. Confirmed on a KASAN
> build with a flow matching recirc_id 0 and actions RECIRC(1),OUTPUT(0):
> with patches 1 and 2 applied it still reproduces the page-cache write,
> with patch 3 on top it no longer does (5/5 runs). A kprobe on
> skb_tx_error() shows the datapath drop path is still reached in both
> cases, so the difference is the guard and not the reproducer.
> 
> As Ilya noted, that makes patch 3 the general fix -- an skb can enter any
> skb_tx_error() caller already cloned elsewhere in the stack -- while
> patches 1 and 2 keep the callers from acting on an skb they do not own.
> Removing skb_tx_error() altogether looks like the right long-term cleanup
> and is planned as a net-next follow-up.
> 
> v3:
>   - patch 3: Fixes tag corrected to 25121173f7b1 ("skb: api to report
>     errors for zero copy skbs"), the commit that added skb_tx_error()
>     (Ilya Maximets)
>   - Tested-by from Jongmin Jang picked up on patches 1 and 3
>   - v2: https://lore.kernel.org/netdev/AD1B7BEE-C04C-4A1B-982C-8385F1908911@doyensec.com/
> 
> v2:
>   - new patch 3: skip the shared skb_shinfo() work in skb_tx_error() when
>     the skb is cloned, which also covers the OVS_ACTION_ATTR_RECIRC path
>     that patch 1 alone leaves open (suggested by Ilya Maximets)
>   - patches 1 and 2 unchanged, Reviewed-by from Ilya Maximets picked up
>   - v1: https://lore.kernel.org/netdev/8063260C-05C9-4997-B9B6-2135063C4858@doyensec.com/
> 
> Norbert Szetei (3):
>   openvswitch: only skb_tx_error() a packet we are about to drop
>   net: skbuff: don't skb_tx_error() the source skb in skb_zerocopy()
>   net: skbuff: don't touch shared zerocopy state in skb_tx_error()
> 
>  net/core/skbuff.c          | 10 ++++++----
>  net/openvswitch/datapath.c |  3 +--
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 

Unfortunately, this needs a rebase now that a conflicting change
for skb_zerocopy() was merged:
  https://lore.kernel.org/all/20260814191336.187243-1-almasrymina@google.com/

Best regards, Ilya Maximets.

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Jakub Kicinski]

On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
> Unfortunately, this needs a rebase now that a conflicting change
> for skb_zerocopy() was merged:

Ugh, I was supposed to merge this first, wasn't I? Sorry.
I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
thing, now I realized that he wasn't CCed :S (please do so on v4)

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Ilya Maximets]

On 8/22/26 2:09 AM, Jakub Kicinski wrote:
> On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
>> Unfortunately, this needs a rebase now that a conflicting change
>> for skb_zerocopy() was merged:
> 
> Ugh, I was supposed to merge this first, wasn't I? Sorry.

Not a huge deal, I guess, the conflict is mechanical and the patches
are simple.  I can take care of manual backports once we get the
'failed to apply' emails.  Just a bit of busy work.

> I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
> thing, now I realized that he wasn't CCed :S (please do so on v4)

FWIW, I CCed a few people on v1 to have a conversation about a proper
fix, but that wasn't fruitful.  So, if I were Norbert, I wouldn't
include them for the new versions either as doing so always feels like
me being annoying. :)

For now, the plan is to get v4 of these targeted fixes into net and
stable and then remove skb_tx_error() entirely once net-next is open,
as it seems to have lost all of its prior meaning.

Best regards, Ilya Maximets.

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Willem de Bruijn]

Ilya Maximets wrote:
> On 8/22/26 2:09 AM, Jakub Kicinski wrote:
> > On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
> >> Unfortunately, this needs a rebase now that a conflicting change
> >> for skb_zerocopy() was merged:
> > 
> > Ugh, I was supposed to merge this first, wasn't I? Sorry.
> 
> Not a huge deal, I guess, the conflict is mechanical and the patches
> are simple.  I can take care of manual backports once we get the
> 'failed to apply' emails.  Just a bit of busy work.
> 
> > I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
> > thing, now I realized that he wasn't CCed :S (please do so on v4)
> 
> FWIW, I CCed a few people on v1 to have a conversation about a proper
> fix, but that wasn't fruitful.  So, if I were Norbert, I wouldn't
> include them for the new versions either as doing so always feels like
> me being annoying. :)

Having a look now.
 
> For now, the plan is to get v4 of these targeted fixes into net and
> stable and then remove skb_tx_error() entirely once net-next is open,
> as it seems to have lost all of its prior meaning.

The original use case in tun_net_xmit introduced in commit
149d36f7187c ("tun: report orphan frags errors to zero copy callback")
still exists. Not sure you can remove the function entirely.

The bug is hit when this function is called with a cloned or shared
skb. The original zerocopy path through tun_net_xmit was probably
expected to not have this problem.

Commit 0110d6f22f39 ("tun: orphan an skb on tx") explains why this
orphan in tun_net_xmit was needed: a (vhost) zerocopy skb injected in
tap1 arriving at tap2 and not being read there indefinitely. Such
loops are not allowed for zerocopy.

Commit 868eefeb17d4 ("tun: orphan frags on xmit") then added the
frags orphan. Since an skb can be cloned on this tap to tap path,
e.g., with a packet socket, I think the bug goes back to the original
commit that introduced the first caller of skb_tx_error, commit
149d36f7187c ("tun: report orphan frags errors to zero copy callback")

If respinning it may be worthile to link to this thread.

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Ilya Maximets]

On 8/22/26 10:55 PM, Willem de Bruijn wrote:
> Ilya Maximets wrote:
>> On 8/22/26 2:09 AM, Jakub Kicinski wrote:
>>> On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
>>>> Unfortunately, this needs a rebase now that a conflicting change
>>>> for skb_zerocopy() was merged:
>>>
>>> Ugh, I was supposed to merge this first, wasn't I? Sorry.
>>
>> Not a huge deal, I guess, the conflict is mechanical and the patches
>> are simple.  I can take care of manual backports once we get the
>> 'failed to apply' emails.  Just a bit of busy work.
>>
>>> I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
>>> thing, now I realized that he wasn't CCed :S (please do so on v4)
>>
>> FWIW, I CCed a few people on v1 to have a conversation about a proper
>> fix, but that wasn't fruitful.  So, if I were Norbert, I wouldn't
>> include them for the new versions either as doing so always feels like
>> me being annoying. :)
> 
> Having a look now.

Thanks!

>  
>> For now, the plan is to get v4 of these targeted fixes into net and
>> stable and then remove skb_tx_error() entirely once net-next is open,
>> as it seems to have lost all of its prior meaning.
> 
> The original use case in tun_net_xmit introduced in commit
> 149d36f7187c ("tun: report orphan frags errors to zero copy callback")
> still exists. Not sure you can remove the function entirely.

The skb_tx_error() prescribes to call kfree_skb() right after it and
all the callers more or less do that (with the fixes applied).

skb_tx_error() does two things:

1. skb_zcopy_downgrade_managed() that takes extra references on frags.
2. Calls skb_zcopy_clear(skb, true);

The kfree_skb() called right after does:

 __kfree_skb
   skb_release_all
     skb_release_data
       if (skb_zcopy)
         bool skip_unref = shinfo->flags & SKBFL_MANAGED_FRAG_REFS;
         skb_zcopy_clear(skb, true);
         if (skip_unref)
           <skip unreferencing the frags, which is the same as taking
            the extra reference>

So, unless I'm missing something, the kfree_skb() already does everything
that skb_tx_error() does.

The fact that skb_tx_error() calls skb_zcopy_clear() with 'true' though
feels weird.  I would understand the need for the function, if it was
actually signalling the error and not success.  But you switched false
to true in commit 1f8b977ab32d ("sock: enable MSG_ZEROCOPY") nine years
ago and it seems like nobody complained so far...

> 
> The bug is hit when this function is called with a cloned or shared
> skb. The original zerocopy path through tun_net_xmit was probably
> expected to not have this problem.
> 
> Commit 0110d6f22f39 ("tun: orphan an skb on tx") explains why this
> orphan in tun_net_xmit was needed: a (vhost) zerocopy skb injected in
> tap1 arriving at tap2 and not being read there indefinitely. Such
> loops are not allowed for zerocopy.
> 
> Commit 868eefeb17d4 ("tun: orphan frags on xmit") then added the
> frags orphan. Since an skb can be cloned on this tap to tap path,
> e.g., with a packet socket, I think the bug goes back to the original
> commit that introduced the first caller of skb_tx_error, commit
> 149d36f7187c ("tun: report orphan frags errors to zero copy callback")
> 
> If respinning it may be worthile to link to this thread.

Re: [PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Willem de Bruijn]

Ilya Maximets wrote:
> On 8/22/26 10:55 PM, Willem de Bruijn wrote:
> > Ilya Maximets wrote:
> >> On 8/22/26 2:09 AM, Jakub Kicinski wrote:
> >>> On Fri, 21 Aug 2026 23:45:41 +0200 Ilya Maximets wrote:
> >>>> Unfortunately, this needs a rebase now that a conflicting change
> >>>> for skb_zerocopy() was merged:
> >>>
> >>> Ugh, I was supposed to merge this first, wasn't I? Sorry.
> >>
> >> Not a huge deal, I guess, the conflict is mechanical and the patches
> >> are simple.  I can take care of manual backports once we get the
> >> 'failed to apply' emails.  Just a bit of busy work.
> >>
> >>> I was hoping for Willem to TAL since skb_tx_error() is a tx ZC
> >>> thing, now I realized that he wasn't CCed :S (please do so on v4)
> >>
> >> FWIW, I CCed a few people on v1 to have a conversation about a proper
> >> fix, but that wasn't fruitful.  So, if I were Norbert, I wouldn't
> >> include them for the new versions either as doing so always feels like
> >> me being annoying. :)
> > 
> > Having a look now.
> 
> Thanks!
> 
> >  
> >> For now, the plan is to get v4 of these targeted fixes into net and
> >> stable and then remove skb_tx_error() entirely once net-next is open,
> >> as it seems to have lost all of its prior meaning.
> > 
> > The original use case in tun_net_xmit introduced in commit
> > 149d36f7187c ("tun: report orphan frags errors to zero copy callback")
> > still exists. Not sure you can remove the function entirely.
> 
> The skb_tx_error() prescribes to call kfree_skb() right after it and
> all the callers more or less do that (with the fixes applied).
> 
> skb_tx_error() does two things:
> 
> 1. skb_zcopy_downgrade_managed() that takes extra references on frags.
> 2. Calls skb_zcopy_clear(skb, true);
> 
> The kfree_skb() called right after does:
> 
>  __kfree_skb
>    skb_release_all
>      skb_release_data
>        if (skb_zcopy)
>          bool skip_unref = shinfo->flags & SKBFL_MANAGED_FRAG_REFS;
>          skb_zcopy_clear(skb, true);
>          if (skip_unref)
>            <skip unreferencing the frags, which is the same as taking
>             the extra reference>
> 
> So, unless I'm missing something, the kfree_skb() already does everything
> that skb_tx_error() does.

Good point. I agree.

> The fact that skb_tx_error() calls skb_zcopy_clear() with 'true' though
> feels weird.  I would understand the need for the function, if it was
> actually signalling the error and not success.  But you switched false
> to true in commit 1f8b977ab32d ("sock: enable MSG_ZEROCOPY") nine years
> ago and it seems like nobody complained so far...

I don't immediately recall the rationale. Probably not intentional
and I should have left the original call-sites, notably tun_net_xmit,
as is.

With MSG_ZEROCOPY, goal is to only set zerocopy_success to false if
a transmission could not be fully completed in zerocopy mode. Falling
back to copying deep in the stack is usually more expensive than doing
it from the start. And if the sendmsg otherwise succeeds, the caller
receives no other signal that MSG_ZEROCOPY is counterproductive.
skb_copy_ubufs will indeed call skb_zcopy_clear(.., false).

General transmit failures are signaled through the normal error path.
IMHO this includes allocation failure with GFP_ATOMIC.

That said, while I don't fully agree with these skb_tx_error()'s with
!zerocopy_success in the skb_orphan_frags() error paths, they did
precede my code. If we want to preserve them we would need to keep
skb_tx_error, with an extra zerocopy_success argument.