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