Skip to content

Fix SNAT rollback #700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
guvenc merged 4 commits into from
Jul 24, 2025
Merged

Fix SNAT rollback #700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
437982e
Fix SNAT rollback
PlagueCZ Jul 18, 2025
01259a8
Put used port macros to one place
PlagueCZ Jul 18, 2025
b3254dd
Fix edge-case in SNAT deletion
PlagueCZ Jul 21, 2025
c52b306
Better logging for snat portoverload deletion error
PlagueCZ Jul 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/dp_nat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ int dp_add_neighnat_entry(uint32_t nat_ip, uint32_t vni, uint16_t min_port, uin

int dp_del_neighnat_entry(uint32_t nat_ip, uint32_t vni, uint16_t min_port, uint16_t max_port);

int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *df, uint32_t vni);
int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *df, struct dp_port *port);
const union dp_ipv6 *dp_lookup_neighnat_underlay_ip(struct dp_flow *df);
int dp_remove_network_snat_port(const struct flow_value *cntrack);

Expand Down
38 changes: 23 additions & 15 deletions src/dp_nat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ const union dp_ipv6 *dp_lookup_neighnat_underlay_ip(struct dp_flow *df)
return NULL;
}

int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *df, uint32_t vni)
int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *df, struct dp_port *port)
{
struct netnat_portoverload_tbl_key portoverload_tbl_key;
struct netnat_portmap_key portmap_key;
Expand Down Expand Up @@ -626,7 +626,7 @@ int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *d
iface_src_port = ntohs(df->l4_info.trans_port.src_port);

portmap_key.iface_src_port = iface_src_port;
portmap_key.vni = vni;
portmap_key.vni = port->iface.vni;

portoverload_tbl_key.nat_ip = snat_data->nat_ip;
portoverload_tbl_key.l4_type = df->l4_type;
Expand Down Expand Up @@ -682,7 +682,7 @@ int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *d
snat_data->log_timestamp = timestamp;
DPS_LOG_WARNING("NAT portmap range is full",
DP_LOG_IPV4(snat_data->nat_ip),
DP_LOG_VNI(vni), DP_LOG_SRC_IPV4(iface_src_ip),
DP_LOG_VNI(port->iface.vni), DP_LOG_SRC_IPV4(iface_src_ip),
DP_LOG_SRC_PORT(iface_src_port));
}
return DP_ERROR;
Expand Down Expand Up @@ -716,6 +716,8 @@ int dp_allocate_network_snat_port(struct snat_data *snat_data, struct dp_flow *d
}
}

DP_STATS_NAT_INC_USED_PORT_CNT(port);

return allocated_port;
}

Expand Down Expand Up @@ -746,8 +748,10 @@ int dp_remove_network_snat_port(const struct flow_value *cntrack)

// forcefully delete, if it was never there, it's fine
ret = rte_hash_del_key(ipv4_netnat_portoverload_tbl, &portoverload_tbl_key);
if (DP_FAILED(ret) && ret != -ENOENT)
if (DP_FAILED(ret) && ret != -ENOENT) {
DPS_LOG_ERR("Cannot delete portoverload key", DP_LOG_RET(ret));
return ret;
}

dp_copy_ipaddr(&portmap_key.src_ip, &flow_key_org->l3_src);
portmap_key.iface_src_port = flow_key_org->src.port_src;
Expand All @@ -759,18 +763,22 @@ int dp_remove_network_snat_port(const struct flow_value *cntrack)
portmap_key.iface_src_port = flow_key_org->src.port_src;

ret = rte_hash_lookup_data(ipv4_netnat_portmap_tbl, &portmap_key, (void **)&portmap_data);
if (DP_FAILED(ret))
return ret == -ENOENT ? DP_OK : ret;

