Brian Haley <[email protected]> writes: > Hi Paolo, > > On 4/19/23 2:40 PM, Paolo Valerio wrote: >> During the creation of a new connection, there's a chance both key and >> rev_key end up having the same hash. This is more common in the case >> of all-zero snat with no collisions. In that case, once the >> connection is expired, but not cleaned up, if a new packet with the >> same 5-tuple is received, an assertion failure gets triggered in >> conn_update_state() because of a previous failure of retrieving a >> CT_CONN_TYPE_DEFAULT connection. >> >> Fix it by releasing the nat_conn during the connection creation in the >> case of same hash for both key and rev_key. > > Sorry for reviving a two month-old thread, but we recently started > seeing this issue which seemed to also be related to [0], but I can't > find it in patchworks or the tree. Was there a plan to update it? >
Hi Brian, It transitioned to "Changes Requested" [0]. At the moment the idea is to upstream a patch initially proposed by Peng. I'm pretty busy at the moment, and I can't look at it right away, but yes, the plan is to update it. [0] https://patchwork.ozlabs.org/project/openvswitch/list/?series=351579&state=* > Thanks, > > -Brian > > [0] https://www.mail-archive.com/[email protected]/msg08945.html > >> >> Reported-by: Michael Plato <[email protected]> >> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.") >> Signed-off-by: Paolo Valerio <[email protected]> >> --- >> In this thread [0] there are some more details. A similar >> approach here could be to avoid to add the nat_conn to the cmap and >> letting the sweeper release the memory for nat_conn once the whole >> connection gets freed. >> That approach could still be ok, but the drawback is that it could >> require a different patch for older branches that don't include >> 3d9c1b855a5f ("conntrack: Replace timeout based expiration lists with >> rculists."). It still worth to be considered. >> >> [0] https://mail.openvswitch.org/pipermail/ovs-discuss/2023-April/052339.html >> --- >> lib/conntrack.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/lib/conntrack.c b/lib/conntrack.c >> index 7e1fc4b1f..d2ee127d9 100644 >> --- a/lib/conntrack.c >> +++ b/lib/conntrack.c >> @@ -1007,14 +1007,19 @@ conn_not_found(struct conntrack *ct, struct >> dp_packet *pkt, >> } >> >> nat_packet(pkt, nc, false, ctx->icmp_related); >> - memcpy(&nat_conn->key, &nc->rev_key, sizeof nat_conn->key); >> - memcpy(&nat_conn->rev_key, &nc->key, sizeof nat_conn->rev_key); >> - nat_conn->conn_type = CT_CONN_TYPE_UN_NAT; >> - 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); >> + uint32_t nat_hash = conn_key_hash(&nc->rev_key, ct->hash_basis); >> + if (nat_hash != ctx->hash) { >> + memcpy(&nat_conn->key, &nc->rev_key, sizeof nat_conn->key); >> + memcpy(&nat_conn->rev_key, &nc->key, sizeof >> nat_conn->rev_key); >> + nat_conn->conn_type = CT_CONN_TYPE_UN_NAT; >> + nat_conn->nat_action = 0; >> + nat_conn->alg = NULL; >> + nat_conn->nat_conn = NULL; >> + cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash); >> + } else { >> + free(nat_conn); >> + nat_conn = NULL; >> + } >> } >> >> nc->nat_conn = nat_conn; >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
