Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net/vsock: severe receive fast-path performance regression --
commit d73a7bc02ed525e8162b0e1c12fa4585af2535c6 Author: Bartłomiej Dmitruk <[email protected]> vsock/vmci: harden datagram receive access check (netns bypass + cached_peer race) This patch fixes a namespace bypass by enforcing vsock_net_mode_global on the receive path. It also removes the unsynchronized vsock->cached_peer cache to avoid data races between the lockless receive tasklet and the send path. > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c > index 1f186e8f83640..37b6f6eca962b 100644 > --- a/net/vmw_vsock/vmci_transport.c > +++ b/net/vmw_vsock/vmci_transport.c > @@ -529,18 +529,25 @@ static bool vmci_transport_allow_dgram(struct > vsock_sock *vsock, u32 peer_cid) > if (VMADDR_CID_HYPERVISOR == peer_cid) > return true; [Severity: Medium] This is a pre-existing issue, but does this early return for the hypervisor CID skip the newly added netns isolation check below? A socket bound to a non-global netns could still receive datagrams from the hypervisor while being blocked from sending to it, which would leave a namespace isolation boundary partially bypassed. > > - if (vsock->cached_peer != peer_cid) { > - vsock->cached_peer = peer_cid; > - if (!vmci_transport_is_trusted(vsock, peer_cid) && > - (vmci_context_get_priv_flags(peer_cid) & > - VMCI_PRIVILEGE_FLAG_RESTRICTED)) { > - vsock->cached_peer_allow_dgram = false; > - } else { > - vsock->cached_peer_allow_dgram = true; > - } > - } > + /* Enforce the per-netns mode on the receive path, symmetrically with > + * the send hook vmci_transport_dgram_allow(): a socket in a non-global > + * (local) netns must not receive datagrams it could never send. > + */ > + if (!vsock_net_mode_global(vsock)) > + return false; > > - return vsock->cached_peer_allow_dgram; > + /* Evaluate on every datagram instead of caching the decision in > + * vsock->cached_peer{,_allow_dgram}: those fields were an > + * unsynchronized check-then-set shared between the lockless receive > + * tasklet and the lock_sock() send path, which could return a stale > + * 'allow' for a restricted peer. > + */ > + if (!vmci_transport_is_trusted(vsock, peer_cid) && > + (vmci_context_get_priv_flags(peer_cid) & > + VMCI_PRIVILEGE_FLAG_RESTRICTED)) [Severity: High] Does evaluating this unconditionally for every incoming datagram cause a severe receive fast-path performance regression? Both vmci_transport_is_trusted and vmci_context_get_priv_flags call vmci_ctx_get, which executes a linear scan of the global ctx_list under rcu_read_lock: drivers/misc/vmw_vmci/vmci_context.c:vmci_ctx_get() { ... rcu_read_lock(); list_for_each_entry_rcu(c, &ctx_list.head, list_item) { ... } Executing two O(N) list traversals per datagram in the bottom-half (tasklet) receive path could cause high CPU overhead, increase latency, and risk soft lockups under high packet rates, replacing what was previously an O(1) cache check. > + return false; > + > + return true; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