portmap_data->flow_cnt--;
if (portmap_data->flow_cnt == 0) {
ret = rte_hash_del_key(ipv4_netnat_portmap_tbl, &portmap_key);
if (DP_FAILED(ret)) {
portmap_data->flow_cnt++;
DPS_LOG_ERR("Cannot delete portmap key", DP_LOG_RET(ret));
return DP_ERROR;
if (DP_SUCCESS(ret)) {
portmap_data->flow_cnt--;
if (portmap_data->flow_cnt == 0) {
ret = rte_hash_del_key(ipv4_netnat_portmap_tbl, &portmap_key);
if (DP_FAILED(ret)) {
portmap_data->flow_cnt++;
DPS_LOG_ERR("Cannot delete portmap key", DP_LOG_RET(ret));
return DP_ERROR;
}
rte_free(portmap_data);
}
rte_free(portmap_data);
} else {
DPS_LOG_ERR("Cannot lookup portmap key", DP_LOG_RET(ret));
if (ret != -ENOENT)
return ret;
// otherwise already deleted, finish
Copy link
Collaborator

@guvenc guvenc Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this control flow fall through is intentional now so that it can hit the INC macro at the end but this would also cause the new code to hit the lines below for ret == ENOENT case.

created_port = dp_get_port_by_id(cntrack->created_port_id);
if (!created_port)
return DP_ERROR;

Was this intentional ?

Copy link
Contributor Author

@PlagueCZ PlagueCZ Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is the commit that "fixes SNAT edge case" because previously it could have found a portmap entry (and immediately deleted it), but not found portoverload entry and then return without actually decreasing the counter.

}

created_port = dp_get_port_by_id(cntrack->created_port_id);
Expand Down
18 changes: 9 additions & 9 deletions src/nodes/snat_node.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,12 @@ static __rte_always_inline int dp_process_ipv4_snat(struct rte_mbuf *m, struct d
cntrack->nf_info.nat_type = DP_FLOW_NAT_TYPE_VIP;
}
if (snat_data->nat_ip != 0) {
ret = dp_allocate_network_snat_port(snat_data, df, port->iface.vni);
ret = dp_allocate_network_snat_port(snat_data, df, port);
if (DP_FAILED(ret))
return DP_ERROR;
nat_port = (uint16_t)ret;
ipv4_hdr->src_addr = htonl(snat_data->nat_ip);

DP_STATS_NAT_INC_USED_PORT_CNT(port);

if (df->l4_type == IPPROTO_ICMP) {
dp_change_icmp_identifier(m, nat_port);
cntrack->offload_state.orig = DP_FLOW_OFFLOADED;
Expand All @@ -69,8 +67,11 @@ static __rte_always_inline int dp_process_ipv4_snat(struct rte_mbuf *m, struct d
if (snat_data->nat_ip != 0)
cntrack->flow_key[DP_FLOW_DIR_REPLY].port_dst = df->nat_port;

if (DP_FAILED(dp_add_flow(&cntrack->flow_key[DP_FLOW_DIR_REPLY], cntrack)))
if (DP_FAILED(dp_add_flow(&cntrack->flow_key[DP_FLOW_DIR_REPLY], cntrack))) {
if (snat_data->nat_ip != 0)
dp_remove_network_snat_port(cntrack);
return DP_ERROR;
}
dp_ref_inc(&cntrack->ref_count);

return DP_OK;
Expand All @@ -88,19 +89,16 @@ static __rte_always_inline int dp_process_ipv6_nat64(struct rte_mbuf *m, struct
snat64_data.nat_ip = port->iface.nat_ip;
snat64_data.nat_port_range[0] = port->iface.nat_port_range[0];
snat64_data.nat_port_range[1] = port->iface.nat_port_range[1];
ret = dp_allocate_network_snat_port(&snat64_data, df, port->iface.vni);
ret = dp_allocate_network_snat_port(&snat64_data, df, port);
if (DP_FAILED(ret))
return DP_ERROR;
nat_port = (uint16_t)ret;

DP_STATS_NAT_INC_USED_PORT_CNT(port);

df->nat_port = nat_port;
df->nat_type = DP_NAT_64_CHG_SRC_IP;
df->nat_addr = snat64_data.nat_ip;
if (DP_FAILED(dp_nat_chg_ipv6_to_ipv4_hdr(df, m, snat64_data.nat_ip, &dest_ip4))) {
dp_remove_network_snat_port(cntrack);
DP_STATS_NAT_DEC_USED_PORT_CNT(port);
return DP_ERROR;
}

Expand Down Expand Up @@ -133,8 +131,10 @@ static __rte_always_inline int dp_process_ipv6_nat64(struct rte_mbuf *m, struct
cntrack->flow_key[DP_FLOW_DIR_REPLY].port_dst = df->nat_port;
cntrack->flow_key[DP_FLOW_DIR_REPLY].proto = df->l4_type;

if (DP_FAILED(dp_add_flow(&cntrack->flow_key[DP_FLOW_DIR_REPLY], cntrack)))
if (DP_FAILED(dp_add_flow(&cntrack->flow_key[DP_FLOW_DIR_REPLY], cntrack))) {
dp_remove_network_snat_port(cntrack);
return DP_ERROR;
}
dp_ref_inc(&cntrack->ref_count);

return DP_OK;
Expand Down
Loading