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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
