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.
> 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.
> +{
> + 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?
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
