Re: [ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)
Eelco Chaudron writes:
> On 16 Nov 2023, at 11:07, Joseph Zhong wrote:
>
>> This patch is to avoid generating incorrect conntrack entry
>> In a certain use case of conntrack flow that if flow included
>> ct(commit, nat) action, but no detail action/direction specified,
>> CT will generate incorrect conntrack entry.
>> For example, add below flow:
>> ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
>> ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
>> table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
>> table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
>> table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
>> start traffic from 192.168.2.2 to 192.168.2.7
>> ovs dpdk datpath generate CT entry as below:
>> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
>> reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
>> reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
>> but ovs kernel datapath will generate correct ct entry as below:
>> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
>> reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)
>>
>> To compatible with this use case of flow, and also be consistent with
>> kernel datapath's behavior, this patch treat this nat without action
>> specified as not nat, and don't do "nat_get_unique_tuple" and malloc
>> a nat_conn that is attached to nc.
>
> Hi Joseph,
>
> Thanks for the patch, I’m not a conntrack expert so I’ll let Aaron, or
> Paolo review it. But would it be possible to have a test case for
> this?
It should for sure be possible to have a test case for this.
> Cheers,
>
> Eelco
>
>
>> Signed-off-by: Zhong Zhong
>>
>> ---
>> lib/conntrack.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 47a443f..581b62b 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
>> *pkt,
Something happened with your patch here and that caused issues with the
robot. Please fix.
>> nc->parent_key = alg_exp->parent_key;
>> }
>> - if (nat_action_info) {
>> + if (nat_action_info && nat_action_info->nat_action) {
>> nc->nat_action = nat_action_info->nat_action;
>> if (alg_exp) {
>> --
I think this will break expectations case with NAT.
We need a more comprehensive solution here, because there are cases to
legitimately flow into a ct(commit,nat) pipeline with a new connection
and expect that things should "just work."
Note that the kernel side has other considerations as well - because the
NAT table is shared with other subsystems. That said, I think the
expectation case is important and should be retained, so to get a more
consistent behavior, we probably need to have multiple checks for the
nat_action value.
And that showcases a deficiency in the design of the userspace NAT as
well, because it assumes:
1) either SRC or DST but not both
2) SRC or DST is set when calling these routines.
>> --
>> Best Regards
>>
>> Zhong, Zhong
>> Email: zhongzh...@gmail.com
>> ___
>> 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] lib/conntrack.c:compatible with nat with no action(direction)
On 16 Nov 2023, at 11:07, Joseph Zhong wrote:
> This patch is to avoid generating incorrect conntrack entry
> In a certain use case of conntrack flow that if flow included
> ct(commit, nat) action, but no detail action/direction specified,
> CT will generate incorrect conntrack entry.
> For example, add below flow:
> ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
> ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
> table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
> table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
> table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
> start traffic from 192.168.2.2 to 192.168.2.7
> ovs dpdk datpath generate CT entry as below:
> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
> reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
> reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
> but ovs kernel datapath will generate correct ct entry as below:
> icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
> reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)
>
> To compatible with this use case of flow, and also be consistent with
> kernel datapath's behavior, this patch treat this nat without action
> specified as not nat, and don't do "nat_get_unique_tuple" and malloc
> a nat_conn that is attached to nc.
Hi Joseph,
Thanks for the patch, I’m not a conntrack expert so I’ll let Aaron, or Paolo
review it. But would it be possible to have a test case for this?
Cheers,
Eelco
> Signed-off-by: Zhong Zhong
>
> ---
> lib/conntrack.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 47a443f..581b62b 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> nc->parent_key = alg_exp->parent_key;
> }
> - if (nat_action_info) {
> + if (nat_action_info && nat_action_info->nat_action) {
> nc->nat_action = nat_action_info->nat_action;
> if (alg_exp) {
> --
>
> --
> Best Regards
>
> Zhong, Zhong
> Email: zhongzh...@gmail.com
> ___
> 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] lib/conntrack.c:compatible with nat with no action(direction)
Bleep bloop. Greetings Joseph Zhong, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: corrupt patch at line 10 error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 lib/conntrack.c:compatible with nat with no action(direction) When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Patch skipped due to previous failure. Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] lib/conntrack.c:compatible with nat with no action(direction)
This patch is to avoid generating incorrect conntrack entry
In a certain use case of conntrack flow that if flow included
ct(commit, nat) action, but no detail action/direction specified,
CT will generate incorrect conntrack entry.
For example, add below flow:
ip,priority=500,in_port=1,ct_state=-trk actions=ct(table=1,nat)'
ip,priority=500,in_port=2,ct_state=-trk actions=ct(table=1,nat)'
table=1,in_port=1,ip,ct_state=+trk+new actions=ct*(commit,nat)*,2
table=1,in_port=1,ip,ct_state=-new+trk+est actions=2
table=1,in_port=2,ip,ct_state=-new+trk+est actions=1
start traffic from 192.168.2.2 to 192.168.2.7
ovs dpdk datpath generate CT entry as below:
icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
reply=(src=*0.0.0.0*,dst=192.168.2.2,id=17038,type=0,code=0)
reply key src 0.0.0.0 is generated not correct by "nat_get_unique_tuple".
but ovs kernel datapath will generate correct ct entry as below:
icmp,orig=(src=192.168.2.2,dst=192.168.2.7,id=17038,type=8,code=0),
reply=(src=192.168.2.7,dst=192.168.2.2,id=17038,type=0,code=0)
To compatible with this use case of flow, and also be consistent with
kernel datapath's behavior, this patch treat this nat without action
specified as not nat, and don't do "nat_get_unique_tuple" and malloc
a nat_conn that is attached to nc.
Signed-off-by: Zhong Zhong
---
lib/conntrack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/conntrack.c b/lib/conntrack.c
index 47a443f..581b62b 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -942,7 +942,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
nc->parent_key = alg_exp->parent_key;
}
- if (nat_action_info) {
+ if (nat_action_info && nat_action_info->nat_action) {
nc->nat_action = nat_action_info->nat_action;
if (alg_exp) {
--
--
Best Regards
Zhong, Zhong
Email: zhongzh...@gmail.com
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
