[PATCH 0/4] VSOCK fix and cleanup

[PATCH 0/4] VSOCK fix and cleanup

[Asias He]

Asias He (4):
  VSOCK: Introduce vsock_auto_bind helper
  VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb
  VSOCK: Remove unnecessary label
  VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

 net/vmw_vsock/af_vsock.c       | 55 ++++++++++++++++++------------------------
 net/vmw_vsock/vmci_transport.c | 18 +++++++-------
 2 files changed, 33 insertions(+), 40 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] VSOCK: Introduce vsock_auto_bind helper

[Asias He]

This peace of code is called tree times, let's have a helper for it.

Signed-off-by: Asias He <asias@redhat.com>
---
 net/vmw_vsock/af_vsock.c | 49 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 3f77f42..b0b362a 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -165,6 +165,18 @@ static struct list_head vsock_bind_table[VSOCK_HASH_SIZE + 1];
 static struct list_head vsock_connected_table[VSOCK_HASH_SIZE];
 static DEFINE_SPINLOCK(vsock_table_lock);
 
+/* Autobind this socket to the local address if necessary. */
+static int vsock_auto_bind(struct vsock_sock *vsk)
+{
+	struct sock *sk = sk_vsock(vsk);
+	struct sockaddr_vm local_addr;
+
+	if (vsock_addr_bound(&vsk->local_addr))
+		return 0;
+	vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
+	return __vsock_bind(sk, &local_addr);
+}
+
 static void vsock_init_tables(void)
 {
 	int i;
@@ -956,15 +968,10 @@ static int vsock_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
 
 	lock_sock(sk);
 
-	if (!vsock_addr_bound(&vsk->local_addr)) {
-		struct sockaddr_vm local_addr;
-
-		vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
-		err = __vsock_bind(sk, &local_addr);
-		if (err != 0)
-			goto out;
+	err = vsock_auto_bind(vsk);
+	if (err)
+		goto out;
 
-	}
 
 	/* If the provided message contains an address, use that.  Otherwise
 	 * fall back on the socket's remote handle (if it has been connected).
@@ -1038,15 +1045,9 @@ static int vsock_dgram_connect(struct socket *sock,
 
 	lock_sock(sk);
 
-	if (!vsock_addr_bound(&vsk->local_addr)) {
-		struct sockaddr_vm local_addr;
-
-		vsock_addr_init(&local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
-		err = __vsock_bind(sk, &local_addr);
-		if (err != 0)
-			goto out;
-
-	}
+	err = vsock_auto_bind(vsk);
+	if (err)
+		goto out;
 
 	if (!transport->dgram_allow(remote_addr->svm_cid,
 				    remote_addr->svm_port)) {
@@ -1163,17 +1164,9 @@ static int vsock_stream_connect(struct socket *sock, struct sockaddr *addr,
 		memcpy(&vsk->remote_addr, remote_addr,
 		       sizeof(vsk->remote_addr));
 
-		/* Autobind this socket to the local address if necessary. */
-		if (!vsock_addr_bound(&vsk->local_addr)) {
-			struct sockaddr_vm local_addr;
-
-			vsock_addr_init(&local_addr, VMADDR_CID_ANY,
-					VMADDR_PORT_ANY);
-			err = __vsock_bind(sk, &local_addr);
-			if (err != 0)
-				goto out;
-
-		}
+		err = vsock_auto_bind(vsk);
+		if (err)
+			goto out;
 
 		sk->sk_state = SS_CONNECTING;
 
-- 
1.8.1.4

[PATCH 2/4] VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb

[Asias He]

vmci_transport_recv_dgram_cb always return VMCI_SUCESS even if we fail
to allocate skb, return VMCI_ERROR_NO_MEM instead.

Signed-off-by: Asias He <asias@redhat.com>
---
 net/vmw_vsock/vmci_transport.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index daff752..99b511d 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -625,13 +625,14 @@ static int vmci_transport_recv_dgram_cb(void *data, struct vmci_datagram *dg)
 
 	/* Attach the packet to the socket's receive queue as an sk_buff. */
 	skb = alloc_skb(size, GFP_ATOMIC);
-	if (skb) {
-		/* sk_receive_skb() will do a sock_put(), so hold here. */
-		sock_hold(sk);
-		skb_put(skb, size);
-		memcpy(skb->data, dg, size);
-		sk_receive_skb(sk, skb, 0);
-	}
+	if (!skb)
+		return VMCI_ERROR_NO_MEM;
+
+	/* sk_receive_skb() will do a sock_put(), so hold here. */
+	sock_hold(sk);
+	skb_put(skb, size);
+	memcpy(skb->data, dg, size);
+	sk_receive_skb(sk, skb, 0);
 
 	return VMCI_SUCCESS;
 }
-- 
1.8.1.4

[PATCH 3/4] VSOCK: Remove unnecessary label

[Asias He]

Signed-off-by: Asias He <asias@redhat.com>
---
 net/vmw_vsock/vmci_transport.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 99b511d..ffc11df 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -940,10 +940,9 @@ static void vmci_transport_recv_pkt_work(struct work_struct *work)
 		 * reset to prevent that.
 		 */
 		vmci_transport_send_reset(sk, pkt);
-		goto out;
+		break;
 	}
 
