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

[Norbert Szetei <norbert@doyensec.com>]

Thanks for the review. Sashiko flagged 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.

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?

Thanks,
Norbert

> On Aug 13, 2026, at 12:00, Ilya Maximets <i.maximets@ovn.org> 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: 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>