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

login
register
mail settings
Submitter Asias He
Date June 20, 2013, 9:20 a.m.
Message ID <1371720033-19714-5-git-send-email-asias@redhat.com>
Download mbox | patch
Permalink /patch/252831/
State Accepted
Delegated to: David Miller
Headers show

Comments

Asias He - June 20, 2013, 9:20 a.m.
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(-)
Andy King - June 20, 2013, 3:12 p.m.
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Asias He - June 21, 2013, 12:22 a.m.
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
Andy King - June 21, 2013, 12:48 p.m.
> [0-250] is for bound sockets, [251] is for unbound sockets, no?

Well caught.

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

Thanks!
- Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)				\