Message ID | 1659412867-105216-1-git-send-email-wenxu@chinatelecom.cn |
---|---|
State | Deferred |
Headers | show |
Series | [ovs-dev,v3] conntrack: narrow the scope of ct_lock in new conn setup | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
On 8/2/22 06:01, wenxu@chinatelecom.cn wrote: > From: wenxu <wenxu@chinatelecom.cn> > > There is a big scope ct_lock for new conn setup. The > ct_lock should be hold only for conns map insert and > expire rculist insert. > > Signed-off-by: wenxu <wenxu@chinatelecom.cn> > --- > lib/conntrack.c | 32 ++++++++++++++++++++------------ > 1 file changed, 20 insertions(+), 12 deletions(-) Paolo, Aaron, could you, please, take a look at this one? Thanks! Best regards, Ilya Maximets. > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 13c5ab6..47d9aac 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -48,6 +48,7 @@ COVERAGE_DEFINE(conntrack_full); > COVERAGE_DEFINE(conntrack_l3csum_err); > COVERAGE_DEFINE(conntrack_l4csum_err); > COVERAGE_DEFINE(conntrack_lookup_natted_miss); > +COVERAGE_DEFINE(conntrack_duplicate); > > struct conn_lookup_ctx { > struct conn_key key; > @@ -1010,10 +1011,10 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, > const struct nat_action_info_t *nat_action_info, > const char *helper, const struct alg_exp_node *alg_exp, > enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id) > - OVS_REQUIRES(ct->ct_lock) > { > struct conn *nc = NULL; > struct conn *nat_conn = NULL; > + uint32_t nat_hash; > > if (!valid_new(pkt, &ctx->key)) { > pkt->md.ct_state = CS_INVALID; > @@ -1089,16 +1090,26 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, > nat_conn->nat_action = 0; > nat_conn->alg = NULL; > nat_conn->nat_conn = NULL; > - uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); > - cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); > + nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); > } > > nc->nat_conn = nat_conn; > ovs_mutex_init_adaptive(&nc->lock); > nc->conn_type = CT_CONN_TYPE_DEFAULT; > atomic_flag_clear(&nc->reclaimed); > + ovs_mutex_lock(&ct->ct_lock); > + if (conn_key_lookup(ct, &ctx->key, ctx->hash, now, NULL, NULL)) { > + ovs_mutex_unlock(&ct->ct_lock); > + COVERAGE_INC(conntrack_duplicate); > + ovs_mutex_destroy(&nc->lock); > + goto out; > + } > + if (nat_conn) { > + cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); > + } > cmap_insert(&ct->conns, &nc->cm_node, ctx->hash); > conn_expire_push_front(ct, nc); > + ovs_mutex_unlock(&ct->ct_lock); > atomic_count_inc(&ct->n_conn); > ctx->conn = nc; /* For completeness. */ > if (zl) { > @@ -1117,12 +1128,13 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, > * ephemeral ports. A DOS attack should be protected against with > * firewall rules or a separate firewall. Also using zone partitioning > * can limit DoS impact. */ > -nat_res_exhaustion: > - free(nat_conn); > - delete_conn__(nc); > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); > +nat_res_exhaustion: > VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " > "if DoS attack, use firewalling and/or zone partitioning."); > +out: > + free(nat_conn); > + delete_conn__(nc); > return NULL; > } > > @@ -1437,12 +1449,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, > } > ovs_rwlock_unlock(&ct->resources_lock); > > - ovs_mutex_lock(&ct->ct_lock); > - if (!conn_lookup(ct, &ctx->key, now, NULL, NULL)) { > - conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, > - helper, alg_exp, ct_alg_ctl, tp_id); > - } > - ovs_mutex_unlock(&ct->ct_lock); > + conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, > + helper, alg_exp, ct_alg_ctl, tp_id); > } > > write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
Ilya Maximets <i.maximets@ovn.org> writes: > On 8/2/22 06:01, wenxu@chinatelecom.cn wrote: >> From: wenxu <wenxu@chinatelecom.cn> >> >> There is a big scope ct_lock for new conn setup. The >> ct_lock should be hold only for conns map insert and >> expire rculist insert. >> >> Signed-off-by: wenxu <wenxu@chinatelecom.cn> >> --- >> lib/conntrack.c | 32 ++++++++++++++++++++------------ >> 1 file changed, 20 insertions(+), 12 deletions(-) > > Paolo, Aaron, could you, please, take a look at this one? okay - I'll take it. > Thanks! > Best regards, Ilya Maximets. > >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 13c5ab6..47d9aac 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -48,6 +48,7 @@ COVERAGE_DEFINE(conntrack_full); >> COVERAGE_DEFINE(conntrack_l3csum_err); >> COVERAGE_DEFINE(conntrack_l4csum_err); >> COVERAGE_DEFINE(conntrack_lookup_natted_miss); >> +COVERAGE_DEFINE(conntrack_duplicate); >> >> struct conn_lookup_ctx { >> struct conn_key key; >> @@ -1010,10 +1011,10 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, >> const struct nat_action_info_t *nat_action_info, >> const char *helper, const struct alg_exp_node *alg_exp, >> enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id) >> - OVS_REQUIRES(ct->ct_lock) >> { >> struct conn *nc = NULL; >> struct conn *nat_conn = NULL; >> + uint32_t nat_hash; >> >> if (!valid_new(pkt, &ctx->key)) { >> pkt->md.ct_state = CS_INVALID; >> @@ -1089,16 +1090,26 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, >> nat_conn->nat_action = 0; >> nat_conn->alg = NULL; >> nat_conn->nat_conn = NULL; >> - uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); >> - cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); >> + nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); >> } >> >> nc->nat_conn = nat_conn; >> ovs_mutex_init_adaptive(&nc->lock); >> nc->conn_type = CT_CONN_TYPE_DEFAULT; >> atomic_flag_clear(&nc->reclaimed); >> + ovs_mutex_lock(&ct->ct_lock); >> + if (conn_key_lookup(ct, &ctx->key, ctx->hash, now, NULL, NULL)) { >> + ovs_mutex_unlock(&ct->ct_lock); >> + COVERAGE_INC(conntrack_duplicate); >> + ovs_mutex_destroy(&nc->lock); >> + goto out; >> + } >> + if (nat_conn) { >> + cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); >> + } >> cmap_insert(&ct->conns, &nc->cm_node, ctx->hash); >> conn_expire_push_front(ct, nc); >> + ovs_mutex_unlock(&ct->ct_lock); >> atomic_count_inc(&ct->n_conn); >> ctx->conn = nc; /* For completeness. */ >> if (zl) { >> @@ -1117,12 +1128,13 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, >> * ephemeral ports. A DOS attack should be protected against with >> * firewall rules or a separate firewall. Also using zone partitioning >> * can limit DoS impact. */ >> -nat_res_exhaustion: >> - free(nat_conn); >> - delete_conn__(nc); >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); >> +nat_res_exhaustion: >> VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " >> "if DoS attack, use firewalling and/or zone partitioning."); >> +out: >> + free(nat_conn); >> + delete_conn__(nc); >> return NULL; >> } >> >> @@ -1437,12 +1449,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, >> } >> ovs_rwlock_unlock(&ct->resources_lock); >> >> - ovs_mutex_lock(&ct->ct_lock); >> - if (!conn_lookup(ct, &ctx->key, now, NULL, NULL)) { >> - conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, >> - helper, alg_exp, ct_alg_ctl, tp_id); >> - } >> - ovs_mutex_unlock(&ct->ct_lock); >> + conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, >> + helper, alg_exp, ct_alg_ctl, tp_id); >> } >> >> write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
On Tue, Aug 02, 2022 at 12:01:07AM -0400, wenxu@chinatelecom.cn wrote: > From: wenxu <wenxu@chinatelecom.cn> > > There is a big scope ct_lock for new conn setup. The > ct_lock should be hold only for conns map insert and > expire rculist insert. > > Signed-off-by: wenxu <wenxu@chinatelecom.cn> Hi Wenxu, This patch appears to have gone stale in patchwork, for one reason or another. If it is still relevant then I think it needs to be revisited, by being reposted after appropriate preparation. As such I'm marking this patch as "Deferred" in patchwork. No action is required unless there is a desire to revisit this patch.
diff --git a/lib/conntrack.c b/lib/conntrack.c index 13c5ab6..47d9aac 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -48,6 +48,7 @@ COVERAGE_DEFINE(conntrack_full); COVERAGE_DEFINE(conntrack_l3csum_err); COVERAGE_DEFINE(conntrack_l4csum_err); COVERAGE_DEFINE(conntrack_lookup_natted_miss); +COVERAGE_DEFINE(conntrack_duplicate); struct conn_lookup_ctx { struct conn_key key; @@ -1010,10 +1011,10 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, const struct nat_action_info_t *nat_action_info, const char *helper, const struct alg_exp_node *alg_exp, enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id) - OVS_REQUIRES(ct->ct_lock) { struct conn *nc = NULL; struct conn *nat_conn = NULL; + uint32_t nat_hash; if (!valid_new(pkt, &ctx->key)) { pkt->md.ct_state = CS_INVALID; @@ -1089,16 +1090,26 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, nat_conn->nat_action = 0; nat_conn->alg = NULL; nat_conn->nat_conn = NULL; - uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); - cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); + nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis); } nc->nat_conn = nat_conn; ovs_mutex_init_adaptive(&nc->lock); nc->conn_type = CT_CONN_TYPE_DEFAULT; atomic_flag_clear(&nc->reclaimed); + ovs_mutex_lock(&ct->ct_lock); + if (conn_key_lookup(ct, &ctx->key, ctx->hash, now, NULL, NULL)) { + ovs_mutex_unlock(&ct->ct_lock); + COVERAGE_INC(conntrack_duplicate); + ovs_mutex_destroy(&nc->lock); + goto out; + } + if (nat_conn) { + cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); + } cmap_insert(&ct->conns, &nc->cm_node, ctx->hash); conn_expire_push_front(ct, nc); + ovs_mutex_unlock(&ct->ct_lock); atomic_count_inc(&ct->n_conn); ctx->conn = nc; /* For completeness. */ if (zl) { @@ -1117,12 +1128,13 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, * ephemeral ports. A DOS attack should be protected against with * firewall rules or a separate firewall. Also using zone partitioning * can limit DoS impact. */ -nat_res_exhaustion: - free(nat_conn); - delete_conn__(nc); static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); +nat_res_exhaustion: VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " "if DoS attack, use firewalling and/or zone partitioning."); +out: + free(nat_conn); + delete_conn__(nc); return NULL; } @@ -1437,12 +1449,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, } ovs_rwlock_unlock(&ct->resources_lock); - ovs_mutex_lock(&ct->ct_lock); - if (!conn_lookup(ct, &ctx->key, now, NULL, NULL)) { - conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, - helper, alg_exp, ct_alg_ctl, tp_id); - } - ovs_mutex_unlock(&ct->ct_lock); + conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info, + helper, alg_exp, ct_alg_ctl, tp_id); } write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);