| Message ID | 1543250920-115500-1-git-send-email-dlu998@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3,1/4] conntrack: Skip ephemeral ports fallback for DNAT. | expand |
On Mon, Nov 26, 2018 at 08:48:37AM -0800, Darrell Ball wrote: > Ephemeral port fallback is being done for DNAT and the code could be hit in > some special cases and testing configurations. Also good packets are > expected to be persistently dropped in this case, which is not a common > user goal. Regardless, this is incorrect, so filter this out. Also, rename > the variable used for checking whether ephemeral ports need to be checked. > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/351629.html > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") > Signed-off-by: Darrell Ball <dlu998@gmail.com> Does the following change actually have a behavioral difference? I see that there's a renaming but the code flow change looks to me like it would have the same behavior before and after. If so, could you please just leave the code the same? > - if (!original_ports_tried) { > - original_ports_tried = true; > + if (ephemeral_ports_tried) { > + break; > + } else { > + ephemeral_ports_tried = true; > ct_addr = conn->nat_info->min_addr; > min_port = MIN_NAT_EPHEMERAL_PORT; > max_port = MAX_NAT_EPHEMERAL_PORT; > - } else { > - break; > } Thanks, Ben.
On Mon, Dec 10, 2018 at 3:53 PM Ben Pfaff <blp@ovn.org> wrote: > On Mon, Nov 26, 2018 at 08:48:37AM -0800, Darrell Ball wrote: > > Ephemeral port fallback is being done for DNAT and the code could be hit > in > > some special cases and testing configurations. Also good packets are > > expected to be persistently dropped in this case, which is not a common > > user goal. Regardless, this is incorrect, so filter this out. Also, > rename > > the variable used for checking whether ephemeral ports need to be > checked. > > > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/351629.html > > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > Does the following change actually have a behavioral difference? I see > that there's a renaming but the code flow change looks to me like it > would have the same behavior before and after. If so, could you please > just leave the code the same? > The switch to positive sense for the selection statement is purely cosmetic; I don't mind omitting it. > > > - if (!original_ports_tried) { > > - original_ports_tried = true; > > + if (ephemeral_ports_tried) { > > + break; > > + } else { > > + ephemeral_ports_tried = true; > > ct_addr = conn->nat_info->min_addr; > > min_port = MIN_NAT_EPHEMERAL_PORT; > > max_port = MAX_NAT_EPHEMERAL_PORT; > > - } else { > > - break; > > } > > Thanks, > > Ben. >
diff --git a/lib/conntrack.c b/lib/conntrack.c index 974f985..31fedc0 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2172,7 +2172,9 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, uint16_t port = first_port; bool all_ports_tried = false; - bool original_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; struct ct_addr first_addr = ct_addr; while (true) { @@ -2218,13 +2220,13 @@ nat_select_range_tuple(struct conntrack *ct, const struct conn *conn, ct_addr = conn->nat_info->min_addr; } if (!memcmp(&ct_addr, &first_addr, sizeof ct_addr)) { - if (!original_ports_tried) { - original_ports_tried = true; + if (ephemeral_ports_tried) { + break; + } else { + ephemeral_ports_tried = true; ct_addr = conn->nat_info->min_addr; min_port = MIN_NAT_EPHEMERAL_PORT; max_port = MAX_NAT_EPHEMERAL_PORT; - } else { - break; } } first_port = min_port;
Ephemeral port fallback is being done for DNAT and the code could be hit in some special cases and testing configurations. Also good packets are expected to be persistently dropped in this case, which is not a common user goal. Regardless, this is incorrect, so filter this out. Also, rename the variable used for checking whether ephemeral ports need to be checked. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/351629.html Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") Signed-off-by: Darrell Ball <dlu998@gmail.com> --- Backport to 2.8. v3: Move backport hint out of commit message. lib/conntrack.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)