Hey Aaron, I understand your point that this may not qualify as a fix to the original implementation, and I'm fine with removing the Fixes tag. However, I'm not sure I agree with that:
Each CT implementation is allowed to implement CT in whichever way it chooses. At the OpenFlow pipeline level with the same flow and packet, a bare ct(nat) action produces different packet tuples in the kernel and userspace datapath. This causes later tables to select different flows and breaks consumers like OVN. I think this is independent of how conntrack may be implemented or offloaded in hardware. IMO the OVS action accepted by both datapaths should provide consistent pipeline behavior. Otherwise, consumers must detect the datapath and program around its different behavior. My understanding is userspace is supposed to have as much parity with kernel as practically possible. If that is not the case then we would need to document the differences in OVS so that consumers can be aware to avoid subtle bugs that are difficult to diagnose. Thanks, -Tim ________________________________ From: Aaron Conole <[email protected]> Sent: Monday, September 14, 2026 11:45 AM To: Eli Britstein <[email protected]> Cc: [email protected] <[email protected]>; Tim Rozet <[email protected]>; Alin Serdean <[email protected]>; Maor Dickman <[email protected]> Subject: Re: [PATCH v1 1/3] conntrack: Match kernel NAT handling for new packets. 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. > 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
