[PATCH net v3 0/3] net: don't strip zerocopy frag markers from a forwarded skb

[Norbert Szetei <norbert@doyensec.com>]

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/AD1B7BEE-C04C-4A1B-982C-8385F1908911@doyensec.com/

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/8063260C-05C9-4997-B9B6-2135063C4858@doyensec.com/

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