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