| Message ID | 162557658579.3649487.7565188486964323862.stgit@fed.void |
|---|---|
| State | Accepted |
| Headers | show |
| Series | conntrack: add all-zero SNAT | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
On 7/6/21 3:03 PM, Paolo Valerio wrote: > when a packet gets dnatted and then recirculated, it could be possible > that it matches another rule that performs another nat action. > The kernel datapath handles this situation turning to a no-op the > second nat action, so natting only once the packet. In the userspace > datapath instead, when the ct action gets executed, an initial lookup > of the translated packet fails to retrieve the connection related to > the packet, leading to the creation of a new entry in ct for the src > nat action with a subsequent failure of the connection establishment. > > with the following flows: > > table=0,priority=30,in_port=1,ip,nw_dst=192.168.2.100,actions=ct(commit,nat(dst=10.1.1.2:80),table=1) > table=0,priority=20,in_port=2,ip,actions=ct(nat,table=1) > table=0,priority=10,ip,actions=resubmit(,2) > table=0,priority=10,arp,actions=NORMAL > table=0,priority=0,actions=drop > table=1,priority=5,ip,actions=ct(commit,nat(src=10.1.1.240),table=2) > table=2,in_port=ovs-l0,actions=2 > table=2,in_port=ovs-r0,actions=1 > > establishing a connection from 10.1.1.1 to 192.168.2.100 the outcome is: > > tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.240,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) > tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) > > with this patch applied the outcome is: > > tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) > > The patch performs, for already natted packets, a lookup of the > reverse key in order to retrieve the related entry, it also adds a > test case that besides testing the scenario ensures that the other ct > actions are executed. > > Reported-by: Dumitru Ceara <dceara@redhat.com> > Signed-off-by: Paolo Valerio <pvalerio@redhat.com> > --- Hi Paolo, > lib/conntrack.c | 32 +++++++++++++++++++++++++++++++- > tests/system-traffic.at | 35 +++++++++++++++++++++++++++++++++++ > 2 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 2e803cac6..b6a35edc0 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -47,6 +47,7 @@ COVERAGE_DEFINE(conntrack_full); > COVERAGE_DEFINE(conntrack_long_cleanup); > COVERAGE_DEFINE(conntrack_l3csum_err); > COVERAGE_DEFINE(conntrack_l4csum_err); > +COVERAGE_DEFINE(conntrack_lookup_natted_miss); > > struct conn_lookup_ctx { > struct conn_key key; > @@ -1282,6 +1283,34 @@ process_one_fast(uint16_t zone, const uint32_t *setmark, > } > } > > +static void > +initial_conn_lookup(struct conntrack *ct, struct conn_lookup_ctx *ctx, > + long long now, bool natted) > +{ > + if (natted) { > + /* if the packet has been already natted (e.g. a previous Really tiny nit: s/if/If I guess this can be easily fixed by maintainers at apply time. I tested this patch (also with OVN) and ran OVS and OVN self and system tests. The results look good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com> Regards, Dumitru
diff --git a/lib/conntrack.c b/lib/conntrack.c index 2e803cac6..b6a35edc0 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -47,6 +47,7 @@ COVERAGE_DEFINE(conntrack_full); COVERAGE_DEFINE(conntrack_long_cleanup); COVERAGE_DEFINE(conntrack_l3csum_err); COVERAGE_DEFINE(conntrack_l4csum_err); +COVERAGE_DEFINE(conntrack_lookup_natted_miss); struct conn_lookup_ctx { struct conn_key key; @@ -1282,6 +1283,34 @@ process_one_fast(uint16_t zone, const uint32_t *setmark, } } +static void +initial_conn_lookup(struct conntrack *ct, struct conn_lookup_ctx *ctx, + long long now, bool natted) +{ + if (natted) { + /* if the packet has been already natted (e.g. a previous + * action took place), retrieve it performing a lookup of its + * reverse key. */ + conn_key_reverse(&ctx->key); + } + + conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply); + + if (natted) { + if (OVS_LIKELY(ctx->conn)) { + ctx->reply = !ctx->reply; + ctx->key = ctx->reply ? ctx->conn->rev_key : ctx->conn->key; + ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); + } else { + /* A lookup failure does not necessarily imply that an + * error occurred, it may simply indicate that a conn got + * removed during the recirculation. */ + COVERAGE_INC(conntrack_lookup_natted_miss); + conn_key_reverse(&ctx->key); + } + } +} + static void process_one(struct conntrack *ct, struct dp_packet *pkt, struct conn_lookup_ctx *ctx, uint16_t zone, @@ -1297,7 +1326,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, } bool create_new_conn = false; - conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply); + initial_conn_lookup(ct, ctx, now, !!(pkt->md.ct_state & + (CS_SRC_NAT | CS_DST_NAT))); struct conn *conn = ctx->conn; /* Delete found entry if in wrong direction. 'force' implies commit. */ diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 6a15d08c2..f400cfabc 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -4588,6 +4588,41 @@ tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src= OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([conntrack - DNAT with additional SNAT]) +CHECK_CONNTRACK() +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0, at_ns1) +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") +NS_CHECK_EXEC([at_ns0], [ip route add 172.1.1.0/24 via 10.1.1.2]) + +OVS_START_L7([at_ns1], [http]) + +AT_DATA([flows.txt], [dnl +table=0,priority=30,in_port=1,ip,nw_dst=172.1.1.2,actions=ct(commit,nat(dst=10.1.1.2:80),table=1) +table=0,priority=20,in_port=2,ip,actions=ct(nat),1 +table=0,priority=10,arp,actions=NORMAL +table=0,priority=1,actions=drop +dnl Be sure all ct() actions but src nat are executed +table=1,ip,actions=ct(commit,nat(src=10.1.1.240),exec(set_field:0xac->ct_mark,set_field:0xac->ct_label),table=2) +table=2,in_port=1,ip,ct_mark=0xac,ct_label=0xac,actions=2 +]) +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) + +NS_CHECK_EXEC([at_ns0], [wget http://172.1.1.2:8080 -t 5 -T 1 --retry-connrefused -v -o wget0.log]) + +dnl - make sure only dst nat has been performed +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.240)], [0], [dnl +]) + +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.1)], [0], [dnl +tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),mark=172,labels=0xac,protoinfo=(state=<cleared>) +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([conntrack - more complex DNAT]) CHECK_CONNTRACK() CHECK_CONNTRACK_NAT()
when a packet gets dnatted and then recirculated, it could be possible that it matches another rule that performs another nat action. The kernel datapath handles this situation turning to a no-op the second nat action, so natting only once the packet. In the userspace datapath instead, when the ct action gets executed, an initial lookup of the translated packet fails to retrieve the connection related to the packet, leading to the creation of a new entry in ct for the src nat action with a subsequent failure of the connection establishment. with the following flows: table=0,priority=30,in_port=1,ip,nw_dst=192.168.2.100,actions=ct(commit,nat(dst=10.1.1.2:80),table=1) table=0,priority=20,in_port=2,ip,actions=ct(nat,table=1) table=0,priority=10,ip,actions=resubmit(,2) table=0,priority=10,arp,actions=NORMAL table=0,priority=0,actions=drop table=1,priority=5,ip,actions=ct(commit,nat(src=10.1.1.240),table=2) table=2,in_port=ovs-l0,actions=2 table=2,in_port=ovs-r0,actions=1 establishing a connection from 10.1.1.1 to 192.168.2.100 the outcome is: tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.240,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) with this patch applied the outcome is: tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),protoinfo=(state=ESTABLISHED) The patch performs, for already natted packets, a lookup of the reverse key in order to retrieve the related entry, it also adds a test case that besides testing the scenario ensures that the other ct actions are executed. Reported-by: Dumitru Ceara <dceara@redhat.com> Signed-off-by: Paolo Valerio <pvalerio@redhat.com> --- lib/conntrack.c | 32 +++++++++++++++++++++++++++++++- tests/system-traffic.at | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-)