[PATCH RFC net v2 0/3] vsock: Fix transport_{h2g,g2h,dgram,local} TOCTOU issues

[PATCH RFC net v2 0/3] vsock: Fix transport_{h2g,g2h,dgram,local} TOCTOU issues

[Michal Luczaj]

transport_{h2g,g2h,dgram,local} may become NULL on vsock_core_unregister().
Make sure a poorly timed `rmmod transport` won't lead to a NULL/stale
pointer dereference.

Note that these oopses are pretty unlikely to happen in the wild. Splats
were collected after sprinkling kernel with mdelay()s.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Changes in v2:
- Introduce a helper function to get local CIDs safely [Stefano]
- Rename goto label to indicate an error path, explain why releasing
  vsock_register_mutex after try_module_get() is safe [Stefano]
- Link to v1: https://lore.kernel.org/r/20250618-vsock-transports-toctou-v1-0-dd2d2ede9052@rbox.co

---
Michal Luczaj (3):
      vsock: Fix transport_{g2h,h2g} TOCTOU
      vsock: Fix transport_* TOCTOU
      vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

 net/vmw_vsock/af_vsock.c | 53 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 11 deletions(-)
---
base-commit: e0fca6f2cebff539e9317a15a37dcf432e3b851a
change-id: 20250523-vsock-transports-toctou-4b75d9c2a805

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj]

vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
transport_{g2h,h2g} may become NULL after the NULL check.

Introduce vsock_transport_local_cid() to protect from a potential
null-ptr-deref.

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_find_cid+0x47/0x90
Call Trace:
 __vsock_bind+0x4b2/0x720
 vsock_bind+0x90/0xe0
 __sys_bind+0x14d/0x1e0
 __x64_sys_bind+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
Call Trace:
 __x64_sys_ioctl+0x12d/0x190
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 }
 EXPORT_SYMBOL_GPL(vsock_assign_transport);
 
