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

On 8/14/26 3:40 PM, Norbert Szetei wrote:
>> On Aug 14, 2026, at 14:31, Ilya Maximets <[email protected]> wrote:
>>
>> On 8/14/26 1:01 PM, Norbert Szetei wrote:
>>> Thanks for the review. Sashiko flagged
>>
>> Hmm.  I do not see any reports in either of the instances.  Do you have a link?
> 
> https://sashiko.dev/#/patchset/[email protected]

Thanks, looks like I was trying to search using the patch name and it
only searches sets.

> 
>>> that the moved call may still be
>>> reachable through the RECIRC action, and I confirmed dynamically that it is.
>>> With 1/2 applied, a flow matching recirc_id 0 exactly, with actions
>>> RECIRC(1), OUTPUT(0) instead of USERSPACE(unbound), OUTPUT(0), reproduces
>>> the same issue. So please hold off on 1/2.
>>>
>>> Moving the call to the "default" branch assumes that branch only sees a
>>> packet the datapath owns. For a non-last OVS_ACTION_ATTR_RECIRC,
>>> clone_execute() does
>>>
>>> skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
>>> ...
>>> ovs_dp_process_packet(skb, clone);
>>>
>>> so a clone lands there while do_execute_actions() carries on with the
>>> original. The clone shares skb_shinfo() exactly for the skbs this series is
>>> about, since skb_clone() -> skb_orphan_frags() returns early on
>>> SKBFL_DONT_ORPHAN and does not copy the frags - so settling the uarg
>>> through the clone clears SKBFL_SHARED_FRAG for the skb still being
>>> forwarded.
>>
>> AFAIU, operations on a cloned skb performed via proper skb helpers must
>> not affect the original.  That's the whole point of the clone.  However,
>> in this case indeed it looks like the skb_tx_copy() just modifies the
>> shared info not checking if it is shared or not.  And this sounds like
>> a bug in skb_tx_copy().
>>
>>>
>>> Removing the call, as I originally suggested, does fix this in my testing.
>>> If you would still rather keep it, how would you prefer to solve this?
>>
>> Just removing the call from openvswitch module doesn't solve the problem.
>> Packet may enter OVS already cloned somewhere else in the stack, and at
>> any other point in the kernel where skb_tx_copy() is called it may be
>> operating on a clone of some other skb causing the exact same issue.  So,
>> it needs to be addressed inside the skb_tx_copy() itself.
>>
>> On the other hand, reading the history of this function, it seems like it
>> lost of its meaning with commit 1f8b977ab32d ("sock: enable MSG_ZEROCOPY")
>> from Willem that changed it to just call skb_zcopy_clear(skb, true);  This
>> changed the "false" signaling to "true".  So it doesn't even signal an error
>> anymore.
>>
>> Later, in commit 753f1ca4e1e5 ("net: introduce managed frags infrastructure")
>> Pavel added skb_zcopy_downgrade_managed(skb); call that takes extra frag
>> references.  Though it seems pointless for an skb that must be freed right
>> after.
>>
>> So, I'm not sure if this function is useful in general.  Feels like it is
>> only harmful as it directly modifies shared data with no regards to clones.
>>
>> We have two options here:
>>
>> 1. Minimal fix: add something like skb_cloned() guard into skb_tx_copy().
>>
>> 2. Remove skb_tx_copy() entirely (all calls and the definition) as it

* I meant skb_tx_error(), of course, everywhere above in place of skb_tx_copy()
  that is not a real function...

>>   seems pointless after 1f8b977ab32d.
> 
> Thanks for digging out 1f8b977ab32d.
> 
> Option 2 sounds cleaner to me, though it touches tun, ovpn and nfnetlink_queue
> as well. Option 1 would not cover the reported case on its own, since there is
> no clone on the OVS_ACTION_ATTR_USERSPACE path, but it should work with this 
> patch 1/2.
> 
> Curious what the others think. I can write whichever you settle on.

If there will be no other suggestions, I'd say what we can do is to have
a minimal fix for net and stable, i.e., a 3-patch set with 2 current patches
plus the new skb_cloned() guard inside skb_tx_error().  These should be
simple enough to backport.

Once those are accepted, we could remove the skb_tx_error() from net-next as
a follow up, so it doesn't muddy the waters moving forward.

> 
> N.
> 
>>
>> Any thoughts?  Willem, Pavel, others?
>>
>>>
>>> Thanks,
>>> Norbert
>>>
>>>> On Aug 13, 2026, at 12:00, Ilya Maximets <[email protected]> wrote:
>>>>
>>>> On 8/13/26 7:47 AM, Norbert Szetei wrote:
>>>>> 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: [email protected]
>>>>> Assisted-by: Claude:claude-opus-5
>>>>> Signed-off-by: Norbert Szetei <[email protected]>
>>>>> ---
>>>> Reviewed-by: Ilya Maximets <[email protected]>
> 
>