[PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY

[PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY

[Lepton Wu]

The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
when VMM crashed, there is still orphaned vsock which is waiting for
close timer, then it could cause connection time out for new started VM
if they are trying to connect to same port with same guest cid since the
new packets could hit that orphaned vsock. We could also fix this by doing
more in vhost_vsock_reset_orphans, but any way, it should be better to start
from a random local port instead of a fixed one.

Signed-off-by: Lepton Wu <ytht.net@gmail.com>
---
 net/vmw_vsock/af_vsock.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index ab27a2872935..73817e846a1f 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -107,6 +107,7 @@
 #include <linux/mutex.h>
 #include <linux/net.h>
 #include <linux/poll.h>
+#include <linux/random.h>
 #include <linux/skbuff.h>
 #include <linux/smp.h>
 #include <linux/socket.h>
@@ -504,9 +505,12 @@ static void vsock_pending_work(struct work_struct *work)
 static int __vsock_bind_stream(struct vsock_sock *vsk,
 			       struct sockaddr_vm *addr)
 {
-	static u32 port = LAST_RESERVED_PORT + 1;
+	static u32 port = 0;
 	struct sockaddr_vm new_addr;
 
+	if (!port)
+		port = prandom_u32();
+
 	vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
 
 	if (addr->svm_port == VMADDR_PORT_ANY) {
-- 
2.20.0.rc2.403.gdbc3b29805-goog

Re: [PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY

[Stefan Hajnoczi]

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

On Mon, Dec 10, 2018 at 11:02:35PM -0800, Lepton Wu wrote:
> The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
> when VMM crashed, there is still orphaned vsock which is waiting for
> close timer, then it could cause connection time out for new started VM
> if they are trying to connect to same port with same guest cid since the
> new packets could hit that orphaned vsock. We could also fix this by doing
> more in vhost_vsock_reset_orphans, but any way, it should be better to start
> from a random local port instead of a fixed one.
> 
> Signed-off-by: Lepton Wu <ytht.net@gmail.com>
> ---
>  net/vmw_vsock/af_vsock.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Jorgen, Dexuan: Any objection to this?  It also affects the other
AF_VSOCK transports.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index ab27a2872935..73817e846a1f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -107,6 +107,7 @@
>  #include <linux/mutex.h>
>  #include <linux/net.h>
>  #include <linux/poll.h>
> +#include <linux/random.h>
>  #include <linux/skbuff.h>
>  #include <linux/smp.h>
>  #include <linux/socket.h>
> @@ -504,9 +505,12 @@ static void vsock_pending_work(struct work_struct *work)
>  static int __vsock_bind_stream(struct vsock_sock *vsk,
>  			       struct sockaddr_vm *addr)
>  {
> -	static u32 port = LAST_RESERVED_PORT + 1;
> +	static u32 port = 0;
>  	struct sockaddr_vm new_addr;
>  
> +	if (!port)
> +		port = prandom_u32();
> +
>  	vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
>  
>  	if (addr->svm_port == VMADDR_PORT_ANY) {
> -- 
> 2.20.0.rc2.403.gdbc3b29805-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY

[Jorgen S. Hansen]

> On Mon, Dec 10, 2018 at 11:02:35PM -0800, Lepton Wu wrote:
> > The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
> > when VMM crashed, there is still orphaned vsock which is waiting for
> > close timer, then it could cause connection time out for new started VM
> > if they are trying to connect to same port with same guest cid since the
> > new packets could hit that orphaned vsock. We could also fix this by doing
> > more in vhost_vsock_reset_orphans, but any way, it should be better to start
> > from a random local port instead of a fixed one.
> >
> > Signed-off-by: Lepton Wu <ytht.net@gmail.com>
> > ---
> >  net/vmw_vsock/af_vsock.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Jorgen, Dexuan: Any objection to this?  It also affects the other
> AF_VSOCK transports.

Makes sense to me.

> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index ab27a2872935..73817e846a1f 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -107,6 +107,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/net.h>
> >  #include <linux/poll.h>
> > +#include <linux/random.h>
> >  #include <linux/skbuff.h>
> >  #include <linux/smp.h>
> >  #include <linux/socket.h>
> > @@ -504,9 +505,12 @@ static void vsock_pending_work(struct work_struct *work)
> >  static int __vsock_bind_stream(struct vsock_sock *vsk,
> >                              struct sockaddr_vm *addr)
> >  {
> > -     static u32 port = LAST_RESERVED_PORT + 1;
> > +     static u32 port = 0;
> >       struct sockaddr_vm new_addr;
> >
> > +     if (!port)
> > +             port = prandom_u32();
> > +

How about making this:
   port = LAST_RESERVED_PORT + 1 + prandom_u32_max(U32_MAX - LAST_RESERVED_PORT);
so the initial assignment is a valid port in the unreserved range. It will be corrected in the first iteration,
but this would make the intention clearer.

> >       vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
> >
> >       if (addr->svm_port == VMADDR_PORT_ANY) {
> > --
> > 2.20.0.rc2.403.gdbc3b29805-goog
>>

Thanks,
Jorgen

[PATCH v2] VSOCK: bind to random port for VMADDR_PORT_ANY

[Lepton Wu]

The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
when VMM crashed, there is still orphaned vsock which is waiting for
close timer, then it could cause connection time out for new started VM
if they are trying to connect to same port with same guest cid since the
new packets could hit that orphaned vsock. We could also fix this by doing
more in vhost_vsock_reset_orphans, but any way, it should be better to start
from a random local port instead of a fixed one.

Signed-off-by: Lepton Wu <ytht.net@gmail.com>
---
 net/vmw_vsock/af_vsock.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index ab27a2872935..43a1dec08825 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -107,6 +107,7 @@
 #include <linux/mutex.h>
 #include <linux/net.h>
 #include <linux/poll.h>
+#include <linux/random.h>
 #include <linux/skbuff.h>
 #include <linux/smp.h>
 #include <linux/socket.h>
@@ -504,9 +505,13 @@ static void vsock_pending_work(struct work_struct *work)
 static int __vsock_bind_stream(struct vsock_sock *vsk,
 			       struct sockaddr_vm *addr)
 {
-	static u32 port = LAST_RESERVED_PORT + 1;
+	static u32 port = 0;
 	struct sockaddr_vm new_addr;
 
+	if (!port)
+		port = LAST_RESERVED_PORT + 1 +
+			prandom_u32_max(U32_MAX - LAST_RESERVED_PORT);
+
 	vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
 
 	if (addr->svm_port == VMADDR_PORT_ANY) {
-- 
2.20.0.405.gbc1bbc6f85-goog

RE: [PATCH v2] VSOCK: bind to random port for VMADDR_PORT_ANY

[Jorgen S. Hansen]

> 
> The old code always starts from fixed port for VMADDR_PORT_ANY.
> Sometimes when VMM crashed, there is still orphaned vsock which is waiting
> for close timer, then it could cause connection time out for new started VM if
> they are trying to connect to same port with same guest cid since the new
> packets could hit that orphaned vsock. We could also fix this by doing more in
> vhost_vsock_reset_orphans, but any way, it should be better to start from a
> random local port instead of a fixed one.
> 
> Signed-off-by: Lepton Wu <ytht.net@gmail.com>
> ---
>  net/vmw_vsock/af_vsock.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> ab27a2872935..43a1dec08825 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -107,6 +107,7 @@
>  #include <linux/mutex.h>
>  #include <linux/net.h>
>  #include <linux/poll.h>
> +#include <linux/random.h>
>  #include <linux/skbuff.h>
>  #include <linux/smp.h>
>  #include <linux/socket.h>
> @@ -504,9 +505,13 @@ static void vsock_pending_work(struct work_struct
> *work)  static int __vsock_bind_stream(struct vsock_sock *vsk,
>  			       struct sockaddr_vm *addr)
>  {
> -	static u32 port = LAST_RESERVED_PORT + 1;
> +	static u32 port = 0;
>  	struct sockaddr_vm new_addr;
> 
> +	if (!port)
> +		port = LAST_RESERVED_PORT + 1 +
> +			prandom_u32_max(U32_MAX -
> LAST_RESERVED_PORT);
> +
>  	vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
> 
>  	if (addr->svm_port == VMADDR_PORT_ANY) {
> --
> 2.20.0.405.gbc1bbc6f85-goog

Thanks for the update - looks good to me.

Reviewed-by: Jorgen Hansen <jhansen@vmware.com>

/jsh

Re: [PATCH v2] VSOCK: bind to random port for VMADDR_PORT_ANY

[David Miller]

From: Lepton Wu <ytht.net@gmail.com>
Date: Tue, 11 Dec 2018 11:12:55 -0800

> The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
> when VMM crashed, there is still orphaned vsock which is waiting for
> close timer, then it could cause connection time out for new started VM
> if they are trying to connect to same port with same guest cid since the
> new packets could hit that orphaned vsock. We could also fix this by doing
> more in vhost_vsock_reset_orphans, but any way, it should be better to start
> from a random local port instead of a fixed one.
> 
> Signed-off-by: Lepton Wu <ytht.net@gmail.com>

Applied, thanks.

RE: [PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY

[Dexuan Cui]

> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: Tuesday, December 11, 2018 8:27 AM
> To: Jorgen Hansen <jhansen@vmware.com>; Dexuan Cui
> <decui@microsoft.com>
> Cc: netdev@vger.kernel.org; Lepton Wu <ytht.net@gmail.com>
> Subject: Re: [PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY
> 
> On Mon, Dec 10, 2018 at 11:02:35PM -0800, Lepton Wu wrote:
> > The old code always starts from fixed port for VMADDR_PORT_ANY.
> Sometimes
> > when VMM crashed, there is still orphaned vsock which is waiting for
> > close timer, then it could cause connection time out for new started VM
> > if they are trying to connect to same port with same guest cid since the
> > new packets could hit that orphaned vsock. We could also fix this by doing
> > more in vhost_vsock_reset_orphans, but any way, it should be better to start
> > from a random local port instead of a fixed one.
> >
> > Signed-off-by: Lepton Wu <ytht.net@gmail.com>
> > ---
> >  net/vmw_vsock/af_vsock.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Jorgen, Dexuan: Any objection to this?  It also affects the other
> AF_VSOCK transports.

Thanks for letting me notice the patch. :-)

Now I saw Lepton's v2 patch, which looks good to me. 

Thanks,
-- Dexuan