Dumitru Ceara <dce...@redhat.com> writes: > On 6/25/21 2:01 PM, Paolo Valerio wrote: >> Dumitru Ceara <dce...@redhat.com> writes: >> >>> On 6/21/21 12:06 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 <dce...@redhat.com> >>>> Signed-off-by: Paolo Valerio <pvale...@redhat.com> >>>> --- >>> >>> Hi Paolo, >>> >>> Thanks for the patch! I tested it and it works fine for OVN. I have a >>> few comments/questions below. >>> >> >> Thanks for the test and for the review. >> >>>> lib/conntrack.c | 30 +++++++++++++++++++++++++++++- >>>> tests/system-traffic.at | 35 +++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 64 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/lib/conntrack.c b/lib/conntrack.c >>>> index 99198a601..7e8b16a3e 100644 >>>> --- a/lib/conntrack.c >>>> +++ b/lib/conntrack.c >>>> @@ -1281,6 +1281,33 @@ 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) >>> >>> Nit: indentation. >>> >> >> ACK >> >>>> +{ >>>> + bool found; >>>> + >>>> + 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); >>>> + } >>>> + >>>> + found = conn_key_lookup(ct, &ctx->key, ctx->hash, >>>> + now, &ctx->conn, &ctx->reply); >>>> + if (natted) { >>>> + if (OVS_LIKELY(found)) { >>>> + 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 { >>>> + /* in case of failure restore the initial key. */ >>>> + conn_key_reverse(&ctx->key); >>> >>> Can the lookup actually fail? I mean, if the packet was natted, there >>> must have been a connection on which it got natted. Anyway, I think we >>> should probably also increment a coverage counter. I guess dropping >>> such packets would be hard, right? >>> >> >> I agree, it should not fail. If I'm not missing something, if it >> happens, it should be because there's been a problem somewhere else >> (e.g. a polluted ct_state value or more in general an unexpected >> scenario). For this reason, I think it's better not to drop it or even >> set it as invalid. > > I'm not sure, won't this create horrible to debug bugs when packets get > forwarded in an unexpected way? Setting it as invalid isn't good enough > in my opinion because there might be flows later in the pipeline that > perform actions (other than drop) on packets with ct_state +inv. > > The problem I have (because I don't know the conntrack code) is that I > see no easy way to drop the packet. > >> >> Yes, the coverage counter gives more meaning to the else branch. >> Alternatively, we could probably even remove it. I would leave the NULL >> check or equivalent. >> >> I have no strong preference. >> WDYT? >> > > I would prefer a coverage counter, at least to have an indication of a > problem "somewhere" in conntrack. >
Speaking with Gaƫtan, he brought a good point we didn't directly discuss about in this thread, but that needs to be considered. Removing a conn (e.g. flushing) during the recirculation would lead to a lookup failure. The counter would then count those cases as well potentially adding noise (if we consider it as an indication of potential problems). > Regards, > Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev