Eli Britstein <[email protected]> writes:
> OVS conntrack NAT has three distinct behaviors at commit time,
> depending on the ct(commit,nat(...)) nest. The kernel has implemented
> this since [1]:
>
> ct(commit,nat) no SRC/DST -> no bind on NEW flows
> ct(commit,nat(src)) direction -> null binding (OVN marker).
> Remap ports on reverse-tuple
> collision only)
> ct(commit,nat(dst)) direction -> null binding
> ct(commit,nat(src|dst=...)) IP/port -> full bind (nf_nat_setup_info)
Looks like 4 behaviors but your commit claims 3.
> Prior userspace had only one path at commit (conn_not_found): any
> ct(commit,nat(...)) nest took the full-bind path. That ignored no-bind
> and null-binding cases, and treated all-zero ranges as full binds.
Userspace supports null bind since 61e48c2d1db2 ("conntrack: Handle SNAT
with all-zero IP address."). So this isn't correct.
Also this patch does a few things all at once, so it is difficult to
consider it properly. For example, you introduce the
nat_log_tuple_exhaustion call and call it in one place. But that is a
change all on its own independent of the rest of the changes.
This patch also needs quite a bit of documentation. See
Documentation/ref/ovs-actions.7.rst - we need to make sure we adequately
distinguish between no bind, null bind, and full bind cases. Maybe
Paolo has some opinions on a capabilities bit (like we did for
ZERO_IP_SNAT) so that OVN deployments know that this exists.
> [1]
> https://github.com/torvalds/linux/blob/master/net/netfilter/nf_conntrack_core.c
Should point to a commit ID if you will say "since"
> Signed-off-by: Eli Britstein <[email protected]>
> ---
> lib/conntrack.c | 141 ++++++++++++++++++++++++++++++++++++----
> tests/system-traffic.at | 129 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 256 insertions(+), 14 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 168954c35..2ea70dd7d 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -120,6 +120,16 @@ static bool
> nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
> const struct nat_action_info_t *nat_info);
>
> +static bool
> +nat_null_binding(struct conntrack *ct, struct conn *conn,
> + const struct nat_action_info_t *nat_info);
> +
> +static bool
> +nat_has_explicit_range(const struct nat_action_info_t *nat, ovs_be16
> dl_type);
> +
> +static bool
> +nat_has_direction(const struct nat_action_info_t *nat);
> +
> static uint8_t
> reverse_icmp_type(uint8_t type);
> static uint8_t
> @@ -1021,6 +1031,48 @@ ct_verify_helper(const char *helper, enum
> ct_alg_ctl_type ct_alg_ctl)
> }
> }
>
> +/* True when NAT defines an explicit IP/port range (vs direction-only).
> + * All-zero min with no distinct max is direction-only, not an explicit
> + * range. */
> +static bool
> +nat_has_explicit_range(const struct nat_action_info_t *nat, ovs_be16 dl_type)
This isn't really needed - we already check for an explicit range when
doing the min check:
case OVS_NAT_ATTR_IP_MIN:
memcpy(&nat_action_info.min_addr,
nl_attr_get(b_nest),
nl_attr_get_size(b_nest));
ip_min_specified = true;
break;
case OVS_NAT_ATTR_IP_MAX:
memcpy(&nat_action_info.max_addr,
nl_attr_get(b_nest),
nl_attr_get_size(b_nest));
ip_max_specified = true;
break;
Probably just better to use a flag (this is also the way kernel does it)
and then drop most of this function into just a bit test. We have the
min/max specified bools locally in dpif-netdev.c so we can just expose
something that says the range is included.
> +{
> + if (!nat) {
> + return false;
> + }
> +
> + if (nat->min_port || nat->max_port) {
> + return true;
> + }
> +
> + if (dl_type == htons(ETH_TYPE_IP)) {
> + return nat->min_addr.ipv4 != 0
> + || nat->max_addr.ipv4 != nat->min_addr.ipv4;
> + } else if (dl_type == htons(ETH_TYPE_IPV6)) {
> + return !ipv6_mask_is_any(&nat->min_addr.ipv6)
> + || (!ipv6_mask_is_any(&nat->max_addr.ipv6)
> + && memcmp(&nat->max_addr.ipv6, &nat->min_addr.ipv6,
> + sizeof nat->max_addr.ipv6));
> + }
> +
> + return false;
> +}
> +
> +static bool
> +nat_has_direction(const struct nat_action_info_t *nat)
nat_action can also carry port bits - today they are only filled when
SRC|DST actions are specified. But if a future bit comes along that
gets stuffed into there, it will be wrong. Should mask with
(NAT_ACTION_SRC | NAT_ACTION_DST)
> +{
> + return nat && nat->nat_action;
> +}
> +
> +static void
> +nat_log_tuple_exhaustion(void)
> +{
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> +
> + VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - "
> + "if DoS attack, use firewalling and/or zone partitioning.");
> +}
> +
> static struct conn *
> conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> @@ -1095,8 +1147,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> }
>
> if (nat_action_info) {
> - nc->nat_action = nat_action_info->nat_action;
> -
> if (alg_exp) {
> if (alg_exp->nat_rpl_dst) {
> rev_key_node->key.dst.addr = alg_exp->alg_nat_repl_addr;
> @@ -1105,18 +1155,28 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> rev_key_node->key.src.addr = alg_exp->alg_nat_repl_addr;
> nc->nat_action = NAT_ACTION_DST;
> }
> - } else {
> - bool nat_res = nat_get_unique_tuple(ct, nc, nat_action_info);
> - if (!nat_res) {
> + } else if (nat_has_explicit_range(nat_action_info,
> + fwd_key_node->key.dl_type)
> + && nat_has_direction(nat_action_info)) {
> + nc->nat_action = nat_action_info->nat_action;
> + if (!nat_get_unique_tuple(ct, nc, nat_action_info)) {
> + goto nat_res_exhaustion;
> + }
> + } else if (nat_has_direction(nat_action_info)) {
> + nc->nat_action |= nat_action_info->nat_action
> + & (NAT_ACTION_SRC | NAT_ACTION_DST);
> + if (!nat_null_binding(ct, nc, nat_action_info)) {
These two different cases of resource exhaustion might need something
to distinguish them for a user.
> goto nat_res_exhaustion;
> }
> }
>
> - nat_packet(pkt, nc, false, ctx->icmp_related);
> - uint32_t rev_hash = conn_key_hash(&rev_key_node->key,
> - ct->hash_basis);
> - cmap_insert(&ct->conns[ctx->key.zone],
> - &rev_key_node->cm_node, rev_hash);
> + if (nc->nat_action) {
> + nat_packet(pkt, nc, false, ctx->icmp_related);
> + uint32_t rev_hash = conn_key_hash(&rev_key_node->key,
> + ct->hash_basis);
> + cmap_insert(&ct->conns[ctx->key.zone],
> + &rev_key_node->cm_node, rev_hash);
> + }
> }
>
> cmap_insert(&ct->conns[ctx->key.zone],
> @@ -1140,9 +1200,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet
> *pkt,
> * can limit DoS impact. */
> nat_res_exhaustion:
> delete_conn__(nc);
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> - VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - "
> - "if DoS attack, use firewalling and/or zone partitioning.");
> + nat_log_tuple_exhaustion();
> return NULL;
> }
>
> @@ -1229,7 +1287,7 @@ check_orig_tuple(struct conntrack *ct, struct dp_packet
> *pkt,
> !pkt->md.ct_orig_tuple.ipv4.ipv4_proto) ||
> (ctx_in->key.dl_type == htons(ETH_TYPE_IPV6) &&
> !pkt->md.ct_orig_tuple.ipv6.ipv6_proto) ||
> - nat_action_info) {
> + nat_has_explicit_range(nat_action_info, ctx_in->key.dl_type)) {
> return false;
> }
>
> @@ -2579,6 +2637,61 @@ another_round:
> return false;
> }
>
> +/* Returns true if no remapping is needed or remapping succeeded.
> + * Returns false if a reverse-tuple collision was detected but a unique
> + * L4 port could not be allocated (tuple-space exhaustion). */
> +static bool
> +nat_null_binding(struct conntrack *ct, struct conn *conn,
> + const struct nat_action_info_t *nat_info)
> +{
This is far too similar to nat_get_unique_tuple(). I think it isn't
needed.
> + struct conn_key *fwd_key = &conn->key_node[CT_DIR_FWD].key;
> + struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key;
> + bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
> + fwd_key->nw_proto == IPPROTO_UDP ||
> + fwd_key->nw_proto == IPPROTO_SCTP ||
> + fwd_key->nw_proto == IPPROTO_ICMP;
> + uint16_t min_sport, max_sport, curr_sport;
> +
> + if (!pat_proto) {
> + return true;
> + }
> +
> + /* Remap ports only when the reverse tuple collides with an existing
> + * connection. */
> + {
> + struct conn *collision = NULL;
> +
> + if (!conn_lookup(ct, rev_key, time_msec(), &collision, NULL)) {
> + return true;
> + }
> +
> + if (collision == conn) {
> + return true;
> + }
> + }
> +
> + if (nat_info->nat_action & (NAT_ACTION_SRC | NAT_ACTION_DST)) {
> + uint16_t direction = nat_info->nat_action
> + & (NAT_ACTION_SRC | NAT_ACTION_DST);
> +
> + conn->nat_action |= direction;
> +
> + set_sport_range(nat_info, fwd_key, 0, &curr_sport,
> + &min_sport, &max_sport);
> + if (!nat_get_unique_l4(ct, rev_key, &rev_key->dst.port,
> + rev_key->nw_proto == IPPROTO_ICMP
> + ? &rev_key->src.port : NULL,
> + curr_sport, min_sport, max_sport)) {
> + return false;
> + }
> +
> + conn->nat_action |= NAT_ACTION_SRC_PORT;
> + return true;
> + }
> +
> + return true;
> +}
> +
> /* This function tries to get a unique tuple.
> * Every iteration checks that the reverse tuple doesn't
> * collide with any existing one.
NOTE: I didn't evaluate the tests, but thanks for including them with
compose-packet rather than some packet bytes.
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 4ad51223d..b34da49f9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -4719,6 +4719,135 @@ n_packets=0
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([conntrack - empty nat on first ct(commit)])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=1,udp,actions=ct(commit,nat,table=2)
> +table=2,priority=0,actions=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
> +flow_l3="eth_src=50:54:00:00:00:09,eth_dst=50:54:00:00:00:0a,dl_type=0x0800,nw_src=10.1.1.1,nw_dst=10.1.1.2,nw_proto=17,nw_ttl=64,nw_frag=no"
> +
> +AT_CHECK([syn_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> udp_src=12345,udp_dst=5201"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${syn_pkt},actions=resubmit(,0)"])
> +
> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack | grep -q
> "orig=.src=10\.1\.1\.1,"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"],
> [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=12345,dport=5201),reply=(src=10.1.1.2,dst=10.1.1.1,sport=5201,dport=12345)
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([conntrack - nat(src) null binding collision])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=1,udp,actions=ct(table=1,nat)
> +table=1,cookie=0x1,priority=200,udp,nw_dst=172.1.1.2,tp_dst=80,ct_state=+new+trk,actions=ct(commit,nat(dst=10.1.1.2:80),table=2)
> +table=1,cookie=0x2,priority=200,udp,nw_dst=10.1.1.2,tp_dst=80,ct_state=+new+trk,actions=ct(commit,nat(src),table=2)
> +table=1,priority=0,actions=drop
> +table=2,priority=0,actions=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
> +flow_l3="eth_src=50:54:00:00:00:09,eth_dst=50:54:00:00:00:0a,dl_type=0x0800,nw_src=10.1.1.1,nw_proto=17,nw_ttl=64,nw_frag=no"
> +
> +AT_CHECK([dnat_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=172.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${dnat_pkt},actions=resubmit(,0)"])
> +
> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack | grep -q
> "orig=.src=10\.1\.1\.1,dst=172\.1\.1\.2,"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep
> "orig=.src=10\.1\.1\.1,dst=172\.1\.1\.2,"], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=30001,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=30001)
> +])
> +
> +AT_CHECK([plain_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=10.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${plain_pkt},actions=resubmit(,0)"])
> +
> +AT_CHECK([sh -c 'line=$(ovs-appctl dpctl/dump-conntrack | grep
> "orig=(src=10.1.1.1,dst=10.1.1.2,sport=30001,dport=80)"); test -n "$line";
> echo "$line" | grep -q "reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=";
> echo "$line" | grep -vq ",dport=30001)"'])
> +
> +AT_CHECK([plain_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=10.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> +
> "in_port=1,packet=${plain_pkt},actions=ct(commit,zone=1,nat,table=2)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack zone=1 | grep
> "orig=.src=10\.1\.1\.1,"], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=30001,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=30001),zone=1
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([conntrack - nat(dst) null binding collision])
> +CHECK_CONNTRACK()
> +CHECK_CONNTRACK_NAT()
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +ADD_NAMESPACES(at_ns0)
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +
> +AT_DATA([flows.txt], [dnl
> +table=0,priority=100,in_port=1,udp,actions=ct(table=1,nat)
> +table=1,cookie=0x1,priority=200,udp,nw_dst=172.1.1.2,tp_dst=80,ct_state=+new+trk,actions=ct(commit,nat(dst=10.1.1.2:80),table=2)
> +table=1,cookie=0x2,priority=200,udp,nw_dst=10.1.1.2,tp_dst=80,ct_state=+new+trk,actions=ct(commit,nat(dst),table=2)
> +table=1,priority=0,actions=drop
> +table=2,priority=0,actions=drop
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
> +flow_l3="eth_src=50:54:00:00:00:09,eth_dst=50:54:00:00:00:0a,dl_type=0x0800,nw_src=10.1.1.1,nw_proto=17,nw_ttl=64,nw_frag=no"
> +
> +AT_CHECK([dnat_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=172.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${dnat_pkt},actions=resubmit(,0)"])
> +
> +OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-conntrack | grep -q
> "orig=.src=10\.1\.1\.1,dst=172\.1\.1\.2,"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep
> "orig=.src=10\.1\.1\.1,dst=172\.1\.1\.2,"], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=30001,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=30001)
> +])
> +
> +AT_CHECK([plain_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=10.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> + "in_port=1,packet=${plain_pkt},actions=resubmit(,0)"])
> +
> +AT_CHECK([sh -c 'line=$(ovs-appctl dpctl/dump-conntrack | grep
> "orig=(src=10.1.1.1,dst=10.1.1.2,sport=30001,dport=80)"); test -n "$line";
> echo "$line" | grep -q "reply=(src=.*,dst=10.1.1.1,sport=80,dport="; echo
> "$line" | grep -vq ",dport=30001)"'])
> +
> +AT_CHECK([plain_pkt=$(ovs-ofctl compose-packet --bare "$flow_l3,
> nw_dst=10.1.1.2, udp_src=30001,udp_dst=80"); dnl
> + ovs-ofctl -O OpenFlow13 packet-out br0 dnl
> +
> "in_port=1,packet=${plain_pkt},actions=ct(commit,zone=1,nat,table=2)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack zone=1 | grep
> "orig=.src=10\.1\.1\.1,"], [0], [dnl
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=30001,dport=80),reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=30001),zone=1
> +])
> +
> +dnl The kernel datapath rejects bare ct commit with nat dst and EINVAL.
> +OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> +/execute ct.*Invalid argument/d"])
> +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
