Eli Britstein <[email protected]> writes:
> From: Tim Rozet <[email protected]>
>
> The userspace datapath applies a connection's existing NAT mapping to
> a packet in the new state when a bare ct(nat) action is executed. This
> differs from the Linux datapath, which leaves a new packet untranslated
> unless the current action explicitly requests source or destination
> NAT.
>
> The kernel's nf_ct_nat() infers the NAT direction from conntrack status
> only when the connection state is not IP_CT_NEW. For IP_CT_NEW, the
> action must provide the manipulation direction:
>
> https://github.com/torvalds/linux/blob/master/net/netfilter/nf_nat_ovs.c
>
> Pass the current NAT action into handle_nat() and use its source or
> destination flags to decide whether NAT may run for a new packet. This
> also applies the same behavior to the userspace fast path.
>
> Add a system test that sends the same UDP packet twice without a reply.
> It verifies that bare NAT leaves the second new packet untranslated so
> it reaches an explicit DNAT action. Run the test against both kernel
> and userspace datapaths.
>
> Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.")
> Assisted-by: GPT-5, Codex
> Co-authored-by: Tim Rozet <[email protected]>
> Signed-off-by: Tim Rozet <[email protected]>
> Signed-off-by: Eli Britstein <[email protected]>
> ---
It needs to be documented that previously a +new+trk packet that got
recirculated through a second zone would have mapping applied for a bare
ct(...nat,...) call. That is no longer the case after this patch. We
need to make sure the documentation and NEWS reflect this so users are
aware.
> lib/conntrack.c | 16 +++++++++---
> tests/system-traffic.at | 56 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f84cdd216..168954c35 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1195,9 +1195,17 @@ conn_update_state(struct conntrack *ct, struct
> dp_packet *pkt,
>
> static void
> handle_nat(struct dp_packet *pkt, struct conn *conn,
> - uint16_t zone, bool reply, bool related)
> + uint16_t zone, bool reply, bool related,
> + const struct nat_action_info_t *nat_action_info)
> {
> + bool nat_config = nat_action_info->nat_action &
> + (NAT_ACTION_SRC | NAT_ACTION_DST);
> +
> + /* Like the kernel datapath, do not infer an existing NAT mapping for a
> + * packet in the new state. Applying NAT in this state requires an
> + * explicit source or destination NAT action. */
> if (conn->nat_action &&
> + (!(pkt->md.ct_state & CS_NEW) || nat_config) &&
One thing that I think is important to consider here -
CT_UPDATE_VALID_NEW case is the default for UDP; cases that bare ct(nat)
would have translated (thinking some unidirectional UDP flow) wouldn't
translate the same. I think retransmitted SYN packet is also falling
into this case (and that may be more common). I think the most common
case where this would really impact a user is probably udp traceroute
(but I didn't actually run that as a test case).
I'm also not sure about how this directly maps to kernel. In nf_ct_nat,
not infering a direction doesn't mean that NAT isn't applied. Netfilter
Hooks run in many places, so I think there is still some opportunity to
run nat in the post processing section (because I think snat null bind
is always processed, but I may have misread the call-in).
> (!(pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT)) ||
> (pkt->md.ct_state & (CS_SRC_NAT | CS_DST_NAT) &&
> zone != pkt->md.ct_zone))) {
> @@ -1320,7 +1328,8 @@ process_one_fast(uint16_t zone, const uint32_t *setmark,
> struct conn *conn, struct dp_packet *pkt)
> {
> if (nat_action_info) {
> - handle_nat(pkt, conn, zone, pkt->md.reply, pkt->md.icmp_related);
> + handle_nat(pkt, conn, zone, pkt->md.reply, pkt->md.icmp_related,
> + nat_action_info);
> pkt->md.conn = NULL;
> }
>
> @@ -1410,7 +1419,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> create_new_conn = conn_update_state(ct, pkt, ctx, conn, now);
> }
> if (nat_action_info && !create_new_conn) {
> - handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related);
> + handle_nat(pkt, conn, zone, ctx->reply, ctx->icmp_related,
> + nat_action_info);
> }
>
> } else if (check_orig_tuple(ct, pkt, ctx, now, &conn, nat_action_info)) {
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index ffb80d1e2..4ad51223d 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -4663,6 +4663,62 @@ NXST_FLOW reply:
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([conntrack - bare NAT on repeated new UDP connection])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_CHECK([ovs-vsctl -- add-port br0 p0 -- set Interface p0 type=internal dnl
> + ofport_request=1])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=1,udp,actions=ct(table=1,zone=42,nat)
> +table=1,cookie=0x1,priority=200,udp,nw_dst=10.1.1.64,ct_state=+new+trk-dnat,ct_mark=0,actions=ct(commit,table=2,zone=42,nat(dst=10.1.1.2:53),exec(set_field:0x1->ct_mark))
> +table=1,cookie=0x2,priority=200,udp,nw_dst=10.1.1.64,ct_state=+new+trk-dnat,ct_mark=0x1,actions=ct(commit,table=2,zone=42,nat(dst=10.1.1.2:53))
> +table=1,cookie=0x3,priority=0,actions=drop
> +table=2,cookie=0x4,priority=100,udp,nw_dst=10.1.1.2,ct_state=+new+trk+dnat,ct_mark=0x1,actions=drop
> +table=2,cookie=0x5,priority=0,actions=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +
> +dnl 10.1.1.1:40000 -> 10.1.1.64:53, with the UDP checksum disabled.
> +packet=50540000000a50540000000908004500001c000000004011648f0a0101010a0101409c40003500080000
> +
> +dnl The first packet creates the DNAT entry and stores the mark.
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${packet},actions=resubmit(,0)"])
> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack zone=42 | grep -q "mark=1"])
> +
> +dnl With no reply seen, the second packet remains new. A bare ct(nat) must
> +dnl leave it untranslated so that the explicit DNAT action is reached again.
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${packet},actions=resubmit(,0)"])
> +
If I'm reading correctly, these checks may be flaky under kernel. The
packet-out path should result in a PACKET_CMD_EXECUTE and that could
complete before the packet counters are updated. I think there should
be an OVS_WAIT_UNTIL_EQUAL here to ensure we don't fail this test
early.
> +AT_CHECK([ovs-ofctl dump-flows br0 cookie=0x1/-1 | dnl
> + grep -o "n_packets=[[0-9]]*"], [0], [dnl
> +n_packets=1
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 cookie=0x2/-1 | dnl
> + grep -o "n_packets=[[0-9]]*"], [0], [dnl
> +n_packets=1
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 cookie=0x3/-1 | dnl
> + grep -o "n_packets=[[0-9]]*"], [0], [dnl
> +n_packets=0
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 cookie=0x4/-1 | dnl
> + grep -o "n_packets=[[0-9]]*"], [0], [dnl
> +n_packets=2
> +])
> +AT_CHECK([ovs-ofctl dump-flows br0 cookie=0x5/-1 | dnl
> + grep -o "n_packets=[[0-9]]*"], [0], [dnl
> +n_packets=0
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([conntrack - generic IP protocol])
> CHECK_CONNTRACK()
> OVS_TRAFFIC_VSWITCHD_START()
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
