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