| Message ID | CAGOK1xVJPBvw7K6DxbsDQYWgcgm6gTqEJP+hNqGC+CGdUDnbXQ@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] lib/conntrack.c:compatible with nat with no action(direction) | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
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 <zhongzh323@gmail.com> > > --- > 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: zhongzh323@gmail.com > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Eelco Chaudron <echaudro@redhat.com> 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 <zhongzh323@gmail.com> >> >> --- >> 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: zhongzh323@gmail.com >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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) { --
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 <zhongzh323@gmail.com> --- lib/conntrack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)