-out:
 	release_sock(sk);
 	kfree(recv_pkt_info);
 	/* Release reference obtained in the stream callback when we fetched
-- 
1.8.1.4

[PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

[Asias He]

If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249.  Actually, we
have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
In this case the last entry will never be used.

We should mod with VSOCK_HASH_SIZE instead.

Signed-off-by: Asias He <asias@redhat.com>
---
 net/vmw_vsock/af_vsock.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index b0b362a..593071d 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -144,18 +144,18 @@ EXPORT_SYMBOL_GPL(vm_sockets_get_local_cid);
  * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
  * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
  * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets.  The hash function
- * mods with VSOCK_HASH_SIZE - 1 to ensure this.
+ * mods with VSOCK_HASH_SIZE to ensure this.
  */
 #define VSOCK_HASH_SIZE         251
 #define MAX_PORT_RETRIES        24
 
-#define VSOCK_HASH(addr)        ((addr)->svm_port % (VSOCK_HASH_SIZE - 1))
+#define VSOCK_HASH(addr)        ((addr)->svm_port % VSOCK_HASH_SIZE)
 #define vsock_bound_sockets(addr) (&vsock_bind_table[VSOCK_HASH(addr)])
 #define vsock_unbound_sockets     (&vsock_bind_table[VSOCK_HASH_SIZE])
 
 /* XXX This can probably be implemented in a better way. */
 #define VSOCK_CONN_HASH(src, dst)				\
-	(((src)->svm_cid ^ (dst)->svm_port) % (VSOCK_HASH_SIZE - 1))
+	(((src)->svm_cid ^ (dst)->svm_port) % VSOCK_HASH_SIZE)
 #define vsock_connected_sockets(src, dst)		\
 	(&vsock_connected_table[VSOCK_CONN_HASH(src, dst)])
 #define vsock_connected_sockets_vsk(vsk)				\
-- 
1.8.1.4

Re: [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

[Andy King]

> If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249.  Actually, we
> have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
> In this case the last entry will never be used.

If I remember correctly, we did this on purpose.  There's actually a
comment about it:

>   * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
>   * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
>   * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets.  The hash

[250] is for unbound sockets.  If you hash on that, you'll mistakenly
get an unbound socket when looking for a bound one.

It is confusing, so perhaps a better way is just to move unbound into
its own table.

Thanks!
- Andy

Re: [PATCH 3/4] VSOCK: Remove unnecessary label

[Andy King]

> Signed-off-by: Asias He <asias@redhat.com>

Acked-by: Andy King <acking@vmware.com>

Thanks for fixing this!
- Andy

Re: [PATCH 2/4] VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb

[Andy King]

> vmci_transport_recv_dgram_cb always return VMCI_SUCESS even if we fail
> to allocate skb, return VMCI_ERROR_NO_MEM instead.
> 
> Signed-off-by: Asias He <asias@redhat.com>

Acked-by: Andy King <acking@vmware.com>

Thanks!
- Andy

Re: [PATCH 1/4] VSOCK: Introduce vsock_auto_bind helper

[Andy King]

> This peace of code is called tree times, let's have a helper for it.
> 
> Signed-off-by: Asias He <asias@redhat.com>

Acked-by: Andy King <acking@vmware.com>

Great, thanks!
- Andy

Re: [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

[Asias He]

On Thu, Jun 20, 2013 at 08:12:13AM -0700, Andy King wrote:
> > If we mod with VSOCK_HASH_SIZE -1, we get 0, 1, .... 249.  Actually, we
> > have vsock_bind_table[0 ... 250] and vsock_connected_table[0 .. 250].
> > In this case the last entry will never be used.
> 
> If I remember correctly, we did this on purpose.  There's actually a
> comment about it:
> 
> >   * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
> >   * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
> >   * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets.  The hash
> 
> [250] is for unbound sockets.  If you hash on that, you'll mistakenly
> get an unbound socket when looking for a bound one.

We have 

   #define VSOCK_HASH_SIZE         251
   static struct list_head vsock_bind_table[VSOCK_HASH_SIZE + 1];
   #define vsock_bound_sockets(addr) (&vsock_bind_table[VSOCK_HASH(addr)])
   #define vsock_unbound_sockets     (&vsock_bind_table[VSOCK_HASH_SIZE])

So

vsock_bind_table[251 + 1]

[0-250] is for bound sockets, [251] is for unbound sockets, no?

> It is confusing, so perhaps a better way is just to move unbound into
> its own table.

This isn't that confusing, but it would be clearer to have a own unbound table.

> Thanks!
> - Andy

-- 
Asias

Re: [PATCH 4/4] VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

[Andy King]

> [0-250] is for bound sockets, [251] is for unbound sockets, no?

Well caught.

Acked-by: Andy King <acking@vmware.com>

Thanks!
- Andy

Re: [PATCH 0/4] VSOCK fix and cleanup

[David Miller]

From: Asias He <asias@redhat.com>
Date: Thu, 20 Jun 2013 17:20:29 +0800

> 
> Asias He (4):
>   VSOCK: Introduce vsock_auto_bind helper
>   VSOCK: Return VMCI_ERROR_NO_MEM when fails to allocate skb
>   VSOCK: Remove unnecessary label
>   VSOCK: Fix VSOCK_HASH and VSOCK_CONN_HASH

Series applied to net-next, thanks.