Eli Britstein <[email protected]> writes: > 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".
This is a long-standing source of friction, tbh. It spans all the way back to 2015 and the initial FreeBSD port of conntrack. There are lots of places where userspace behavior and kernel behavior do not match. That doesn't mean one or the other is wrong. They are different. There have been many efforts to try and make these implementations match. Here is one such discussion (from Dec 2017): https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341586.html That also doesn't mean I would reject changes (since I'm reviewing this series). I just don't think they are fixes. > 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). That isn't an argument for something being a fix. Fragmentation handling is different, ICMP handling is different, routing is (slightly) different. I can design cases where things work with netlink datapath or with netdev datapath and not the other. > Regarding offload, I don't exactly understand your meaning. I meant hardware CT offload implementations may also diverge from kernel CT behavior, furthering "different implementation" != "bug to fix". For example, MLX doesn't do things like window validation or control flow validation (last I understood). These aren't minor differences. > 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
