diff mbox series

[ovs-dev,v3,1/8] conntrack: Init hash basis first at creation

Message ID 308d3ef9f3fbb8c41f52dd1150cf549c5890287f.1623786081.git.grive@u256.net
State Accepted
Headers show
Series conntrack: improve multithread scalability | expand

Commit Message

Gaƫtan Rivet June 15, 2021, 11:22 p.m. UTC
The 'hash_basis' field is used sometimes during sub-systems init
routine. It will be 0 by default before randomization. Sub-systems would
then init some nodes with incorrect hash values.

The timeout policies module is affected, making the default policy being
referenced using an incorrect hash value.

Fixes: 2078901a4c14 ("userspace: Add conntrack timeout policy support.")
Signed-off-by: Gaetan Rivet <grive@u256.net>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
Acked-by: William Tu <u9012063@gmail.com>
---
 lib/conntrack.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Ilya Maximets July 9, 2021, 8:57 p.m. UTC | #1
On 6/16/21 1:22 AM, Gaetan Rivet wrote:
> The 'hash_basis' field is used sometimes during sub-systems init
> routine. It will be 0 by default before randomization. Sub-systems would
> then init some nodes with incorrect hash values.
> 
> The timeout policies module is affected, making the default policy being
> referenced using an incorrect hash value.
> 
> Fixes: 2078901a4c14 ("userspace: Add conntrack timeout policy support.")
> Signed-off-by: Gaetan Rivet <grive@u256.net>
> Reviewed-by: Eli Britstein <elibr@nvidia.com>
> Acked-by: William Tu <u9012063@gmail.com>
> ---

Hi, Gaetan.  Thanks for the patch!

This seems to be an independent bug fix, so I applied it to master.
Also backported down to 2.14.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 99198a601..a5efb37aa 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -291,6 +291,11 @@  conntrack_init(void)
     static struct ovsthread_once setup_l4_once = OVSTHREAD_ONCE_INITIALIZER;
     struct conntrack *ct = xzalloc(sizeof *ct);
 
+    /* This value can be used during init (e.g. timeout_policy_init()),
+     * set it first to ensure it is available.
+     */
+    ct->hash_basis = random_uint32();
+
     ovs_rwlock_init(&ct->resources_lock);
     ovs_rwlock_wrlock(&ct->resources_lock);
     hmap_init(&ct->alg_expectations);
@@ -308,7 +313,6 @@  conntrack_init(void)
     timeout_policy_init(ct);
     ovs_mutex_unlock(&ct->ct_lock);
 
-    ct->hash_basis = random_uint32();
     atomic_count_init(&ct->n_conn, 0);
     atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);
     atomic_init(&ct->tcp_seq_chk, true);