+static u32 vsock_transport_local_cid(const struct vsock_transport **transport)
+{
+	u32 cid = VMADDR_CID_ANY;
+
+	mutex_lock(&vsock_register_mutex);
+	if (*transport)
+		cid = (*transport)->get_local_cid();
+	mutex_unlock(&vsock_register_mutex);
+
+	return cid;
+}
+
 bool vsock_find_cid(unsigned int cid)
 {
-	if (transport_g2h && cid == transport_g2h->get_local_cid())
+	if (cid == vsock_transport_local_cid(&transport_g2h))
 		return true;
 
 	if (transport_h2g && cid == VMADDR_CID_HOST)
@@ -2536,18 +2548,17 @@ static long vsock_dev_do_ioctl(struct file *filp,
 			       unsigned int cmd, void __user *ptr)
 {
 	u32 __user *p = ptr;
-	u32 cid = VMADDR_CID_ANY;
 	int retval = 0;
+	u32 cid;
 
 	switch (cmd) {
 	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
 		/* To be compatible with the VMCI behavior, we prioritize the
 		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
 		 */
-		if (transport_g2h)
-			cid = transport_g2h->get_local_cid();
-		else if (transport_h2g)
-			cid = transport_h2g->get_local_cid();
+		cid = vsock_transport_local_cid(&transport_g2h);
+		if (cid == VMADDR_CID_ANY)
+			cid = vsock_transport_local_cid(&transport_h2g);
 
 		if (put_user(cid, p) != 0)
 			retval = -EFAULT;

-- 
2.49.0

[PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Michal Luczaj]

Transport assignment may race with module unload. Protect new_transport
from becoming a stale pointer.

This also takes care of an insecure call in vsock_use_local_transport();
add a lockdep assert.

BUG: unable to handle page fault for address: fffffbfff8056000
Oops: Oops: 0000 [#1] SMP KASAN
RIP: 0010:vsock_assign_transport+0x366/0x600
Call Trace:
 vsock_connect+0x59c/0xc40
 __sys_connect+0xe8/0x100
 __x64_sys_connect+0x6e/0xc0
 do_syscall_64+0x92/0x1c0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53

Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
 
 static bool vsock_use_local_transport(unsigned int remote_cid)
 {
+	lockdep_assert_held(&vsock_register_mutex);
+
 	if (!transport_local)
 		return false;
 
@@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 
 	remote_flags = vsk->remote_addr.svm_flags;
 
+	mutex_lock(&vsock_register_mutex);
+
 	switch (sk->sk_type) {
 	case SOCK_DGRAM:
 		new_transport = transport_dgram;
@@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 			new_transport = transport_h2g;
 		break;
 	default:
-		return -ESOCKTNOSUPPORT;
+		ret = -ESOCKTNOSUPPORT;
+		goto err;
 	}
 
 	if (vsk->transport) {
-		if (vsk->transport == new_transport)
-			return 0;
+		if (vsk->transport == new_transport) {
+			ret = 0;
+			goto err;
+		}
 
 		/* transport->release() must be called with sock lock acquired.
 		 * This path can only be taken during vsock_connect(), where we
@@ -508,8 +515,16 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	/* We increase the module refcnt to prevent the transport unloading
 	 * while there are open sockets assigned to it.
 	 */
-	if (!new_transport || !try_module_get(new_transport->module))
-		return -ENODEV;
+	if (!new_transport || !try_module_get(new_transport->module)) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	/* It's safe to release the mutex after a successful try_module_get().
+	 * Whichever transport `new_transport` points at, it won't go await
+	 * until the last module_put() below or in vsock_deassign_transport().
+	 */
+	mutex_unlock(&vsock_register_mutex);
 
 	if (sk->sk_type == SOCK_SEQPACKET) {
 		if (!new_transport->seqpacket_allow ||
@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	vsk->transport = new_transport;
 
 	return 0;
+err:
+	mutex_unlock(&vsock_register_mutex);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vsock_assign_transport);
 

-- 
2.49.0

[PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Michal Luczaj]

Support returning VMADDR_CID_LOCAL in case no other vsock transport is
available.

Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:

   Ioctls
       ...
       IOCTL_VM_SOCKETS_GET_LOCAL_CID
              ...
              Consider using VMADDR_CID_ANY when binding instead of
              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.

   Local communication
       ....
       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
       used for the same purpose, but it is preferable to use
       VMADDR_CID_LOCAL.

I was wondering it that would need some rewriting, since we're adding
VMADDR_CID_LOCAL as a possible ioctl's return value.
---
 net/vmw_vsock/af_vsock.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
 		cid = vsock_transport_local_cid(&transport_g2h);
 		if (cid == VMADDR_CID_ANY)
 			cid = vsock_transport_local_cid(&transport_h2g);
+		if (cid == VMADDR_CID_ANY && transport_local)
+			cid = VMADDR_CID_LOCAL;
 
 		if (put_user(cid, p) != 0)
 			retval = -EFAULT;

-- 
2.49.0

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Stefano Garzarella]

On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>transport_{g2h,h2g} may become NULL after the NULL check.
>
>Introduce vsock_transport_local_cid() to protect from a potential
>null-ptr-deref.
>
>KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>RIP: 0010:vsock_find_cid+0x47/0x90
>Call Trace:
> __vsock_bind+0x4b2/0x720
> vsock_bind+0x90/0xe0
> __sys_bind+0x14d/0x1e0
> __x64_sys_bind+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>Call Trace:
> __x64_sys_ioctl+0x12d/0x190
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> }
> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>
>+static u32 vsock_transport_local_cid(const struct vsock_transport **transport)

Why we need double pointer?

>+{
>+	u32 cid = VMADDR_CID_ANY;
>+
>+	mutex_lock(&vsock_register_mutex);
>+	if (*transport)
>+		cid = (*transport)->get_local_cid();
>+	mutex_unlock(&vsock_register_mutex);
>+
>+	return cid;
>+}
>+
> bool vsock_find_cid(unsigned int cid)
> {
>-	if (transport_g2h && cid == transport_g2h->get_local_cid())
>+	if (cid == vsock_transport_local_cid(&transport_g2h))
> 		return true;
>
> 	if (transport_h2g && cid == VMADDR_CID_HOST)
>@@ -2536,18 +2548,17 @@ static long vsock_dev_do_ioctl(struct file *filp,
> 			       unsigned int cmd, void __user *ptr)
> {
> 	u32 __user *p = ptr;
>-	u32 cid = VMADDR_CID_ANY;
> 	int retval = 0;
>+	u32 cid;
>
> 	switch (cmd) {
> 	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
> 		/* To be compatible with the VMCI behavior, we prioritize the
> 		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
> 		 */
>-		if (transport_g2h)
>-			cid = transport_g2h->get_local_cid();
>-		else if (transport_h2g)
>-			cid = transport_h2g->get_local_cid();
>+		cid = vsock_transport_local_cid(&transport_g2h);
>+		if (cid == VMADDR_CID_ANY)
>+			cid = vsock_transport_local_cid(&transport_h2g);

I still prefer the old `if ... else if ...`, what is the reason of this
change? I may miss the point.

But overall LGTM!

Thanks,
Stefano

>
> 		if (put_user(cid, p) != 0)
> 			retval = -EFAULT;
>
>-- 
>2.49.0
>

Re: [PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Stefano Garzarella]

On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:
>Transport assignment may race with module unload. Protect new_transport
>from becoming a stale pointer.
>
>This also takes care of an insecure call in vsock_use_local_transport();
>add a lockdep assert.
>
>BUG: unable to handle page fault for address: fffffbfff8056000
>Oops: Oops: 0000 [#1] SMP KASAN
>RIP: 0010:vsock_assign_transport+0x366/0x600
>Call Trace:
> vsock_connect+0x59c/0xc40
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)

LGTM!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>
> static bool vsock_use_local_transport(unsigned int remote_cid)
> {
>+	lockdep_assert_held(&vsock_register_mutex);
>+
> 	if (!transport_local)
> 		return false;
>
>@@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>
> 	remote_flags = vsk->remote_addr.svm_flags;
>
>+	mutex_lock(&vsock_register_mutex);
>+
> 	switch (sk->sk_type) {
> 	case SOCK_DGRAM:
> 		new_transport = transport_dgram;
>@@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 			new_transport = transport_h2g;
> 		break;
> 	default:
>-		return -ESOCKTNOSUPPORT;
>+		ret = -ESOCKTNOSUPPORT;
>+		goto err;
> 	}
>
> 	if (vsk->transport) {
>-		if (vsk->transport == new_transport)
>-			return 0;
>+		if (vsk->transport == new_transport) {
>+			ret = 0;
>+			goto err;
>+		}
>
> 		/* transport->release() must be called with sock lock acquired.
> 		 * This path can only be taken during vsock_connect(), where we
>@@ -508,8 +515,16 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	/* We increase the module refcnt to prevent the transport unloading
> 	 * while there are open sockets assigned to it.
> 	 */
>-	if (!new_transport || !try_module_get(new_transport->module))
>-		return -ENODEV;
>+	if (!new_transport || !try_module_get(new_transport->module)) {
>+		ret = -ENODEV;
>+		goto err;
>+	}
>+
>+	/* It's safe to release the mutex after a successful try_module_get().
>+	 * Whichever transport `new_transport` points at, it won't go await
>+	 * until the last module_put() below or in vsock_deassign_transport().
>+	 */
>+	mutex_unlock(&vsock_register_mutex);
>
> 	if (sk->sk_type == SOCK_SEQPACKET) {
> 		if (!new_transport->seqpacket_allow ||
>@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	vsk->transport = new_transport;
>
> 	return 0;
>+err:
>+	mutex_unlock(&vsock_register_mutex);
>+	return ret;
> }
> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>
>
>-- 
>2.49.0
>

Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Stefano Garzarella]

On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>available.
>
>Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
>man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>
>   Ioctls
>       ...
>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>              ...
>              Consider using VMADDR_CID_ANY when binding instead of
>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>
>   Local communication
>       ....
>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>       used for the same purpose, but it is preferable to use
>       VMADDR_CID_LOCAL.
>
>I was wondering it that would need some rewriting, since we're adding
>VMADDR_CID_LOCAL as a possible ioctl's return value.

IIRC the reason was, that if we have for example a G2H module loaded, 
the ioctl returns the CID of that module (e.g. 42). So, we can use both 
42 and VMADDR_CID_LOCAL to do the loopback communication, but we 
encourage to always use VMADDR_CID_LOCAL.  With this change we basically 
don't change that, but we change the fact that if there is only the 
loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while 
now it returns LOCAL rightly.

So, IMO we are fine.

>---
> net/vmw_vsock/af_vsock.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
> 		cid = vsock_transport_local_cid(&transport_g2h);
> 		if (cid == VMADDR_CID_ANY)
> 			cid = vsock_transport_local_cid(&transport_h2g);
>+		if (cid == VMADDR_CID_ANY && transport_local)
>+			cid = VMADDR_CID_LOCAL;

why not `cid = vsock_transport_local_cid(&transport_local)` like for 
H2G?

Thanks,
Stefano

>
> 		if (put_user(cid, p) != 0)
> 			retval = -EFAULT;
>
>-- 
>2.49.0
>

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj]

On 6/25/25 10:43, Stefano Garzarella wrote:
> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>> transport_{g2h,h2g} may become NULL after the NULL check.
>>
>> Introduce vsock_transport_local_cid() to protect from a potential
>> null-ptr-deref.
>>
>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>> RIP: 0010:vsock_find_cid+0x47/0x90
>> Call Trace:
>> __vsock_bind+0x4b2/0x720
>> vsock_bind+0x90/0xe0
>> __sys_bind+0x14d/0x1e0
>> __x64_sys_bind+0x6e/0xc0
>> do_syscall_64+0x92/0x1c0
>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>
>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>> Call Trace:
>> __x64_sys_ioctl+0x12d/0x190
>> do_syscall_64+0x92/0x1c0
>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>
>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>> }
>> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>>
>> +static u32 vsock_transport_local_cid(const struct vsock_transport **transport)
> 
> Why we need double pointer?

Because of a possible race. If @transport is `struct vsock_transport*` and
we pass `transport_g2h`, the passed non-NULL pointer value may immediately
become stale (due to module unload). But if it's `vsock_transport**` and we
pass `&transport_g2h`, then we can take the mutex, check `*transport` for
NULL and safely go ahead.

Or are you saying this could be simplified?

>> +{
>> +	u32 cid = VMADDR_CID_ANY;
>> +
>> +	mutex_lock(&vsock_register_mutex);
>> +	if (*transport)
>> +		cid = (*transport)->get_local_cid();
>> +	mutex_unlock(&vsock_register_mutex);
>> +
>> +	return cid;
>> +}
>> +
>> bool vsock_find_cid(unsigned int cid)
>> {
>> -	if (transport_g2h && cid == transport_g2h->get_local_cid())
>> +	if (cid == vsock_transport_local_cid(&transport_g2h))
>> 		return true;
>>
>> 	if (transport_h2g && cid == VMADDR_CID_HOST)
>> @@ -2536,18 +2548,17 @@ static long vsock_dev_do_ioctl(struct file *filp,
>> 			       unsigned int cmd, void __user *ptr)
>> {
>> 	u32 __user *p = ptr;
>> -	u32 cid = VMADDR_CID_ANY;
>> 	int retval = 0;
>> +	u32 cid;
>>
>> 	switch (cmd) {
>> 	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
>> 		/* To be compatible with the VMCI behavior, we prioritize the
>> 		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
>> 		 */
>> -		if (transport_g2h)
>> -			cid = transport_g2h->get_local_cid();
>> -		else if (transport_h2g)
>> -			cid = transport_h2g->get_local_cid();
>> +		cid = vsock_transport_local_cid(&transport_g2h);
>> +		if (cid == VMADDR_CID_ANY)
>> +			cid = vsock_transport_local_cid(&transport_h2g);
> 
> I still prefer the old `if ... else if ...`, what is the reason of this
> change? I may miss the point.

Ah, ok, I've just thought such cascade would be cleaner.

So is this what you prefer?

if (transport_g2h)
	cid = vsock_transport_local_cid(&transport_g2h);
else if (transport_h2g)
	cid = vsock_transport_local_cid(&transport_h2g);

Thanks,
Michal

Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Michal Luczaj]

On 6/25/25 10:54, Stefano Garzarella wrote:
> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>> available.
>>
>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>
>>   Ioctls
>>       ...
>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>              ...
>>              Consider using VMADDR_CID_ANY when binding instead of
>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>
>>   Local communication
>>       ....
>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>       used for the same purpose, but it is preferable to use
>>       VMADDR_CID_LOCAL.
>>
>> I was wondering it that would need some rewriting, since we're adding
>> VMADDR_CID_LOCAL as a possible ioctl's return value.
> 
> IIRC the reason was, that if we have for example a G2H module loaded, 
> the ioctl returns the CID of that module (e.g. 42). So, we can use both 
> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we 
> encourage to always use VMADDR_CID_LOCAL.  With this change we basically 
> don't change that, but we change the fact that if there is only the 
> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while 
> now it returns LOCAL rightly.
> 
> So, IMO we are fine.

All right, got it.

>> ---
>> net/vmw_vsock/af_vsock.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>> 		cid = vsock_transport_local_cid(&transport_g2h);
>> 		if (cid == VMADDR_CID_ANY)
>> 			cid = vsock_transport_local_cid(&transport_h2g);
>> +		if (cid == VMADDR_CID_ANY && transport_local)
>> +			cid = VMADDR_CID_LOCAL;
> 
> why not `cid = vsock_transport_local_cid(&transport_local)` like for 
> H2G?

Sure, can do. I've assumed transport_local would always have a local CID of
VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
it symmetrical with the other vsock_transport_local_cid()s.

Thanks,
Michal

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Stefano Garzarella]

On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>On 6/25/25 10:43, Stefano Garzarella wrote:
>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>
>>> Introduce vsock_transport_local_cid() to protect from a potential
>>> null-ptr-deref.
>>>
>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>> Call Trace:
>>> __vsock_bind+0x4b2/0x720
>>> vsock_bind+0x90/0xe0
>>> __sys_bind+0x14d/0x1e0
>>> __x64_sys_bind+0x6e/0xc0
>>> do_syscall_64+0x92/0x1c0
>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>
>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>> Call Trace:
>>> __x64_sys_ioctl+0x12d/0x190
>>> do_syscall_64+0x92/0x1c0
>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>
>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>> }
>>> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>>>
>>> +static u32 vsock_transport_local_cid(const struct vsock_transport **transport)
>>
>> Why we need double pointer?
>
>Because of a possible race. If @transport is `struct vsock_transport*` and
>we pass `transport_g2h`, the passed non-NULL pointer value may immediately
>become stale (due to module unload). But if it's `vsock_transport**` and we
>pass `&transport_g2h`, then we can take the mutex, check `*transport` for
>NULL and safely go ahead.
>
>Or are you saying this could be simplified?

Nope, you're right! I was still thinking about my old version where we 
had the switch inside...

BTW I'd like to change the name, `vsock_transport_local` prefix is 
confusing IMO, since it seems related only to the `transport_local`.

Another thing I'm worried about is that we'll then start using it on 
`vsk->transport` when this is only to be used on registered transports 
(i.e. `static ...`), though, I don't think there's a way to force type 
checking from the compiler (unless you wrap it in a struct). It's not a 
big issue, but taking the mutex is useless in that cases.

So, if we can't do much, I'd add a comment and make the function name 
more clear. e.g. vsock_registered_transport_cid() ? or something 
similar.

>
>>> +{
>>> +	u32 cid = VMADDR_CID_ANY;
>>> +
>>> +	mutex_lock(&vsock_register_mutex);
>>> +	if (*transport)
>>> +		cid = (*transport)->get_local_cid();
>>> +	mutex_unlock(&vsock_register_mutex);
>>> +
>>> +	return cid;
>>> +}
>>> +
>>> bool vsock_find_cid(unsigned int cid)
>>> {
>>> -	if (transport_g2h && cid == transport_g2h->get_local_cid())
>>> +	if (cid == vsock_transport_local_cid(&transport_g2h))
>>> 		return true;
>>>
>>> 	if (transport_h2g && cid == VMADDR_CID_HOST)
>>> @@ -2536,18 +2548,17 @@ static long vsock_dev_do_ioctl(struct file *filp,
>>> 			       unsigned int cmd, void __user *ptr)
>>> {
>>> 	u32 __user *p = ptr;
>>> -	u32 cid = VMADDR_CID_ANY;
>>> 	int retval = 0;
>>> +	u32 cid;
>>>
>>> 	switch (cmd) {
>>> 	case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
>>> 		/* To be compatible with the VMCI behavior, we prioritize the
>>> 		 * guest CID instead of well-know host CID (VMADDR_CID_HOST).
>>> 		 */
>>> -		if (transport_g2h)
>>> -			cid = transport_g2h->get_local_cid();
>>> -		else if (transport_h2g)
>>> -			cid = transport_h2g->get_local_cid();
>>> +		cid = vsock_transport_local_cid(&transport_g2h);
>>> +		if (cid == VMADDR_CID_ANY)
>>> +			cid = vsock_transport_local_cid(&transport_h2g);
>>
>> I still prefer the old `if ... else if ...`, what is the reason of this
>> change? I may miss the point.
>
>Ah, ok, I've just thought such cascade would be cleaner.
>
>So is this what you prefer?

I usually prefer less changes as possibile, but in this case I see your 
point, so up to you ;-)

In your way we save `cid` initialization and an if, so it's fine.

Thanks,
Stefano

>
>if (transport_g2h)
>	cid = vsock_transport_local_cid(&transport_g2h);
>else if (transport_h2g)
>	cid = vsock_transport_local_cid(&transport_h2g);
>
>Thanks,
>Michal
>

Re: [PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Stefano Garzarella]

On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:
>Transport assignment may race with module unload. Protect new_transport
>from becoming a stale pointer.
>
>This also takes care of an insecure call in vsock_use_local_transport();
>add a lockdep assert.
>
>BUG: unable to handle page fault for address: fffffbfff8056000
>Oops: Oops: 0000 [#1] SMP KASAN
>RIP: 0010:vsock_assign_transport+0x366/0x600
>Call Trace:
> vsock_connect+0x59c/0xc40
> __sys_connect+0xe8/0x100
> __x64_sys_connect+0x6e/0xc0
> do_syscall_64+0x92/0x1c0
> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
>Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
> 1 file changed, 23 insertions(+), 5 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>
> static bool vsock_use_local_transport(unsigned int remote_cid)
> {
>+	lockdep_assert_held(&vsock_register_mutex);
>+
> 	if (!transport_local)
> 		return false;
>
>@@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>
> 	remote_flags = vsk->remote_addr.svm_flags;
>
>+	mutex_lock(&vsock_register_mutex);
>+
> 	switch (sk->sk_type) {
> 	case SOCK_DGRAM:
> 		new_transport = transport_dgram;
>@@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 			new_transport = transport_h2g;
> 		break;
> 	default:
>-		return -ESOCKTNOSUPPORT;
>+		ret = -ESOCKTNOSUPPORT;
>+		goto err;
> 	}
>
> 	if (vsk->transport) {
>-		if (vsk->transport == new_transport)
>-			return 0;
>+		if (vsk->transport == new_transport) {
>+			ret = 0;
>+			goto err;
>+		}

		/* transport->release() must be called with sock lock acquired.
		 * This path can only be taken during vsock_connect(), where we
		 * have already held the sock lock. In the other cases, this
		 * function is called on a new socket which is not assigned to
		 * any transport.
		 */
		vsk->transport->release(vsk);
		vsock_deassign_transport(vsk);

Thinking back to this patch, could there be a deadlock between call
vsock_deassign_transport(), which call module_put(), now with the
`vsock_register_mutex` held, and the call to vsock_core_unregister()
usually made by modules in the exit function?

Thanks,
Stefano

>
> 		/* transport->release() must be called with sock lock acquired.
> 		 * This path can only be taken during vsock_connect(), where we
>@@ -508,8 +515,16 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	/* We increase the module refcnt to prevent the transport unloading
> 	 * while there are open sockets assigned to it.
> 	 */
>-	if (!new_transport || !try_module_get(new_transport->module))
>-		return -ENODEV;
>+	if (!new_transport || !try_module_get(new_transport->module)) {
>+		ret = -ENODEV;
>+		goto err;
>+	}
>+
>+	/* It's safe to release the mutex after a successful try_module_get().
>+	 * Whichever transport `new_transport` points at, it won't go await
>+	 * until the last module_put() below or in vsock_deassign_transport().
>+	 */
>+	mutex_unlock(&vsock_register_mutex);
>
> 	if (sk->sk_type == SOCK_SEQPACKET) {
> 		if (!new_transport->seqpacket_allow ||
>@@ -528,6 +543,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	vsk->transport = new_transport;
>
> 	return 0;
>+err:
>+	mutex_unlock(&vsock_register_mutex);
>+	return ret;
> }
> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>
>
>-- 
>2.49.0
>

Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Stefano Garzarella]

On Wed, Jun 25, 2025 at 11:23:54PM +0200, Michal Luczaj wrote:
>On 6/25/25 10:54, Stefano Garzarella wrote:
>> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>>> available.
>>>
>>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>>
>>>   Ioctls
>>>       ...
>>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>>              ...
>>>              Consider using VMADDR_CID_ANY when binding instead of
>>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>>
>>>   Local communication
>>>       ....
>>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>>       used for the same purpose, but it is preferable to use
>>>       VMADDR_CID_LOCAL.
>>>
>>> I was wondering it that would need some rewriting, since we're adding
>>> VMADDR_CID_LOCAL as a possible ioctl's return value.
>>
>> IIRC the reason was, that if we have for example a G2H module loaded,
>> the ioctl returns the CID of that module (e.g. 42). So, we can use both
>> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we
>> encourage to always use VMADDR_CID_LOCAL.  With this change we basically
>> don't change that, but we change the fact that if there is only the
>> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while
>> now it returns LOCAL rightly.
>>
>> So, IMO we are fine.
>
>All right, got it.
>
>>> ---
>>> net/vmw_vsock/af_vsock.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>>> 		cid = vsock_transport_local_cid(&transport_g2h);
>>> 		if (cid == VMADDR_CID_ANY)
>>> 			cid = vsock_transport_local_cid(&transport_h2g);
>>> +		if (cid == VMADDR_CID_ANY && transport_local)
>>> +			cid = VMADDR_CID_LOCAL;
>>
>> why not `cid = vsock_transport_local_cid(&transport_local)` like for
>> H2G?
>
>Sure, can do. I've assumed transport_local would always have a local CID of
>VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
>get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
>it symmetrical with the other vsock_transport_local_cid()s.

Yeah, BTW for transport_h2g is the same, they always should return 
VMADDR_CID_HOST, so I think we should be symmetrical.

Thanks,
Stefano

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj]

On 6/27/25 10:02, Stefano Garzarella wrote:
> On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>> On 6/25/25 10:43, Stefano Garzarella wrote:
>>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>>
>>>> Introduce vsock_transport_local_cid() to protect from a potential
>>>> null-ptr-deref.
>>>>
>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>>> Call Trace:
>>>> __vsock_bind+0x4b2/0x720
>>>> vsock_bind+0x90/0xe0
>>>> __sys_bind+0x14d/0x1e0
>>>> __x64_sys_bind+0x6e/0xc0
>>>> do_syscall_64+0x92/0x1c0
>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>
>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>>> Call Trace:
>>>> __x64_sys_ioctl+0x12d/0x190
>>>> do_syscall_64+0x92/0x1c0
>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>
>>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
>>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>>> }
>>>> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>>>>
>>>> +static u32 vsock_transport_local_cid(const struct vsock_transport **transport)
>>>
>>> Why we need double pointer?
>>
>> Because of a possible race. If @transport is `struct vsock_transport*` and
>> we pass `transport_g2h`, the passed non-NULL pointer value may immediately
>> become stale (due to module unload). But if it's `vsock_transport**` and we
>> pass `&transport_g2h`, then we can take the mutex, check `*transport` for
>> NULL and safely go ahead.
>>
>> Or are you saying this could be simplified?
> 
> Nope, you're right! I was still thinking about my old version where we 
> had the switch inside...
> 
> BTW I'd like to change the name, `vsock_transport_local` prefix is 
> confusing IMO, since it seems related only to the `transport_local`.
> 
> Another thing I'm worried about is that we'll then start using it on 
> `vsk->transport` when this is only to be used on registered transports 
> (i.e. `static ...`), though, I don't think there's a way to force type 
> checking from the compiler (unless you wrap it in a struct). (...)

I've found (on SO[1]) this somewhat hackish compile-time `static`-checking:

static u32 __vsock_registered_transport_cid(const struct vsock_transport
**transport)
{
	u32 cid = VMADDR_CID_ANY;

	mutex_lock(&vsock_register_mutex);
	if (*transport)
		cid = (*transport)->get_local_cid();
	mutex_unlock(&vsock_register_mutex);

	return cid;
}

#define ASSERT_REGISTERED_TRANSPORT(t)					\
	__always_unused static void *__UNIQUE_ID(vsock) = (t)

#define vsock_registered_transport_cid(transport)			\
({									\
	ASSERT_REGISTERED_TRANSPORT(transport);				\
	__vsock_registered_transport_cid(transport);			\
})

It does the trick, compilation fails on
vsock_registered_transport_cid(&vsk->transport):

net/vmw_vsock/af_vsock.c: In function ‘vsock_send_shutdown’:
net/vmw_vsock/af_vsock.c:565:59: error: initializer element is not constant
  565 |         __always_unused static void *__UNIQUE_ID(vsock) = (t)
      |                                                           ^
net/vmw_vsock/af_vsock.c:569:9: note: in expansion of macro
‘ASSERT_REGISTERED_TRANSPORT’
  569 |         ASSERT_REGISTERED_TRANSPORT(transport);
    \
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
net/vmw_vsock/af_vsock.c:626:9: note: in expansion of macro
‘vsock_registered_transport_cid’
  626 |         vsock_registered_transport_cid(&vsk->transport);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

But perhaps adding a comment wouldn't hurt either, e.g.

/* Provide safe access to static transport_{h2g,g2h,dgram,local} callbacks.
 * Otherwise we may race with module removal. Do not use on
 * `vsk->transport`.
 */

? ...which begs another question: do we stick to the netdev special comment
style? See commit 82b8000c28b5 ("net: drop special comment style").

Oh, and come to think of it, we don't really need that (easily contended?)
mutex here. Same can be done with RCU. Which should speed up vsock_bind()
-> __vsock_bind() -> vsock_find_cid(), right? This is what I mean, roughly:

+static u32 vsock_registered_transport_cid(const struct vsock_transport
__rcu **trans_ptr)
+{
+	const struct vsock_transport *transport;
+	u32 cid = VMADDR_CID_ANY;
+
+	rcu_read_lock();
+	transport = rcu_dereference(*trans_ptr);
+	if (transport)
+		cid = transport->get_local_cid();
+	rcu_read_unlock();
+
+	return cid;
+}
...
@@ -2713,6 +2726,7 @@ void vsock_core_unregister(const struct
vsock_transport *t)
 		transport_local = NULL;

 	mutex_unlock(&vsock_register_mutex);
+	synchronize_rcu();
 }

I've realized I'm throwing multiple unrelated ideas/questions, so let me
summarise:
1. Hackish macro can be used to guard against calling
vsock_registered_transport_cid() on a non-static variable.
2. We can comment the function to add some context and avoid confusion.
3. Instead of taking mutex in vsock_registered_transport_cid() we can use RCU.

> So, if we can't do much, I'd add a comment and make the function name 
> more clear. e.g. vsock_registered_transport_cid() ? or something 
> similar.

Sure, will do.

Thanks!

[1]:
https://stackoverflow.com/questions/5645695/how-can-i-add-a-static-assert-to-check-if-a-variable-is-static/5672637#5672637

Re: [PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Michal Luczaj]

On 6/27/25 10:08, Stefano Garzarella wrote:
> On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:
>> Transport assignment may race with module unload. Protect new_transport
>>from becoming a stale pointer.
>>
>> This also takes care of an insecure call in vsock_use_local_transport();
>> add a lockdep assert.
>>
>> BUG: unable to handle page fault for address: fffffbfff8056000
>> Oops: Oops: 0000 [#1] SMP KASAN
>> RIP: 0010:vsock_assign_transport+0x366/0x600
>> Call Trace:
>> vsock_connect+0x59c/0xc40
>> __sys_connect+0xe8/0x100
>> __x64_sys_connect+0x6e/0xc0
>> do_syscall_64+0x92/0x1c0
>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>
>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
>> 1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>>
>> static bool vsock_use_local_transport(unsigned int remote_cid)
>> {
>> +	lockdep_assert_held(&vsock_register_mutex);
>> +
>> 	if (!transport_local)
>> 		return false;
>>
>> @@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>
>> 	remote_flags = vsk->remote_addr.svm_flags;
>>
>> +	mutex_lock(&vsock_register_mutex);
>> +
>> 	switch (sk->sk_type) {
>> 	case SOCK_DGRAM:
>> 		new_transport = transport_dgram;
>> @@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>> 			new_transport = transport_h2g;
>> 		break;
>> 	default:
>> -		return -ESOCKTNOSUPPORT;
>> +		ret = -ESOCKTNOSUPPORT;
>> +		goto err;
>> 	}
>>
>> 	if (vsk->transport) {
>> -		if (vsk->transport == new_transport)
>> -			return 0;
>> +		if (vsk->transport == new_transport) {
>> +			ret = 0;
>> +			goto err;
>> +		}
> 
> 		/* transport->release() must be called with sock lock acquired.
> 		 * This path can only be taken during vsock_connect(), where we
> 		 * have already held the sock lock. In the other cases, this
> 		 * function is called on a new socket which is not assigned to
> 		 * any transport.
> 		 */
> 		vsk->transport->release(vsk);
> 		vsock_deassign_transport(vsk);
> 
> Thinking back to this patch, could there be a deadlock between call
> vsock_deassign_transport(), which call module_put(), now with the
> `vsock_register_mutex` held, and the call to vsock_core_unregister()
> usually made by modules in the exit function?

I think we're good. module_put() does not call the module cleanup function
(kernel/module/main.c:delete_module() syscall does that), so
vsock_core_unregister() won't happen in this path here. Have I missed
anything else?

Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

[Michal Luczaj]

On 6/27/25 10:10, Stefano Garzarella wrote:
> On Wed, Jun 25, 2025 at 11:23:54PM +0200, Michal Luczaj wrote:
>> On 6/25/25 10:54, Stefano Garzarella wrote:
>>> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>>>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>>>> available.
>>>>
>>>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>>>
>>>>   Ioctls
>>>>       ...
>>>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>>>              ...
>>>>              Consider using VMADDR_CID_ANY when binding instead of
>>>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>>>
>>>>   Local communication
>>>>       ....
>>>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>>>       used for the same purpose, but it is preferable to use
>>>>       VMADDR_CID_LOCAL.
>>>>
>>>> I was wondering it that would need some rewriting, since we're adding
>>>> VMADDR_CID_LOCAL as a possible ioctl's return value.
>>>
>>> IIRC the reason was, that if we have for example a G2H module loaded,
>>> the ioctl returns the CID of that module (e.g. 42). So, we can use both
>>> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we
>>> encourage to always use VMADDR_CID_LOCAL.  With this change we basically
>>> don't change that, but we change the fact that if there is only the
>>> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while
>>> now it returns LOCAL rightly.
>>>
>>> So, IMO we are fine.
>>
>> All right, got it.
>>
>>>> ---
>>>> net/vmw_vsock/af_vsock.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>>>> 		cid = vsock_transport_local_cid(&transport_g2h);
>>>> 		if (cid == VMADDR_CID_ANY)
>>>> 			cid = vsock_transport_local_cid(&transport_h2g);
>>>> +		if (cid == VMADDR_CID_ANY && transport_local)
>>>> +			cid = VMADDR_CID_LOCAL;
>>>
>>> why not `cid = vsock_transport_local_cid(&transport_local)` like for
>>> H2G?
>>
>> Sure, can do. I've assumed transport_local would always have a local CID of
>> VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
>> get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
>> it symmetrical with the other vsock_transport_local_cid()s.
> 
> Yeah, BTW for transport_h2g is the same, they always should return 
> VMADDR_CID_HOST, so I think we should be symmetrical.

Heh, I've missed that VMADDR_CID_HOST completely :)

Thanks,
Michal

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Stefano Garzarella]

On Sun, Jun 29, 2025 at 11:26:12PM +0200, Michal Luczaj wrote:
>On 6/27/25 10:02, Stefano Garzarella wrote:
>> On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>>> On 6/25/25 10:43, Stefano Garzarella wrote:
>>>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>>>
>>>>> Introduce vsock_transport_local_cid() to protect from a potential
>>>>> null-ptr-deref.
>>>>>
>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>>>> Call Trace:
>>>>> __vsock_bind+0x4b2/0x720
>>>>> vsock_bind+0x90/0xe0
>>>>> __sys_bind+0x14d/0x1e0
>>>>> __x64_sys_bind+0x6e/0xc0
>>>>> do_syscall_64+0x92/0x1c0
>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>
>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>>>> Call Trace:
>>>>> __x64_sys_ioctl+0x12d/0x190
>>>>> do_syscall_64+0x92/0x1c0
>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>
>>>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>>> ---
>>>>> net/vmw_vsock/af_vsock.c | 23 +++++++++++++++++------
>>>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>>> index 2e7a3034e965db30b6ee295370d866e6d8b1c341..63a920af5bfe6960306a3e5eeae0cbf30648985e 100644
>>>>> --- a/net/vmw_vsock/af_vsock.c
>>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>>> @@ -531,9 +531,21 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(vsock_assign_transport);
>>>>>
>>>>> +static u32 vsock_transport_local_cid(const struct vsock_transport **transport)
>>>>
>>>> Why we need double pointer?
>>>
>>> Because of a possible race. If @transport is `struct vsock_transport*` and
>>> we pass `transport_g2h`, the passed non-NULL pointer value may immediately
>>> become stale (due to module unload). But if it's `vsock_transport**` and we
>>> pass `&transport_g2h`, then we can take the mutex, check `*transport` for
>>> NULL and safely go ahead.
>>>
>>> Or are you saying this could be simplified?
>>
>> Nope, you're right! I was still thinking about my old version where we
>> had the switch inside...
>>
>> BTW I'd like to change the name, `vsock_transport_local` prefix is
>> confusing IMO, since it seems related only to the `transport_local`.
>>
>> Another thing I'm worried about is that we'll then start using it on
>> `vsk->transport` when this is only to be used on registered transports
>> (i.e. `static ...`), though, I don't think there's a way to force type
>> checking from the compiler (unless you wrap it in a struct). (...)
>
>I've found (on SO[1]) this somewhat hackish compile-time `static`-checking:
>
>static u32 __vsock_registered_transport_cid(const struct vsock_transport
>**transport)
>{
>	u32 cid = VMADDR_CID_ANY;
>
>	mutex_lock(&vsock_register_mutex);
>	if (*transport)
>		cid = (*transport)->get_local_cid();
>	mutex_unlock(&vsock_register_mutex);
>
>	return cid;
>}
>
>#define ASSERT_REGISTERED_TRANSPORT(t)					\
>	__always_unused static void *__UNIQUE_ID(vsock) = (t)
>
>#define vsock_registered_transport_cid(transport)			\
>({									\
>	ASSERT_REGISTERED_TRANSPORT(transport);				\
>	__vsock_registered_transport_cid(transport);			\
>})
>
>It does the trick, compilation fails on
>vsock_registered_transport_cid(&vsk->transport):
>
>net/vmw_vsock/af_vsock.c: In function ‘vsock_send_shutdown’:
>net/vmw_vsock/af_vsock.c:565:59: error: initializer element is not constant
>  565 |         __always_unused static void *__UNIQUE_ID(vsock) = (t)
>      |                                                           ^
>net/vmw_vsock/af_vsock.c:569:9: note: in expansion of macro
>‘ASSERT_REGISTERED_TRANSPORT’
>  569 |         ASSERT_REGISTERED_TRANSPORT(transport);
>    \
>      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>net/vmw_vsock/af_vsock.c:626:9: note: in expansion of macro
>‘vsock_registered_transport_cid’
>  626 |         vsock_registered_transport_cid(&vsk->transport);
>      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>But perhaps adding a comment wouldn't hurt either, e.g.
>
>/* Provide safe access to static transport_{h2g,g2h,dgram,local} callbacks.
> * Otherwise we may race with module removal. Do not use on
> * `vsk->transport`.
> */

Yeah, I'd just go with the comment, without introduce complex macros.
Also because in the worst case we don't do anything wrong.

BTW if we have some macros already defined in the kernel that we can 
re-use, it's fine.

>
>? ...which begs another question: do we stick to the netdev special comment
>style? See commit 82b8000c28b5 ("net: drop special comment style").

If checkpatch is fine, I'm fine :-)

>
>Oh, and come to think of it, we don't really need that (easily contended?)
>mutex here. Same can be done with RCU. Which should speed up vsock_bind()
>-> __vsock_bind() -> vsock_find_cid(), right? This is what I mean, roughly:
>
>+static u32 vsock_registered_transport_cid(const struct vsock_transport
>__rcu **trans_ptr)
>+{
>+	const struct vsock_transport *transport;
>+	u32 cid = VMADDR_CID_ANY;
>+
>+	rcu_read_lock();
>+	transport = rcu_dereference(*trans_ptr);
>+	if (transport)
>+		cid = transport->get_local_cid();
>+	rcu_read_unlock();
>+
>+	return cid;
>+}
>...
>@@ -2713,6 +2726,7 @@ void vsock_core_unregister(const struct
>vsock_transport *t)
> 		transport_local = NULL;
>
> 	mutex_unlock(&vsock_register_mutex);
>+	synchronize_rcu();
> }
>
>I've realized I'm throwing multiple unrelated ideas/questions, so let me
>summarise:
>1. Hackish macro can be used to guard against calling
>vsock_registered_transport_cid() on a non-static variable.
>2. We can comment the function to add some context and avoid confusion.

I'd go with 2.

>3. Instead of taking mutex in vsock_registered_transport_cid() we can use RCU.

Since the vsock_bind() is not in the hot path, maybe a mutex is fine.
WDYT?

Thanks,
Stefano

>
>> So, if we can't do much, I'd add a comment and make the function name
>> more clear. e.g. vsock_registered_transport_cid() ? or something
>> similar.
>
>Sure, will do.
>
>Thanks!
>
>[1]:
>https://stackoverflow.com/questions/5645695/how-can-i-add-a-static-assert-to-check-if-a-variable-is-static/5672637#5672637
>

Re: [PATCH RFC net v2 2/3] vsock: Fix transport_* TOCTOU

[Stefano Garzarella]

On Sun, Jun 29, 2025 at 11:26:25PM +0200, Michal Luczaj wrote:
>On 6/27/25 10:08, Stefano Garzarella wrote:
>> On Fri, Jun 20, 2025 at 09:52:44PM +0200, Michal Luczaj wrote:
>>> Transport assignment may race with module unload. Protect new_transport
>>>from becoming a stale pointer.
>>>
>>> This also takes care of an insecure call in vsock_use_local_transport();
>>> add a lockdep assert.
>>>
>>> BUG: unable to handle page fault for address: fffffbfff8056000
>>> Oops: Oops: 0000 [#1] SMP KASAN
>>> RIP: 0010:vsock_assign_transport+0x366/0x600
>>> Call Trace:
>>> vsock_connect+0x59c/0xc40
>>> __sys_connect+0xe8/0x100
>>> __x64_sys_connect+0x6e/0xc0
>>> do_syscall_64+0x92/0x1c0
>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>
>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> net/vmw_vsock/af_vsock.c | 28 +++++++++++++++++++++++-----
>>> 1 file changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 63a920af5bfe6960306a3e5eeae0cbf30648985e..a1b1073a2c89f865fcdb58b38d8e7feffcf1544f 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -407,6 +407,8 @@ EXPORT_SYMBOL_GPL(vsock_enqueue_accept);
>>>
>>> static bool vsock_use_local_transport(unsigned int remote_cid)
>>> {
>>> +	lockdep_assert_held(&vsock_register_mutex);
>>> +
>>> 	if (!transport_local)
>>> 		return false;
>>>
>>> @@ -464,6 +466,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>>
>>> 	remote_flags = vsk->remote_addr.svm_flags;
>>>
>>> +	mutex_lock(&vsock_register_mutex);
>>> +
>>> 	switch (sk->sk_type) {
>>> 	case SOCK_DGRAM:
>>> 		new_transport = transport_dgram;
>>> @@ -479,12 +483,15 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>>> 			new_transport = transport_h2g;
>>> 		break;
>>> 	default:
>>> -		return -ESOCKTNOSUPPORT;
>>> +		ret = -ESOCKTNOSUPPORT;
>>> +		goto err;
>>> 	}
>>>
>>> 	if (vsk->transport) {
>>> -		if (vsk->transport == new_transport)
>>> -			return 0;
>>> +		if (vsk->transport == new_transport) {
>>> +			ret = 0;
>>> +			goto err;
>>> +		}
>>
>> 		/* transport->release() must be called with sock lock acquired.
>> 		 * This path can only be taken during vsock_connect(), where we
>> 		 * have already held the sock lock. In the other cases, this
>> 		 * function is called on a new socket which is not assigned to
>> 		 * any transport.
>> 		 */
>> 		vsk->transport->release(vsk);
>> 		vsock_deassign_transport(vsk);
>>
>> Thinking back to this patch, could there be a deadlock between call
>> vsock_deassign_transport(), which call module_put(), now with the
>> `vsock_register_mutex` held, and the call to vsock_core_unregister()
>> usually made by modules in the exit function?
>
>I think we're good. module_put() does not call the module cleanup function
>(kernel/module/main.c:delete_module() syscall does that), so
>vsock_core_unregister() won't happen in this path here. Have I missed
>anything else?
>

Nope, I reached the same conclusion!

Thanks,
Stefano

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj]

On 6/30/25 11:05, Stefano Garzarella wrote:
> On Sun, Jun 29, 2025 at 11:26:12PM +0200, Michal Luczaj wrote:
>> On 6/27/25 10:02, Stefano Garzarella wrote:
>>> On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>>>> On 6/25/25 10:43, Stefano Garzarella wrote:
>>>>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>>>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>>>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>>>>
>>>>>> Introduce vsock_transport_local_cid() to protect from a potential
>>>>>> null-ptr-deref.
>>>>>>
>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>>>>> Call Trace:
>>>>>> __vsock_bind+0x4b2/0x720
>>>>>> vsock_bind+0x90/0xe0
>>>>>> __sys_bind+0x14d/0x1e0
>>>>>> __x64_sys_bind+0x6e/0xc0
>>>>>> do_syscall_64+0x92/0x1c0
>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>
>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>>>>> Call Trace:
>>>>>> __x64_sys_ioctl+0x12d/0x190
>>>>>> do_syscall_64+0x92/0x1c0
>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>
>>>>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
...
>> Oh, and come to think of it, we don't really need that (easily contended?)
>> mutex here. Same can be done with RCU. Which should speed up vsock_bind()
>> -> __vsock_bind() -> vsock_find_cid(), right? This is what I mean, roughly:
>>
>> +static u32 vsock_registered_transport_cid(const struct vsock_transport
>> __rcu **trans_ptr)
>> +{
>> +	const struct vsock_transport *transport;
>> +	u32 cid = VMADDR_CID_ANY;
>> +
>> +	rcu_read_lock();
>> +	transport = rcu_dereference(*trans_ptr);
>> +	if (transport)
>> +		cid = transport->get_local_cid();
>> +	rcu_read_unlock();
>> +
>> +	return cid;
>> +}
>> ...
>> @@ -2713,6 +2726,7 @@ void vsock_core_unregister(const struct
>> vsock_transport *t)
>> 		transport_local = NULL;
>>
>> 	mutex_unlock(&vsock_register_mutex);
>> +	synchronize_rcu();
>> }
>>
>> I've realized I'm throwing multiple unrelated ideas/questions, so let me
>> summarise:
>> 1. Hackish macro can be used to guard against calling
>> vsock_registered_transport_cid() on a non-static variable.
>> 2. We can comment the function to add some context and avoid confusion.
> 
> I'd go with 2.

All right, will do.

>> 3. Instead of taking mutex in vsock_registered_transport_cid() we can use RCU.
> 
> Since the vsock_bind() is not in the hot path, maybe a mutex is fine.
> WDYT?

I wrote a benchmark that attempts (and fails due to a non-existing CID) to
bind() 100s of vsocks in multiple threads. `perf lock con` shows that this
mutex is contended, and things are slowed down by 100+% compared with RCU
approach. Which makes sense: every explicit vsock bind() across the whole
system would need to acquire the mutex. And now we're also taking the same
mutex in vsock_assign_transport(), i.e. during connect(). But maybe such
stress testing is just unrealistic, I really don't know.

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Stefano Garzarella]

On Mon, Jun 30, 2025 at 01:02:26PM +0200, Michal Luczaj wrote:
>On 6/30/25 11:05, Stefano Garzarella wrote:
>> On Sun, Jun 29, 2025 at 11:26:12PM +0200, Michal Luczaj wrote:
>>> On 6/27/25 10:02, Stefano Garzarella wrote:
>>>> On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>>>>> On 6/25/25 10:43, Stefano Garzarella wrote:
>>>>>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>>>>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>>>>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>>>>>
>>>>>>> Introduce vsock_transport_local_cid() to protect from a potential
>>>>>>> null-ptr-deref.
>>>>>>>
>>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>>>>>> Call Trace:
>>>>>>> __vsock_bind+0x4b2/0x720
>>>>>>> vsock_bind+0x90/0xe0
>>>>>>> __sys_bind+0x14d/0x1e0
>>>>>>> __x64_sys_bind+0x6e/0xc0
>>>>>>> do_syscall_64+0x92/0x1c0
>>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>>
>>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>>>>>> Call Trace:
>>>>>>> __x64_sys_ioctl+0x12d/0x190
>>>>>>> do_syscall_64+0x92/0x1c0
>>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>>
>>>>>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>...
>>> Oh, and come to think of it, we don't really need that (easily contended?)
>>> mutex here. Same can be done with RCU. Which should speed up vsock_bind()
>>> -> __vsock_bind() -> vsock_find_cid(), right? This is what I mean, roughly:
>>>
>>> +static u32 vsock_registered_transport_cid(const struct vsock_transport
>>> __rcu **trans_ptr)
>>> +{
>>> +	const struct vsock_transport *transport;
>>> +	u32 cid = VMADDR_CID_ANY;
>>> +
>>> +	rcu_read_lock();
>>> +	transport = rcu_dereference(*trans_ptr);
>>> +	if (transport)
>>> +		cid = transport->get_local_cid();
>>> +	rcu_read_unlock();
>>> +
>>> +	return cid;
>>> +}
>>> ...
>>> @@ -2713,6 +2726,7 @@ void vsock_core_unregister(const struct
>>> vsock_transport *t)
>>> 		transport_local = NULL;
>>>
>>> 	mutex_unlock(&vsock_register_mutex);
>>> +	synchronize_rcu();
>>> }
>>>
>>> I've realized I'm throwing multiple unrelated ideas/questions, so let me
>>> summarise:
>>> 1. Hackish macro can be used to guard against calling
>>> vsock_registered_transport_cid() on a non-static variable.
>>> 2. We can comment the function to add some context and avoid confusion.
>>
>> I'd go with 2.
>
>All right, will do.
>
>>> 3. Instead of taking mutex in vsock_registered_transport_cid() we can use RCU.
>>
>> Since the vsock_bind() is not in the hot path, maybe a mutex is fine.
>> WDYT?
>
>I wrote a benchmark that attempts (and fails due to a non-existing CID) to
>bind() 100s of vsocks in multiple threads. `perf lock con` shows that this
>mutex is contended, and things are slowed down by 100+% compared with RCU
>approach. Which makes sense: every explicit vsock bind() across the whole
>system would need to acquire the mutex. And now we're also taking the same
>mutex in vsock_assign_transport(), i.e. during connect(). But maybe such
>stress testing is just unrealistic, I really don't know.
>

I still don't think it's a hot path to optimize, but I'm not totally 
against it. If you want to do it though, I would say do it in a separate 
patch.

Thanks,
Stefano

Re: [PATCH RFC net v2 1/3] vsock: Fix transport_{g2h,h2g} TOCTOU

[Michal Luczaj]

On 7/1/25 12:34, Stefano Garzarella wrote:
> On Mon, Jun 30, 2025 at 01:02:26PM +0200, Michal Luczaj wrote:
>> On 6/30/25 11:05, Stefano Garzarella wrote:
>>> On Sun, Jun 29, 2025 at 11:26:12PM +0200, Michal Luczaj wrote:
>>>> On 6/27/25 10:02, Stefano Garzarella wrote:
>>>>> On Wed, Jun 25, 2025 at 11:23:30PM +0200, Michal Luczaj wrote:
>>>>>> On 6/25/25 10:43, Stefano Garzarella wrote:
>>>>>>> On Fri, Jun 20, 2025 at 09:52:43PM +0200, Michal Luczaj wrote:
>>>>>>>> vsock_find_cid() and vsock_dev_do_ioctl() may race with module unload.
>>>>>>>> transport_{g2h,h2g} may become NULL after the NULL check.
>>>>>>>>
>>>>>>>> Introduce vsock_transport_local_cid() to protect from a potential
>>>>>>>> null-ptr-deref.
>>>>>>>>
>>>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>>>> RIP: 0010:vsock_find_cid+0x47/0x90
>>>>>>>> Call Trace:
>>>>>>>> __vsock_bind+0x4b2/0x720
>>>>>>>> vsock_bind+0x90/0xe0
>>>>>>>> __sys_bind+0x14d/0x1e0
>>>>>>>> __x64_sys_bind+0x6e/0xc0
>>>>>>>> do_syscall_64+0x92/0x1c0
>>>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>>>
>>>>>>>> KASAN: null-ptr-deref in range [0x0000000000000118-0x000000000000011f]
>>>>>>>> RIP: 0010:vsock_dev_do_ioctl.isra.0+0x58/0xf0
>>>>>>>> Call Trace:
>>>>>>>> __x64_sys_ioctl+0x12d/0x190
>>>>>>>> do_syscall_64+0x92/0x1c0
>>>>>>>> entry_SYSCALL_64_after_hwframe+0x4b/0x53
>>>>>>>>
>>>>>>>> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>>>>>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ...
>>>> Oh, and come to think of it, we don't really need that (easily contended?)
>>>> mutex here. Same can be done with RCU. Which should speed up vsock_bind()
>>>> -> __vsock_bind() -> vsock_find_cid(), right? This is what I mean, roughly:
>>>>
>>>> +static u32 vsock_registered_transport_cid(const struct vsock_transport
>>>> __rcu **trans_ptr)
>>>> +{
>>>> +	const struct vsock_transport *transport;
>>>> +	u32 cid = VMADDR_CID_ANY;
>>>> +
>>>> +	rcu_read_lock();
>>>> +	transport = rcu_dereference(*trans_ptr);
>>>> +	if (transport)
>>>> +		cid = transport->get_local_cid();
>>>> +	rcu_read_unlock();
>>>> +
>>>> +	return cid;
>>>> +}
>>>> ...
>>>> @@ -2713,6 +2726,7 @@ void vsock_core_unregister(const struct
>>>> vsock_transport *t)
>>>> 		transport_local = NULL;
>>>>
>>>> 	mutex_unlock(&vsock_register_mutex);
>>>> +	synchronize_rcu();
>>>> }
>>>>
>>>> I've realized I'm throwing multiple unrelated ideas/questions, so let me
>>>> summarise:
>>>> 1. Hackish macro can be used to guard against calling
>>>> vsock_registered_transport_cid() on a non-static variable.
>>>> 2. We can comment the function to add some context and avoid confusion.
>>>
>>> I'd go with 2.
>>
>> All right, will do.
>>
>>>> 3. Instead of taking mutex in vsock_registered_transport_cid() we can use RCU.
>>>
>>> Since the vsock_bind() is not in the hot path, maybe a mutex is fine.
>>> WDYT?
>>
>> I wrote a benchmark that attempts (and fails due to a non-existing CID) to
>> bind() 100s of vsocks in multiple threads. `perf lock con` shows that this
>> mutex is contended, and things are slowed down by 100+% compared with RCU
>> approach. Which makes sense: every explicit vsock bind() across the whole
>> system would need to acquire the mutex. And now we're also taking the same
>> mutex in vsock_assign_transport(), i.e. during connect(). But maybe such
>> stress testing is just unrealistic, I really don't know.
>>
> 
> I still don't think it's a hot path to optimize, but I'm not totally 
> against it. If you want to do it though, I would say do it in a separate 
> patch.

All right, so here's v3:
https://lore.kernel.org/netdev/20250702-vsock-transports-toctou-v3-0-0a7e2e692987@rbox.co/

Thanks,
Michal