[ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
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.
Reported-by: Michael Plato
Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
Signed-off-by: Paolo Valerio
---
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
On 4/19/23 20:40, 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.
This sounds a bit odd. Shouldn't we treat hash collision as a normal case?
Looking at the code, I'm assuming that the issue comes from the following
part in process_one():
if (OVS_LIKELY(conn)) {
if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
...
conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
And here we get the same connection again, because the default one is already
expired. Is that correct?
If so, maybe we should add an extra condition to conn_key_lookup() to
only look for DEFAULT connections instead, just for this case? Since
we really don't want to get the UN_NAT one here.
Best regards, Ilya Maximets.
>
> Reported-by: Michael Plato
> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
> Signed-off-by: Paolo Valerio
> ---
> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
Ilya Maximets writes:
> On 4/19/23 20:40, 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.
>
> This sounds a bit odd. Shouldn't we treat hash collision as a normal case?
>
> Looking at the code, I'm assuming that the issue comes from the following
> part in process_one():
>
> if (OVS_LIKELY(conn)) {
> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
> ...
> conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
>
> And here we get the same connection again, because the default one is already
> expired. Is that correct?
>
> If so, maybe we should add an extra condition to conn_key_lookup() to
> only look for DEFAULT connections instead, just for this case? Since
> we really don't want to get the UN_NAT one here.
>
Hello Ilya,
It's a fair point.
I initially thought about the approach you're suggesting, but I had some
concerns about it that I'll try to summarize below.
For sure it would fix the issue (it could require the first patch to be
applied as well for the branches with rcu exp lists).
Based on the current logic, new packets matching that expired connection
but not evicted will be marked as +inv and further packets will be
marked so for the whole sweep interval unless an exception like this get
added:
uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
/* the last flag indicates CT_CONN_TYPE_DEFAULT only */
conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
/* special case where there's hash collision */
if (!conn && ctx->hash != hash) {
pkt->md.ct_state |= CS_INVALID;
write_ct_md(pkt, zone, NULL, NULL, NULL);
...
return;
}
This would further require that subsequent lookup in the create_new_conn
path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
/* Only check for CT_CONN_TYPE_DEFAULT */
if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
helper, alg_exp, ct_alg_ctl, tp_id);
}
otherwise we could incur in a false positive which prevent to create a
new connection.
> Best regards, Ilya Maximets.
>
>>
>> Reported-by: Michael Plato
>> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
>> Signed-off-by: Paolo Valerio
>> ---
>> 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 {
>> +
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
On 5/4/23 19:21, Paolo Valerio wrote:
> Ilya Maximets writes:
>
>> On 4/19/23 20:40, 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.
>>
>> This sounds a bit odd. Shouldn't we treat hash collision as a normal case?
>>
>> Looking at the code, I'm assuming that the issue comes from the following
>> part in process_one():
>>
>> if (OVS_LIKELY(conn)) {
>> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
>> ...
>> conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
>>
>> And here we get the same connection again, because the default one is already
>> expired. Is that correct?
>>
>> If so, maybe we should add an extra condition to conn_key_lookup() to
>> only look for DEFAULT connections instead, just for this case? Since
>> we really don't want to get the UN_NAT one here.
>>
>
> Hello Ilya,
>
> It's a fair point.
> I initially thought about the approach you're suggesting, but I had some
> concerns about it that I'll try to summarize below.
>
> For sure it would fix the issue (it could require the first patch to be
> applied as well for the branches with rcu exp lists).
>
> Based on the current logic, new packets matching that expired connection
> but not evicted will be marked as +inv and further packets will be
> marked so for the whole sweep interval unless an exception like this get
> added:
>
> uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
> /* the last flag indicates CT_CONN_TYPE_DEFAULT only */
> conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
> /* special case where there's hash collision */
> if (!conn && ctx->hash != hash) {
> pkt->md.ct_state |= CS_INVALID;
> write_ct_md(pkt, zone, NULL, NULL, NULL);
> ...
> return;
> }
>
> This would further require that subsequent lookup in the create_new_conn
> path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
>
> uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
> /* Only check for CT_CONN_TYPE_DEFAULT */
> if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> helper, alg_exp, ct_alg_ctl, tp_id);
> }
>
> otherwise we could incur in a false positive which prevent to create a
> new connection.
I'm not really sure if what described above is more correct way of doing
things or not... Aaron, do you have opinion on this?
Another thought: Can we expire the CT_CONN_TYPE_UN_NAT connection the
moment DEFAULT counterpart of it expires? Or that will that be against
some logic / not possible to do?
>
>> Best regards, Ilya Maximets.
>>
>>>
>>> Reported-by: Michael Plato
>>> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
>>> Signed-off-by: Paolo Valerio
>>> ---
>>> 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);
>>> +i
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
Ilya Maximets writes:
> On 5/4/23 19:21, Paolo Valerio wrote:
>> Ilya Maximets writes:
>>
>>> On 4/19/23 20:40, 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.
>>>
>>> This sounds a bit odd. Shouldn't we treat hash collision as a normal case?
>>>
>>> Looking at the code, I'm assuming that the issue comes from the following
>>> part in process_one():
>>>
>>> if (OVS_LIKELY(conn)) {
>>> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
>>> ...
>>> conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
>>>
>>> And here we get the same connection again, because the default one is
>>> already
>>> expired. Is that correct?
>>>
>>> If so, maybe we should add an extra condition to conn_key_lookup() to
>>> only look for DEFAULT connections instead, just for this case? Since
>>> we really don't want to get the UN_NAT one here.
>>>
>>
>> Hello Ilya,
>>
>> It's a fair point.
>> I initially thought about the approach you're suggesting, but I had some
>> concerns about it that I'll try to summarize below.
>>
>> For sure it would fix the issue (it could require the first patch to be
>> applied as well for the branches with rcu exp lists).
>>
>> Based on the current logic, new packets matching that expired connection
>> but not evicted will be marked as +inv and further packets will be
>> marked so for the whole sweep interval unless an exception like this get
>> added:
>>
>> uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
>> /* the last flag indicates CT_CONN_TYPE_DEFAULT only */
>> conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
>> /* special case where there's hash collision */
>> if (!conn && ctx->hash != hash) {
>> pkt->md.ct_state |= CS_INVALID;
>> write_ct_md(pkt, zone, NULL, NULL, NULL);
>> ...
>> return;
>> }
>>
>> This would further require that subsequent lookup in the create_new_conn
>> path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
>>
>> uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
>> /* Only check for CT_CONN_TYPE_DEFAULT */
>> if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
>> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
>> helper, alg_exp, ct_alg_ctl, tp_id);
>> }
>>
>> otherwise we could incur in a false positive which prevent to create a
>> new connection.
>
> I'm not really sure if what described above is more correct way of doing
> things or not... Aaron, do you have opinion on this?
>
> Another thought: Can we expire the CT_CONN_TYPE_UN_NAT connection the
> moment DEFAULT counterpart of it expires? Or that will that be against
> some logic / not possible to do?
>
As far as I can tell, this could not be straightforward as simply
marking it as expired should not be reliable (e.g. doing it from the
sweeper), and I guess that managing the expiration time field for the
nat_conn as well would require updating the nat_conn every time the
default one gets updated, probably making it a bit unpractical.
Another approach would be removing the nat_conn [1] altogether.
The problem in this case is backporting. Some adjustments that would add
to the patch might be needed for older branches.
[1]
https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>
>>
>>> Best regards, Ilya Maximets.
>>>
Reported-by: Michael Plato
Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
Signed-off-by: Paolo Valerio
---
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,
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
Paolo Valerio writes:
> Ilya Maximets writes:
>
>> On 5/4/23 19:21, Paolo Valerio wrote:
>>> Ilya Maximets writes:
>>>
On 4/19/23 20:40, 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.
This sounds a bit odd. Shouldn't we treat hash collision as a normal case?
Looking at the code, I'm assuming that the issue comes from the following
part in process_one():
if (OVS_LIKELY(conn)) {
if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
...
conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
And here we get the same connection again, because the default one is
already
expired. Is that correct?
If so, maybe we should add an extra condition to conn_key_lookup() to
only look for DEFAULT connections instead, just for this case? Since
we really don't want to get the UN_NAT one here.
>>>
>>> Hello Ilya,
>>>
>>> It's a fair point.
>>> I initially thought about the approach you're suggesting, but I had some
>>> concerns about it that I'll try to summarize below.
>>>
>>> For sure it would fix the issue (it could require the first patch to be
>>> applied as well for the branches with rcu exp lists).
>>>
>>> Based on the current logic, new packets matching that expired connection
>>> but not evicted will be marked as +inv and further packets will be
>>> marked so for the whole sweep interval unless an exception like this get
>>> added:
>>>
>>> uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
>>> /* the last flag indicates CT_CONN_TYPE_DEFAULT only */
>>> conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
>>> /* special case where there's hash collision */
>>> if (!conn && ctx->hash != hash) {
>>> pkt->md.ct_state |= CS_INVALID;
>>> write_ct_md(pkt, zone, NULL, NULL, NULL);
>>> ...
>>> return;
>>> }
>>>
>>> This would further require that subsequent lookup in the create_new_conn
>>> path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
>>>
>>> uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
>>> /* Only check for CT_CONN_TYPE_DEFAULT */
>>> if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
>>> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
>>> helper, alg_exp, ct_alg_ctl, tp_id);
>>> }
>>>
>>> otherwise we could incur in a false positive which prevent to create a
>>> new connection.
>>
>> I'm not really sure if what described above is more correct way of doing
>> things or not... Aaron, do you have opinion on this?
>>
>> Another thought: Can we expire the CT_CONN_TYPE_UN_NAT connection the
>> moment DEFAULT counterpart of it expires? Or that will that be against
>> some logic / not possible to do?
>>
>
> As far as I can tell, this could not be straightforward as simply
> marking it as expired should not be reliable (e.g. doing it from the
> sweeper), and I guess that managing the expiration time field for the
> nat_conn as well would require updating the nat_conn every time the
> default one gets updated, probably making it a bit unpractical.
>
> Another approach would be removing the nat_conn [1] altogether.
> The problem in this case is backporting. Some adjustments that would add
> to the patch might be needed for older branches.
>
> [1]
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
I think that work was interesting, and maybe the best way to go
forward. Backports would become difficult, though - agreed.
>>
>>>
Best regards, Ilya Maximets.
>
> Reported-by: Michael Plato
> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
> Signed-off-by: Paolo Valerio
> ---
> 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
>
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
Hi, Paolo,
IIRC, you have a revision version on these patches?
I guess it should be closer to the upstream than mine?
Aaron Conole 于2023年5月17日周三 21:54写道：
> Paolo Valerio writes:
>
> > Ilya Maximets writes:
> >
> >> On 5/4/23 19:21, Paolo Valerio wrote:
> >>> Ilya Maximets writes:
> >>>
> On 4/19/23 20:40, 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.
>
> This sounds a bit odd. Shouldn't we treat hash collision as a normal
> case?
>
> Looking at the code, I'm assuming that the issue comes from the
> following
> part in process_one():
>
> if (OVS_LIKELY(conn)) {
> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
> ...
> conn_key_lookup(ct, &ctx->key, hash, now, &conn,
> &ctx->reply);
>
> And here we get the same connection again, because the default one is
> already
> expired. Is that correct?
>
> If so, maybe we should add an extra condition to conn_key_lookup() to
> only look for DEFAULT connections instead, just for this case? Since
> we really don't want to get the UN_NAT one here.
>
> >>>
> >>> Hello Ilya,
> >>>
> >>> It's a fair point.
> >>> I initially thought about the approach you're suggesting, but I had
> some
> >>> concerns about it that I'll try to summarize below.
> >>>
> >>> For sure it would fix the issue (it could require the first patch to be
> >>> applied as well for the branches with rcu exp lists).
> >>>
> >>> Based on the current logic, new packets matching that expired
> connection
> >>> but not evicted will be marked as +inv and further packets will be
> >>> marked so for the whole sweep interval unless an exception like this
> get
> >>> added:
> >>>
> >>> uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
> >>> /* the last flag indicates CT_CONN_TYPE_DEFAULT only */
> >>> conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
> >>> /* special case where there's hash collision */
> >>> if (!conn && ctx->hash != hash) {
> >>> pkt->md.ct_state |= CS_INVALID;
> >>> write_ct_md(pkt, zone, NULL, NULL, NULL);
> >>> ...
> >>> return;
> >>> }
> >>>
> >>> This would further require that subsequent lookup in the
> create_new_conn
> >>> path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
> >>>
> >>> uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
> >>> /* Only check for CT_CONN_TYPE_DEFAULT */
> >>> if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
> >>> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> >>> helper, alg_exp, ct_alg_ctl, tp_id);
> >>> }
> >>>
> >>> otherwise we could incur in a false positive which prevent to create a
> >>> new connection.
> >>
> >> I'm not really sure if what described above is more correct way of doing
> >> things or not... Aaron, do you have opinion on this?
> >>
> >> Another thought: Can we expire the CT_CONN_TYPE_UN_NAT connection the
> >> moment DEFAULT counterpart of it expires? Or that will that be against
> >> some logic / not possible to do?
> >>
> >
> > As far as I can tell, this could not be straightforward as simply
> > marking it as expired should not be reliable (e.g. doing it from the
> > sweeper), and I guess that managing the expiration time field for the
> > nat_conn as well would require updating the nat_conn every time the
> > default one gets updated, probably making it a bit unpractical.
> >
> > Another approach would be removing the nat_conn [1] altogether.
> > The problem in this case is backporting. Some adjustments that would add
> > to the patch might be needed for older branches.
> >
> > [1]
> >
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0...@bytedance.com/
>
> I think that work was interesting, and maybe the best way to go
> forward. Backports would become difficult, though - agreed.
>
> >>
> >>>
> Best regards, Ilya Maximets.
>
> >
> > Reported-by: Michael Plato
> > Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP
> address.")
> > Signed-off-by: Paolo Valerio
> > ---
> > 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
> > connecti
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
On 5/18/23 09:37, Peng He wrote:
> Hi, Paolo,
>
> IIRC, you have a revision version on these patches?
> I guess it should be closer to the upstream than mine?
I suppose, this one was the last revision of the patch:
https://patchwork.ozlabs.org/project/openvswitch/patch/165668250354.1967719.13981409928749386554.st...@fed.void/
It was part of the 'buckets' approach for multithread
scalability and we didn't go with it.
Best regards, Ilya Maximets.
>
>
> Aaron Conole mailto:acon...@redhat.com>> 于2023年5月17日周三
> 21:54写道：
>
> Paolo Valerio mailto:pvale...@redhat.com>> writes:
>
> > Ilya Maximets mailto:i.maxim...@ovn.org>> writes:
> >
> >> On 5/4/23 19:21, Paolo Valerio wrote:
> >>> Ilya Maximets mailto:i.maxim...@ovn.org>> writes:
> >>>
> On 4/19/23 20:40, 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.
>
> This sounds a bit odd. Shouldn't we treat hash collision as a
> normal case?
>
> Looking at the code, I'm assuming that the issue comes from the
> following
> part in process_one():
>
> if (OVS_LIKELY(conn)) {
> if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
> ...
> conn_key_lookup(ct, &ctx->key, hash, now, &conn,
> &ctx->reply);
>
> And here we get the same connection again, because the default one
> is already
> expired. Is that correct?
>
> If so, maybe we should add an extra condition to conn_key_lookup() to
> only look for DEFAULT connections instead, just for this case? Since
> we really don't want to get the UN_NAT one here.
>
> >>>
> >>> Hello Ilya,
> >>>
> >>> It's a fair point.
> >>> I initially thought about the approach you're suggesting, but I had
> some
> >>> concerns about it that I'll try to summarize below.
> >>>
> >>> For sure it would fix the issue (it could require the first patch to
> be
> >>> applied as well for the branches with rcu exp lists).
> >>>
> >>> Based on the current logic, new packets matching that expired
> connection
> >>> but not evicted will be marked as +inv and further packets will be
> >>> marked so for the whole sweep interval unless an exception like this
> get
> >>> added:
> >>>
> >>> uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
> >>> /* the last flag indicates CT_CONN_TYPE_DEFAULT only */
> >>> conn_key_lookup_(ct, &ctx->key, hash, now, &conn, &ctx->reply, true);
> >>> /* special case where there's hash collision */
> >>> if (!conn && ctx->hash != hash) {
> >>> pkt->md.ct_state |= CS_INVALID;
> >>> write_ct_md(pkt, zone, NULL, NULL, NULL);
> >>> ...
> >>> return;
> >>> }
> >>>
> >>> This would further require that subsequent lookup in the
> create_new_conn
> >>> path are restricted to CT_CONN_TYPE_DEFAULT, e.g.:
> >>>
> >>> uint32_t hash = conn_key_hash(&ctx->key, ct->hash_basis);
> >>> /* Only check for CT_CONN_TYPE_DEFAULT */
> >>> if (!conn_key_lookup_(ct, &ctx->key, hash, now, NULL, NULL, true)) {
> >>> conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> >>> helper, alg_exp, ct_alg_ctl, tp_id);
> >>> }
> >>>
> >>> otherwise we could incur in a false positive which prevent to create a
> >>> new connection.
> >>
> >> I'm not really sure if what described above is more correct way of
> doing
> >> things or not... Aaron, do you have opinion on this?
> >>
> >> Another thought: Can we expire the CT_CONN_TYPE_UN_NAT connection the
> >> moment DEFAULT counterpart of it expires? Or that will that be against
> >> some logic / not possible to do?
> >>
> >
> > As far as I can tell, this could not be straightforward as simply
> > marking it as expired should not be reliable (e.g. doing it from the
> > sweeper), and I guess that managing the expiration time field for the
> > nat_conn as well would require updating the nat_conn every time the
> > default one gets updated, probably making it a bit unpractical.
> >
> > Another approach would be removing the nat_conn [1] altogether.
> > The problem
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
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?
Thanks,
-Brian
[0] https://www.mail-archive.com/ovs-discuss@openvswitch.org/msg08945.html
Reported-by: Michael Plato
Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
Signed-off-by: Paolo Valerio
---
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
Brian Haley 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/ovs-discuss@openvswitch.org/msg08945.html
>
>>
>> Reported-by: Michael Plato
>> Fixes: 61e48c2d1db2 ("conntrack: Handle SNAT with all-zero IP address.")
>> Signed-off-by: Paolo Valerio
>> ---
>> 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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] conntrack: Release nat_conn in case both keys have the same hash.
On 6/8/23 3:52 PM, Paolo Valerio wrote: Brian Haley 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=* Ok, let me know if you need any help with it, might be able to put it in our environment that's showing the problem. -Brian ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
