| Message ID | 1ac49446-e0a2-e25f-82dd-6372e96d8cff@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] ovs: skip ephemeral port for icmp and SNAT in which port range is specified | expand |
Bleep bloop. Greetings solomon, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author solomon <liwei.solomon@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: LiWei <liwei.solomon@gmail.com> Lines checked: 70, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
Thanks for the report I think there are two separate issues: 1/ Fallback to ephemeral ports for SNAT being less restrictive than in kernel 2/ Wasted local variable port incrementing work for ICMPv4/v6 I sent an alternative series here: https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356654.html Darrell On Mon, Feb 25, 2019 at 7:03 AM solomon <liwei.solomon@gmail.com> wrote: > > If setting the port range in SNAT, we expect that the selected port is in > the range we set. > At the same time, this behavior is consistent with the kernel-datapath. > > The port has no meaning for the icmp/icmpv6 protocol. > If no key is available, it will exit early. > > Signed-off-by: LiWei <liwei.solomon@gmail.com> > --- > lib/conntrack.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 4028ba9a0..c3fd7d63d 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -2115,6 +2115,12 @@ nat_range_hash(const struct conn *conn, uint32_t > basis) > return hash_finish(hash, 0); > } > > +/* This function selects a unique new connection key based on the ip and > port > + * settings in the nat tuple. For the case where the port range is not > + * specified, the ephemeral port from 1024 to 65535 can be selected when > the > + * address is exhausted. > + * However, the ephemeral ports are not enabled by ICMP/ICMPv6 and DNAT. > + */ > static bool > nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, > struct conn *nat_conn) > @@ -2126,12 +2132,14 @@ nat_select_range_tuple(struct conntrack *ct, const > struct conn *conn, > uint16_t max_port; > uint16_t first_port; > uint32_t hash = nat_range_hash(conn, ct->hash_basis); > + bool ephemeral_ports_tried = true; > > if ((conn->nat_info->nat_action & NAT_ACTION_SRC) && > (!(conn->nat_info->nat_action & NAT_ACTION_SRC_PORT))) { > min_port = ntohs(conn->key.src.port); > max_port = ntohs(conn->key.src.port); > first_port = min_port; > + ephemeral_ports_tried = false; > } else if ((conn->nat_info->nat_action & NAT_ACTION_DST) && > (!(conn->nat_info->nat_action & NAT_ACTION_DST_PORT))) { > min_port = ntohs(conn->key.dst.port); > @@ -2175,9 +2183,6 @@ nat_select_range_tuple(struct conntrack *ct, const > struct conn *conn, > > uint16_t port = first_port; > bool all_ports_tried = false; > - /* For DNAT, we don't use ephemeral ports. */ > - bool ephemeral_ports_tried = conn->nat_info->nat_action & > NAT_ACTION_DST > - ? true : false; > union ct_addr first_addr = ct_addr; > > while (true) { > @@ -2190,6 +2195,7 @@ nat_select_range_tuple(struct conntrack *ct, const > struct conn *conn, > if ((conn->key.nw_proto == IPPROTO_ICMP) || > (conn->key.nw_proto == IPPROTO_ICMPV6)) { > all_ports_tried = true; > + ephemeral_ports_tried = true; > } else if (conn->nat_info->nat_action & NAT_ACTION_SRC) { > nat_conn->rev_key.dst.port = htons(port); > } else { > -- > 2.11.0 >
Darrell Ball wrote: > Thanks for the report > > I think there are two separate issues: > 1/ Fallback to ephemeral ports for SNAT being less restrictive than in > kernel > 2/ Wasted local variable port incrementing work for ICMPv4/v6 > > I sent an alternative series here: > https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356654.html > Thanks for your quick ack. I have seen the accepted patch-set in the upstream.
diff --git a/lib/conntrack.c b/lib/conntrack.c index 4028ba9a0..c3fd7d63d 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2115,6 +2115,12 @@ nat_range_hash(const struct conn *conn, uint32_t basis) return hash_finish(hash, 0); } +/* This function selects a unique new connection key based on the ip and port + * settings in the nat tuple. For the case where the port range is not + * specified, the ephemeral port from 1024 to 65535 can be selected when the + * address is exhausted. + * However, the ephemeral ports are not enabled by ICMP/ICMPv6 and DNAT. + */ static bool nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, struct conn *nat_conn) @@ -2126,12 +2132,14 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, uint16_t max_port; uint16_t first_port; uint32_t hash = nat_range_hash(conn, ct->hash_basis); + bool ephemeral_ports_tried = true; if ((conn->nat_info->nat_action & NAT_ACTION_SRC) && (!(conn->nat_info->nat_action & NAT_ACTION_SRC_PORT))) { min_port = ntohs(conn->key.src.port); max_port = ntohs(conn->key.src.port); first_port = min_port; + ephemeral_ports_tried = false; } else if ((conn->nat_info->nat_action & NAT_ACTION_DST) && (!(conn->nat_info->nat_action & NAT_ACTION_DST_PORT))) { min_port = ntohs(conn->key.dst.port); @@ -2175,9 +2183,6 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, uint16_t port = first_port; bool all_ports_tried = false; - /* For DNAT, we don't use ephemeral ports. */ - bool ephemeral_ports_tried = conn->nat_info->nat_action & NAT_ACTION_DST - ? true : false; union ct_addr first_addr = ct_addr; while (true) { @@ -2190,6 +2195,7 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, if ((conn->key.nw_proto == IPPROTO_ICMP) || (conn->key.nw_proto == IPPROTO_ICMPV6)) { all_ports_tried = true; + ephemeral_ports_tried = true; } else if (conn->nat_info->nat_action & NAT_ACTION_SRC) { nat_conn->rev_key.dst.port = htons(port); } else {
If setting the port range in SNAT, we expect that the selected port is in the range we set. At the same time, this behavior is consistent with the kernel-datapath. The port has no meaning for the icmp/icmpv6 protocol. If no key is available, it will exit early. Signed-off-by: LiWei <liwei.solomon@gmail.com> --- lib/conntrack.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)