| Message ID | 1543250920-115500-3-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:39AM -0800, Darrell Ball wrote: > The user should only reference a conntrack entry by the forward > direction context, as per 'conntrack_flush()', enforce this by > checking for 'default' conn_type. The likelihood of a user > not using the original tuple is low, but it should be guarded > against, logged and documented. > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > --- > > Backport to 2.9. > > v3: Move backport hint out of commit message. > Remove warning log conditional for now. Would it be more user-friendly to translate these into the forward equivalent and flush that one?
On Mon, Dec 10, 2018 at 5:42 PM Ben Pfaff <blp@ovn.org> wrote: > On Mon, Nov 26, 2018 at 08:48:39AM -0800, Darrell Ball wrote: > > The user should only reference a conntrack entry by the forward > > direction context, as per 'conntrack_flush()', enforce this by > > checking for 'default' conn_type. The likelihood of a user > > not using the original tuple is low, but it should be guarded > > against, logged and documented. > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > --- > > > > Backport to 2.9. > > > > v3: Move backport hint out of commit message. > > Remove warning log conditional for now. > > Would it be more user-friendly to translate these into the forward > equivalent and flush that one? > If there were a practical application to try to flush a tuple using the dynamic/random NAT tuple assignment, yes; but there is not and it is also not worth the added complexity to handle the race b/w buckets for this purpose.
On Mon, Dec 10, 2018 at 07:26:32PM -0800, Darrell Ball wrote: > On Mon, Dec 10, 2018 at 5:42 PM Ben Pfaff <blp@ovn.org> wrote: > > > On Mon, Nov 26, 2018 at 08:48:39AM -0800, Darrell Ball wrote: > > > The user should only reference a conntrack entry by the forward > > > direction context, as per 'conntrack_flush()', enforce this by > > > checking for 'default' conn_type. The likelihood of a user > > > not using the original tuple is low, but it should be guarded > > > against, logged and documented. > > > > > > Signed-off-by: Darrell Ball <dlu998@gmail.com> > > > --- > > > > > > Backport to 2.9. > > > > > > v3: Move backport hint out of commit message. > > > Remove warning log conditional for now. > > > > Would it be more user-friendly to translate these into the forward > > equivalent and flush that one? > > > > If there were a practical application to try to flush a tuple using the > dynamic/random > NAT tuple assignment, yes; but there is not and it is also not worth the > added complexity > to handle the race b/w buckets for this purpose. OK.
diff --git a/lib/conntrack.c b/lib/conntrack.c index bb3eeca..f6ff90a 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2572,9 +2572,10 @@ conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple, ct_lock_lock(&ct->buckets[bucket].lock); conn_key_lookup(&ct->buckets[bucket], &ctx, time_msec()); - if (ctx.conn) { + if (ctx.conn && ctx.conn->conn_type == CT_CONN_TYPE_DEFAULT) { conn_clean(ct, ctx.conn, &ct->buckets[bucket]); } else { + VLOG_WARN("Must flush tuple using the original pre-NATed tuple"); error = ENOENT; } ct_lock_unlock(&ct->buckets[bucket].lock); diff --git a/lib/dpctl.man b/lib/dpctl.man index 9b13e0d..fe0aec9 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -237,6 +237,8 @@ If \fBzone\fR=\fIzone\fR is specified, only flushes the connections in .IP If \fIct-tuple\fR is provided, flushes the connection entry specified by \fIct-tuple\fR in \fIzone\fR. The zone defaults to 0 if it is not provided. +The userspace connection tracker requires flushing with the original pre-NATed +tuple and a warning log will be otherwise generated. An example of an IPv4 ICMP \fIct-tuple\fR: .IP "ct_nw_src=10.1.1.1,ct_nw_dst=10.1.1.2,ct_nw_proto=1,icmp_type=8,icmp_code=0,icmp_id=10"
The user should only reference a conntrack entry by the forward direction context, as per 'conntrack_flush()', enforce this by checking for 'default' conn_type. The likelihood of a user not using the original tuple is low, but it should be guarded against, logged and documented. Signed-off-by: Darrell Ball <dlu998@gmail.com> --- Backport to 2.9. v3: Move backport hint out of commit message. Remove warning log conditional for now. lib/conntrack.c | 3 ++- lib/dpctl.man | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-)