On 14/09/2026 18:45, Aaron Conole wrote:
External email: Use caution opening links or attachments


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.")
I wouldn't call this a fix.  Each CT implementation is allowed to
implement CT in whichever way it chooses.  For example, there will be CT
offload paths, and they may not behave similarly.

I am planning on reviewing the series this week, but just noting that
even if the series is applied as-is, I wouldn't consider this a 'fix'
and would strip that label unless there was compelling reason not to do
so.
I think "whichever way it chooses" is too loose claim. The kernel's implementation is the most mature one, and treated in this series as the "source of truth".

For each commit in the series there is a testsuite part(s) that passes on the kernel's CT but fail on the userspace (without applying lib/conntrack.c part).

Regarding offload, I don't exactly understand your meaning.

Thanks upfront for the review.


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]>
---
  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) &&
          (!(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)"])
+
+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

Reply via email to