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
