[PATCH v2 1/2] vsock/vmci: make the cached_peer dgram decision race-safe

[PATCH v2 1/2] vsock/vmci: make the cached_peer dgram decision race-safe

[Bartłomiej Dmitruk]

vmci_transport_allow_dgram() cached its result in vsock->cached_peer and
vsock->cached_peer_allow_dgram with an unsynchronized check-then-set. The
function runs both in the lockless receive tasklet
(vmci_transport_recv_dgram_cb(), no socket lock) and in the lock_sock() send
path; lock_sock() does not exclude bottom halves, so the two contexts race on
those fields and can return a stale 'allow' for a VMCI_PRIVILEGE_FLAG_RESTRICTED
peer. It is also a plain data race. The in-code comment claiming the fields
are never modified outside create/destruct is contradicted by the send path.

Keep the O(1) cache -- it avoids an O(N) vmci_ctx_get() lookup on every
datagram in the bottom-half receive path -- but pack the peer CID and the
decision into a single word accessed with READ_ONCE()/WRITE_ONCE(). A race
then only forces a recompute and can never return a stale allow.

This was found by code inspection; I do not have VMCI hardware to test on
(compile-tested only).

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
Assisted-by: Claude (Anthropic)
---
v2: keep an O(1) cache made race-safe rather than dropping it entirely; an
    earlier revision removed the cache, which the Sashiko AI review flagged as
    an O(N)-per-datagram fast-path regression. Split out per Stefano
    Garzarella; independent of namespace support.
v1: https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/

diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
index 5549298c1..97968ac53 100644
--- a/include/net/af_vsock.h
+++ b/include/net/af_vsock.h
@@ -39,10 +39,13 @@ struct vsock_sock {
 	 * modified outsided of socket create or destruct.
 	 */
 	bool trusted;
-	bool cached_peer_allow_dgram;	/* Dgram communication allowed to
-					 * cached peer?
-					 */
-	u32 cached_peer;  /* Context ID of last dgram destination check. */
+	/* Cached dgram access decision for the last peer, packed as
+	 * (cid << 32) | VALID | ALLOW and accessed via READ_ONCE()/
+	 * WRITE_ONCE() so the lockless receive tasklet and the
+	 * lock_sock() send path cannot race to a stale decision.
+	 * See vmci_transport_allow_dgram().
+	 */
+	u64 cached_peer_access;
 	const struct cred *owner;
 	/* Rest are SOCK_STREAM only. */
 	long connect_timeout;
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -524,23 +524,38 @@
  * only if it is trusted as described in vmci_transport_is_trusted.
  */
 
+/* Packing for vsk->cached_peer_access. */
+#define VMCI_DGRAM_ACCESS_VALID		BIT_ULL(0)
+#define VMCI_DGRAM_ACCESS_ALLOW		BIT_ULL(1)
+#define VMCI_DGRAM_ACCESS_CID_SHIFT	32
+
 static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
 {
+	u64 access;
+
 	if (VMADDR_CID_HYPERVISOR == peer_cid)
 		return true;
 
-	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;
-		}
-	}
+	/* Cache the trusted/restricted decision for the last peer to avoid the
+	 * O(N) vmci_ctx_get() lookup on every datagram.  Read/update it through
+	 * a single word so a race between the lockless receive tasklet and the
+	 * lock_sock() send path only forces a recompute -- it can never return a
+	 * stale allow for a restricted peer.
+	 */
+	access = READ_ONCE(vsock->cached_peer_access);
+	if ((access & VMCI_DGRAM_ACCESS_VALID) &&
+	    (u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
+		return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
 
-	return vsock->cached_peer_allow_dgram;
+	access = VMCI_DGRAM_ACCESS_VALID |
+		 ((u64)peer_cid << VMCI_DGRAM_ACCESS_CID_SHIFT);
+	if (vmci_transport_is_trusted(vsock, peer_cid) ||
+	    !(vmci_context_get_priv_flags(peer_cid) &
+	      VMCI_PRIVILEGE_FLAG_RESTRICTED))
+		access |= VMCI_DGRAM_ACCESS_ALLOW;
+
+	WRITE_ONCE(vsock->cached_peer_access, access);
+	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
 }
 
 static int

[PATCH v2 2/2] vsock/vmci: enforce per-netns mode on the datagram receive path

[Bartłomiej Dmitruk]

The send hook vmci_transport_dgram_allow() refuses datagrams when the socket's
netns is not in global mode (vsock_net_mode_global()), but the receive path
(vmci_transport_recv_dgram_cb() -> vmci_transport_allow_dgram()) did not, so a
socket bound in a non-global (local) netns received datagrams from peers it
could never send to, defeating namespace isolation.

Add the check at the start of the receive decision, before the
VMADDR_CID_HYPERVISOR short-circuit, so the hypervisor CID is covered too
(matching the send side).

This was found by code inspection; I do not have VMCI hardware to test on
(compile-tested only).

Fixes: eafb64f40ca4 ("vsock: add netns to vsock core")
Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
Assisted-by: Claude (Anthropic)
---
v2: perform the netns check first (before the hypervisor early return) so
    hypervisor datagrams are also gated in local mode -- the Sashiko AI review
    noted v1 left that path partially bypassed.
v1: https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -532,6 +532,14 @@
 static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
 {
 	u64 access;
+
+	/* Enforce the per-netns mode first, 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 (this also covers the
+	 * hypervisor CID).
+	 */
+	if (!vsock_net_mode_global(vsock))
+		return false;
 
 	if (VMADDR_CID_HYPERVISOR == peer_cid)
 		return true;

Re: [PATCH v2 2/2] vsock/vmci: enforce per-netns mode on the datagram receive path

[Stefano Garzarella]

On Sat, Sep 19, 2026 at 02:31:57PM +0200, Bartłomiej Dmitruk wrote:
>The send hook vmci_transport_dgram_allow() refuses datagrams when the socket's
>netns is not in global mode (vsock_net_mode_global()), but the receive path
>(vmci_transport_recv_dgram_cb() -> vmci_transport_allow_dgram()) did not, so a
>socket bound in a non-global (local) netns received datagrams from peers it
>could never send to, defeating namespace isolation.
>
>Add the check at the start of the receive decision, before the
>VMADDR_CID_HYPERVISOR short-circuit, so the hypervisor CID is covered too
>(matching the send side).
>
>This was found by code inspection; I do not have VMCI hardware to test on
>(compile-tested only).
>
>Fixes: eafb64f40ca4 ("vsock: add netns to vsock core")
>Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
>Assisted-by: Claude (Anthropic)
>---
>v2: perform the netns check first (before the hypervisor early return) so
>    hypervisor datagrams are also gated in local mode -- the Sashiko AI review
>    noted v1 left that path partially bypassed.
>v1: https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/
>

LGTM, but please add a cover letter to a series with multiple patches.

Stefano

>diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>--- a/net/vmw_vsock/vmci_transport.c
>+++ b/net/vmw_vsock/vmci_transport.c
>@@ -532,6 +532,14 @@
> static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
> {
> 	u64 access;
>+
>+	/* Enforce the per-netns mode first, 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 (this also covers the
>+	 * hypervisor CID).
>+	 */
>+	if (!vsock_net_mode_global(vsock))
>+		return false;
>
> 	if (VMADDR_CID_HYPERVISOR == peer_cid)
> 		return true;
>

Re: [PATCH v2 2/2] vsock/vmci: enforce per-netns mode on the datagram receive path

[Vishnu Dasa]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

On Sat, Sep 19, 2026 at 5:32 AM Bartłomiej Dmitruk
<bartlomiej.dmitruk@isec.pl> wrote:
>
> The send hook vmci_transport_dgram_allow() refuses datagrams when the socket's
> netns is not in global mode (vsock_net_mode_global()), but the receive path
> (vmci_transport_recv_dgram_cb() -> vmci_transport_allow_dgram()) did not, so a
> socket bound in a non-global (local) netns received datagrams from peers it
> could never send to, defeating namespace isolation.
>
> Add the check at the start of the receive decision, before the
> VMADDR_CID_HYPERVISOR short-circuit, so the hypervisor CID is covered too
> (matching the send side).
>
> This was found by code inspection; I do not have VMCI hardware to test on
> (compile-tested only).
>
> Fixes: eafb64f40ca4 ("vsock: add netns to vsock core")
> Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
> Assisted-by: Claude (Anthropic)

Reviewed-by: Vishnu Dasa <vishnu.dasa@broadcom.com>

> ---
> v2: perform the netns check first (before the hypervisor early return) so
>     hypervisor datagrams are also gated in local mode -- the Sashiko AI review
>     noted v1 left that path partially bypassed.
> v1: https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -532,6 +532,14 @@
>  static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
>  {
>         u64 access;
> +
> +       /* Enforce the per-netns mode first, 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 (this also covers the
> +        * hypervisor CID).
> +        */
> +       if (!vsock_net_mode_global(vsock))
> +               return false;
>
>         if (VMADDR_CID_HYPERVISOR == peer_cid)
>                 return true;

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5417 bytes --]

Re: [PATCH v2 2/2] vsock/vmci: enforce per-netns mode on the datagram receive path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Torn 64-bit lockless access to `vsock->cached_peer_access` on
  32-bit builds defeats the access-control invariant the series claims…

Pre-existing issues:
- [Low] Stale justification comments in both lockless callers of
  `vmci_transport_allow_dgram()`.

--

vsock/vmci: enforce per-netns mode on the datagram receive path

This adds a vsock_net_mode_global() check at the top of
vmci_transport_allow_dgram() so the receive path refuses datagrams when the
socket's netns is not in global mode, matching the send hook
vmci_transport_dgram_allow().  The check is placed before the
VMADDR_CID_HYPERVISOR short-circuit so hypervisor datagrams are gated too.

> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 31728e26a770..d6791459e2fd 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -533,6 +533,14 @@ static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
>  {
>  	u64 access;
>  
> +	/* Enforce the per-netns mode first, 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 (this also covers the
> +	 * hypervisor CID).
> +	 */
> +	if (!vsock_net_mode_global(vsock))
> +		return false;
> +
>  	if (VMADDR_CID_HYPERVISOR == peer_cid)
>  		return true;
>  

[Severity: High]
While looking at the rest of vmci_transport_allow_dgram(), can the packed
cached_peer_access word tear on 32-bit builds?

The helper reads and writes the decision as one u64:

net/vmw_vsock/vmci_transport.c:vmci_transport_allow_dgram() {
	access = READ_ONCE(vsock->cached_peer_access);
	if ((access & VMCI_DGRAM_ACCESS_VALID) &&
	    (u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
		return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
	...
	WRITE_ONCE(vsock->cached_peer_access, access);
	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
}

and the comment above it, plus the field comment in include/net/af_vsock.h,
state that using a single word means the race between the receive tasklet
and the send path "can never return a stale allow for a restricted peer".

include/asm-generic/rwonce.h says the opposite for a 64-bit access on a
32-bit architecture:

 * Yes, this permits 64-bit accesses on 32-bit architectures. These will
 * actually be atomic in some cases (namely Armv7 + LPAE), but for others we
 * rely on the access being split into 2x32-bit accesses ... and a strong
 * prevailing wind.

drivers/misc/vmw_vmci/Kconfig allows this on 32-bit x86:

	depends on (X86 || ARM64) && !CPU_BIG_ENDIAN && PCI

The reader runs with no socket lock held, from
vmci_transport_recv_dgram_cb() and vmci_transport_recv_stream_cb(), while
the writer can run concurrently from vmci_transport_dgram_enqueue() under
lock_sock(), which does not exclude bottom halves.

So on i386, with the cache holding {cid=B, VALID, ALLOW clear} for a
RESTRICTED peer B, can this happen?

CPU0 (sendmsg, trusted peer A) in vmci_transport_allow_dgram()
	WRITE_ONCE(vsock->cached_peer_access, access);
	  -> low word stored first: VALID|ALLOW
	  -> high word still holds B

CPU1 (receive tasklet, datagram from restricted peer B)
	access = READ_ONCE(vsock->cached_peer_access);
	  -> low = VALID|ALLOW, high = B
	(u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid  /* matches */
	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);              /* true */

That would accept a datagram from a VMCI_PRIVILEGE_FLAG_RESTRICTED context
on a non-trusted socket, and the mirrored tear (new high word, old low word)
would drop legitimate datagrams.

Would an atomic64_t accessed with atomic64_read()/atomic64_set(), a
seqcount, or plain serialization be needed here instead of u64
READ_ONCE()/WRITE_ONCE()?

[Severity: Low]
This isn't an issue introduced by this patch, but since
vmci_transport_allow_dgram() is being touched: both lockless callers still
carry a justification that no longer matches the helper.

vmci_transport_recv_dgram_cb() and vmci_transport_recv_stream_cb() both say:

	 * NOTE: We access the socket struct without holding the lock here.
	 * This is ok because the field we are interested is never modified
	 * outside of the create and destruct socket functions.
	 */
	vsk = vsock_sk(sk);
	if (!vmci_transport_allow_dgram(vsk, dg->src.context))
		return VMCI_ERROR_NO_ACCESS;

but the helper writes socket state on the ordinary receive and send paths:

	WRITE_ONCE(vsock->cached_peer_access, access);

and the field comment in include/net/af_vsock.h documents that both the
receive tasklet and the lock_sock() send path update it.

Could the NOTE be reworded to name the fields that really are immutable
(trusted, owner) and mention that cached_peer_access has its own
READ_ONCE()/WRITE_ONCE() protocol?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919123208.29032-1-bartlomiej.dmitruk%40isec.pl

Re: [PATCH v2 1/2] vsock/vmci: make the cached_peer dgram decision race-safe

[Stefano Garzarella]

On Sat, Sep 19, 2026 at 02:31:56PM +0200, Bartłomiej Dmitruk wrote:
>vmci_transport_allow_dgram() cached its result in vsock->cached_peer and
>vsock->cached_peer_allow_dgram with an unsynchronized check-then-set. The
>function runs both in the lockless receive tasklet
>(vmci_transport_recv_dgram_cb(), no socket lock) and in the lock_sock() send
>path; lock_sock() does not exclude bottom halves, so the two contexts race on
>those fields and can return a stale 'allow' for a VMCI_PRIVILEGE_FLAG_RESTRICTED
>peer. It is also a plain data race. The in-code comment claiming the fields
>are never modified outside create/destruct is contradicted by the send path.
>
>Keep the O(1) cache -- it avoids an O(N) vmci_ctx_get() lookup on every
>datagram in the bottom-half receive path -- but pack the peer CID and the
>decision into a single word accessed with READ_ONCE()/WRITE_ONCE(). A race
>then only forces a recompute and can never return a stale allow.

I honestly don't understand this part...

>
>This was found by code inspection; I do not have VMCI hardware to test on
>(compile-tested only).
>
>Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>Signed-off-by: Bartłomiej Dmitruk <bartlomiej.dmitruk@isec.pl>
>Assisted-by: Claude (Anthropic)
>---
>v2: keep an O(1) cache made race-safe rather than dropping it entirely; an
>    earlier revision removed the cache, which the Sashiko AI review flagged as
>    an O(N)-per-datagram fast-path regression. Split out per Stefano
>    Garzarella; independent of namespace support.
>v1: https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/
>
>diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>index 5549298c1..97968ac53 100644
>--- a/include/net/af_vsock.h
>+++ b/include/net/af_vsock.h
>@@ -39,10 +39,13 @@ struct vsock_sock {
> 	 * modified outsided of socket create or destruct.
> 	 */
> 	bool trusted;
>-	bool cached_peer_allow_dgram;	/* Dgram communication allowed to
>-					 * cached peer?
>-					 */
>-	u32 cached_peer;  /* Context ID of last dgram destination check. */
>+	/* Cached dgram access decision for the last peer, packed as
>+	 * (cid << 32) | VALID | ALLOW and accessed via READ_ONCE()/
>+	 * WRITE_ONCE() so the lockless receive tasklet and the
>+	 * lock_sock() send path cannot race to a stale decision.
>+	 * See vmci_transport_allow_dgram().
>+	 */
>+	u64 cached_peer_access;
> 	const struct cred *owner;
> 	/* Rest are SOCK_STREAM only. */
> 	long connect_timeout;
>diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>--- a/net/vmw_vsock/vmci_transport.c
>+++ b/net/vmw_vsock/vmci_transport.c
>@@ -524,23 +524,38 @@
>  * only if it is trusted as described in vmci_transport_is_trusted.
>  */
>
>+/* Packing for vsk->cached_peer_access. */
>+#define VMCI_DGRAM_ACCESS_VALID		BIT_ULL(0)
>+#define VMCI_DGRAM_ACCESS_ALLOW		BIT_ULL(1)
>+#define VMCI_DGRAM_ACCESS_CID_SHIFT	32
>+
> static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
> {
>+	u64 access;
>+
> 	if (VMADDR_CID_HYPERVISOR == peer_cid)
> 		return true;
>
>-	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;
>-		}
>-	}
>+	/* Cache the trusted/restricted decision for the last peer to avoid the
>+	 * O(N) vmci_ctx_get() lookup on every datagram.  Read/update it through
>+	 * a single word so a race between the lockless receive tasklet and the
>+	 * lock_sock() send path only forces a recompute -- it can never 
>return a

Is this a real issue?

We have this in the code:

	 * NOTE: We access the socket struct without holding the lock here.
	 * This is ok because the field we are interested is never modified
	 * outside of the create and destruct socket functions.
	 */
	vsk = vsock_sk(sk);
	if (!vmci_transport_allow_dgram(vsk, dg->src.context))
		return VMCI_ERROR_NO_ACCESS;


>+	 * stale allow for a restricted peer.
>+	 */
>+	access = READ_ONCE(vsock->cached_peer_access);

How this will work on 32-bit systems?

Stefano

>+	if ((access & VMCI_DGRAM_ACCESS_VALID) &&
>+	    (u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
>+		return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
>
>-	return vsock->cached_peer_allow_dgram;
>+	access = VMCI_DGRAM_ACCESS_VALID |
>+		 ((u64)peer_cid << VMCI_DGRAM_ACCESS_CID_SHIFT);
>+	if (vmci_transport_is_trusted(vsock, peer_cid) ||
>+	    !(vmci_context_get_priv_flags(peer_cid) &
>+	      VMCI_PRIVILEGE_FLAG_RESTRICTED))
>+		access |= VMCI_DGRAM_ACCESS_ALLOW;
>+
>+	WRITE_ONCE(vsock->cached_peer_access, access);
>+	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
> }
>
> static int
>

Re: [PATCH v2 1/2] vsock/vmci: make the cached_peer dgram decision race-safe

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix's entire synchronization mechanism is a plain `u64`
  (`vsock_sock::cached_peer_access`) accessed with…
- [Low] Both lockless call sites of vmci_transport_allow_dgram() still
  carry the comment "NOTE: We access the socket struct without holding…

Pre-existing issues:
- [Medium] The per-socket decision cache is keyed only on the peer CID
  and is never invalidated: grep confirms vsock_sock::cached_peer_access…
- [Low] The patch keeps a VMCI-transport-only cache inside the generic
  struct vsock_sock and, in the core header…

--

vsock/vmci: make the cached_peer dgram decision race-safe

This replaces the two unsynchronized cache fields in struct vsock_sock
(cached_peer and cached_peer_allow_dgram) with a single u64
cached_peer_access that packs the peer CID together with a validity and an
allow bit.  vmci_transport_allow_dgram() now reads and publishes that word
with READ_ONCE()/WRITE_ONCE() so that the lockless receive tasklet and the
lock_sock() send path are said to be unable to observe a stale allow
decision.

The v1 posting is referenced from the changelog:
https://lore.kernel.org/netdev/20260917220225.56200-1-bartlomiej.dmitruk@isec.pl/

> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> index 5549298c1ec6..9d59b8fe2853 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -39,10 +39,13 @@ struct vsock_sock {
>  	 * modified outsided of socket create or destruct.
>  	 */
>  	bool trusted;
> -	bool cached_peer_allow_dgram;	/* Dgram communication allowed to
> -					 * cached peer?
> -					 */
> -	u32 cached_peer;  /* Context ID of last dgram destination check. */
> +	/* Cached dgram access decision for the last peer, packed as
> +	 * (cid << 32) | VALID | ALLOW and accessed via READ_ONCE()/
> +	 * WRITE_ONCE() so the lockless receive tasklet and the
> +	 * lock_sock() send path cannot race to a stale decision.
> +	 * See vmci_transport_allow_dgram().
> +	 */
> +	u64 cached_peer_access;

[Severity: Low]
This isn't a bug introduced by this patch (the cached_peer and
cached_peer_allow_dgram fields already lived here), but since the
representation is being rewritten anyway: could the cache move into the
VMCI transport's own state (vsk->trans, struct vmci_transport) instead?

grep shows cached_peer_access is read and written only from
net/vmw_vsock/vmci_transport.c, yet the core header now documents a
VMCI-specific bit packing convention and points at
vmci_transport_allow_dgram(), which the virtio, hyperv and loopback
transports never use.

>  	const struct cred *owner;
>  	/* Rest are SOCK_STREAM only. */
>  	long connect_timeout;
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 1f186e8f8364..31728e26a770 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -524,23 +524,38 @@ static bool vmci_transport_is_trusted(struct vsock_sock *vsock, u32 peer_cid)
>   * only if it is trusted as described in vmci_transport_is_trusted.
>   */
>  
> +/* Packing for vsk->cached_peer_access. */
> +#define VMCI_DGRAM_ACCESS_VALID		BIT_ULL(0)
> +#define VMCI_DGRAM_ACCESS_ALLOW		BIT_ULL(1)
> +#define VMCI_DGRAM_ACCESS_CID_SHIFT	32
> +
>  static bool vmci_transport_allow_dgram(struct vsock_sock *vsock, u32 peer_cid)
>  {
> +	u64 access;
> +
>  	if (VMADDR_CID_HYPERVISOR == peer_cid)
>  		return true;
>  
> -	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;
> -		}
> -	}
> -
> -	return vsock->cached_peer_allow_dgram;
> +	/* Cache the trusted/restricted decision for the last peer to avoid the
> +	 * O(N) vmci_ctx_get() lookup on every datagram.  Read/update it through
> +	 * a single word so a race between the lockless receive tasklet and the
> +	 * lock_sock() send path only forces a recompute -- it can never return a
> +	 * stale allow for a restricted peer.
> +	 */
> +	access = READ_ONCE(vsock->cached_peer_access);
> +	if ((access & VMCI_DGRAM_ACCESS_VALID) &&
> +	    (u32)(access >> VMCI_DGRAM_ACCESS_CID_SHIFT) == peer_cid)
> +		return !!(access & VMCI_DGRAM_ACCESS_ALLOW);

[Severity: High]
Does the "single word" property actually hold on 32-bit builds?  The CID
lives in bits 32-63 and VALID/ALLOW in bits 0-1, so the tag and the
decision are in two different 32-bit access units, and a plain u64
READ_ONCE()/WRITE_ONCE() is not single-copy atomic there.
include/asm-generic/rwonce.h is explicit about it:

/*
 * Yes, this permits 64-bit accesses on 32-bit architectures. These will
 * actually be atomic in some cases (namely Armv7 + LPAE), but for others we
 * rely on the access being split into 2x32-bit accesses for a 32-bit quantity
 * (e.g. a virtual address) and a strong prevailing wind.
 */

CONFIG_VMWARE_VMCI builds on 32-bit x86 as well:

drivers/misc/vmw_vmci/Kconfig:
	depends on (X86 || ARM64) && !CPU_BIG_ENDIAN && PCI

So with the cache holding (A << 32) | VALID | ALLOW, and
vmci_transport_dgram_enqueue() -> vmci_transport_allow_dgram() publishing
(B << 32) | VALID for a restricted peer B:

CPU0 (sendmsg, lock_sock held, i386: two 32-bit stores)
	store high word: CID = B
	<-- softirq / other CPU runs here
	store low word:  VALID (ALLOW cleared)

CPU1 (vmci_transport_recv_dgram_cb(), receive tasklet, no socket lock)
	access = READ_ONCE(vsock->cached_peer_access);
	  sees high word B, low word still VALID | ALLOW
	returns true for the restricted peer B

The reader side can tear the same way, since gcc on i386 loads the low word
first.  Since lock_sock() does not exclude softirqs, this also happens on a
single CPU.

Would an atomic64_t / cmpxchg64 based field, a seqcount, or an encoding
that fits in one native word (for example a separate validity/allow flag
plus a CID compared after the flags load, or a 30-bit CID plus two flag
bits in an unsigned long) close this for CONFIG_X86_32?

As written, the unconditional claims in the changelog ("pack the peer CID
and the decision into a single word accessed with READ_ONCE()/WRITE_ONCE().
A race then only forces a recompute and can never return a stale allow"),
in the new function comment, and in the new struct vsock_sock comment hold
only on 64-bit.  Could those be either fixed or scoped?

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, since the
old "if (vsock->cached_peer != peer_cid)" logic had the same lifetime
semantics, but the cache-hit path above is keyed on the peer CID alone and
is never invalidated.  Can it outlive the context it describes?

vmci_ctx_create() only avoids CIDs of contexts that are currently live:

drivers/misc/vmw_vmci/vmci_context.c:vmci_ctx_create() {
	while (vmci_ctx_exists(cid)) {
		/* We reserve the lowest 16 ids for fixed contexts. */
		cid = max(cid, VMCI_RESERVED_CID_LIMIT - 1) + 1;
	...
}

and IOCTL_VMCI_INIT_CONTEXT lets the caller pick both the CID and the
privilege flags:

drivers/misc/vmw_vmci/vmci_host.c:vmci_host_do_init_context() {
	vmci_host_dev->context = vmci_ctx_create(init_block.cid,
						 init_block.flags, 0,
	...
}

So an ALLOW cached for CID X can survive destruction of that context, and a
later context reusing CID X with VMCI_PRIVILEGE_FLAG_RESTRICTED (or a
different owning uid) keeps being allowed without vmci_context_get_priv_flags()
being consulted again.  The inverse direction exists too:
vmci_context_get_priv_flags() returns VMCI_NO_PRIVILEGE_FLAGS while
!vmci_host_code_active(), so an ALLOW cached in that window survives host
personality activation.

Would a generation counter bumped on VMCI context create/destroy, folded
into the cached word, address this?

> +
> +	access = VMCI_DGRAM_ACCESS_VALID |
> +		 ((u64)peer_cid << VMCI_DGRAM_ACCESS_CID_SHIFT);
> +	if (vmci_transport_is_trusted(vsock, peer_cid) ||
> +	    !(vmci_context_get_priv_flags(peer_cid) &
> +	      VMCI_PRIVILEGE_FLAG_RESTRICTED))
> +		access |= VMCI_DGRAM_ACCESS_ALLOW;
> +
> +	WRITE_ONCE(vsock->cached_peer_access, access);

[Severity: Low]
Should the two call-site comments be updated in the same patch?  Both
lockless callers still say the opposite of the new struct vsock_sock
comment:

net/vmw_vsock/vmci_transport.c:vmci_transport_recv_dgram_cb() {
	...
	 * NOTE: We access the socket struct without holding the lock here.
	 * This is ok because the field we are interested is never modified
	 * outside of the create and destruct socket functions.
	 */
	vsk = vsock_sk(sk);
	if (!vmci_transport_allow_dgram(vsk, dg->src.context))
	...
}

The same text sits above the call in vmci_transport_recv_stream_cb(), and
both copies are still there at the end of the series (45f790ccc007).  The
helper now does WRITE_ONCE(vsock->cached_peer_access, access) from exactly
that tasklet context, and the changelog already calls the comment out ("The
in-code comment claiming the fields are never modified outside
create/destruct is contradicted by the send path") without touching it.

> +	return !!(access & VMCI_DGRAM_ACCESS_ALLOW);
>  }
>  
>  static int

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919123208.29032-1-bartlomiej.dmitruk%40isec.pl