[ovs-dev,v3,3/3] conntrack: Disable algs by default.

Message ID 1512403987-112612-4-git-send-email-dlu998@gmail.com
State Accepted
Headers show
Series
  • conntrack: Alg improvements.
Related show

Commit Message

Darrell Ball Dec. 4, 2017, 4:13 p.m.
Presently, alg processing is enabled by default to better exercise code.
This is similar to kernels before 4.7 as well.  The recommended default
behavior in the newer kernels is to only process algs if a helper is
supplied in a conntrack rule.  The behavior is changed to match the
later kernels.

A test is extended to check that the control connection is still
created in such a case.

Signed-off-by: Darrell Ball <dlu998@gmail.com>
---
 lib/conntrack.c         | 32 +++++++++++++++++++++++++++-----
 tests/system-traffic.at | 21 +++++++++++++++++++++
 2 files changed, 48 insertions(+), 5 deletions(-)

Comments

Aaron Conole Dec. 6, 2017, 7:02 p.m. | #1
Darrell Ball <dlu998@gmail.com> writes:

> Presently, alg processing is enabled by default to better exercise code.
> This is similar to kernels before 4.7 as well.  The recommended default
> behavior in the newer kernels is to only process algs if a helper is
> supplied in a conntrack rule.  The behavior is changed to match the
> later kernels.
>
> A test is extended to check that the control connection is still
> created in such a case.
>
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> ---

LGTM.

Acked-by: Aaron Conole <aconole@redhat.com>
Darrell Ball Dec. 6, 2017, 7:10 p.m. | #2
Thanks for the reviews Aaron.

Darrell



On 12/6/17, 11:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Aaron Conole" <ovs-dev-bounces@openvswitch.org on behalf of aconole@redhat.com> wrote:

    Darrell Ball <dlu998@gmail.com> writes:
    
    > Presently, alg processing is enabled by default to better exercise code.
    > This is similar to kernels before 4.7 as well.  The recommended default
    > behavior in the newer kernels is to only process algs if a helper is
    > supplied in a conntrack rule.  The behavior is changed to match the
    > later kernels.
    >
    > A test is extended to check that the control connection is still
    > created in such a case.
    >
    > Signed-off-by: Darrell Ball <dlu998@gmail.com>
    > ---
    
    LGTM.
    
    Acked-by: Aaron Conole <aconole@redhat.com>
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=D7-nKQ8low4ohzl_EtbVC_dQ1XBd0pg8gTmTzeYVte0&s=o95-s6j6eDyJTjLmhlMjK8t_qwOO439Wigl_cgMNv0Q&e=
Ben Pfaff Dec. 11, 2017, 10:17 p.m. | #3
Thanks, Darrell and Aaron.  I applied this series to master.

On Wed, Dec 06, 2017 at 07:10:03PM +0000, Darrell Ball wrote:
> Thanks for the reviews Aaron.
> 
> Darrell
> 
> 
> 
> On 12/6/17, 11:04 AM, "ovs-dev-bounces@openvswitch.org on behalf of Aaron Conole" <ovs-dev-bounces@openvswitch.org on behalf of aconole@redhat.com> wrote:
> 
>     Darrell Ball <dlu998@gmail.com> writes:
>     
>     > Presently, alg processing is enabled by default to better exercise code.
>     > This is similar to kernels before 4.7 as well.  The recommended default
>     > behavior in the newer kernels is to only process algs if a helper is
>     > supplied in a conntrack rule.  The behavior is changed to match the
>     > later kernels.
>     >
>     > A test is extended to check that the control connection is still
>     > created in such a case.
>     >
>     > Signed-off-by: Darrell Ball <dlu998@gmail.com>
>     > ---
>     
>     LGTM.
>     
>     Acked-by: Aaron Conole <aconole@redhat.com>
>     _______________________________________________
>     dev mailing list
>     dev@openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=D7-nKQ8low4ohzl_EtbVC_dQ1XBd0pg8gTmTzeYVte0&s=o95-s6j6eDyJTjLmhlMjK8t_qwOO439Wigl_cgMNv0Q&e=
>     
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 61f3a79..cd54ba7 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -505,7 +505,7 @@  handle_alg_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
                const struct conn *conn_for_expectation)
 {
     /* ALG control packet handling with expectation creation. */
-    if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
+    if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
         alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
                                 CT_FTP_CTL_INTEREST, nat);
     }
@@ -819,6 +819,26 @@  conn_clean(struct conntrack *ct, struct conn *conn,
     }
 }
 
+static bool
+ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
+{
+    if (ct_alg_ctl == CT_ALG_CTL_NONE) {
+        return true;
+    } else if (helper) {
+        if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
+             !strncmp(helper, "ftp", strlen("ftp"))) {
+            return true;
+        } else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
+                   !strncmp(helper, "tftp", strlen("tftp"))) {
+            return true;
+        } else {
+            return false;
+        }
+    } else {
+        return false;
+    }
+}
+
 /* This function is called with the bucket lock held. */
 static struct conn *
 conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
@@ -826,7 +846,8 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
                const struct nat_action_info_t *nat_action_info,
                struct conn *conn_for_un_nat_copy,
                const char *helper,
-               const struct alg_exp_node *alg_exp)
+               const struct alg_exp_node *alg_exp,
+               enum ct_alg_ctl_type ct_alg_ctl)
 {
     unsigned bucket = hash_to_bucket(ctx->hash);
     struct conn *nc = NULL;
@@ -855,8 +876,8 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
         nc->rev_key = nc->key;
         conn_key_reverse(&nc->rev_key);
 
-        if (helper) {
-            nc->alg = xstrdup(helper);
+        if (ct_verify_helper(helper, ct_alg_ctl)) {
+            nc->alg = nullable_xstrdup(helper);
         }
 
         if (alg_exp) {
@@ -1238,7 +1259,8 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
         ct_rwlock_unlock(&ct->resources_lock);
 
         conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
-                              &conn_for_un_nat_copy, helper, alg_exp);
+                              &conn_for_un_nat_copy, helper, alg_exp,
+                              ct_alg_ctl);
     }
 
     write_ct_md(pkt, zone, conn, &ctx->key, alg_exp);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index fd7b612..4551c5c 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2400,6 +2400,17 @@  table=1,in_port=2,tcp,ct_state=+trk+est,action=1
 table=1,in_port=2,tcp,ct_state=+trk-new+rel,action=1
 ])
 
+dnl flows3 is same as flows1, except no ALG is specified.
+AT_DATA([flows3.txt], [dnl
+table=0,priority=1,action=drop
+table=0,priority=10,arp,action=normal
+table=0,priority=10,icmp,action=normal
+table=0,priority=100,in_port=1,tcp,action=ct(commit),2
+table=0,priority=100,in_port=2,tcp,action=ct(table=1)
+table=1,in_port=2,tcp,ct_state=+trk+est,action=1
+table=1,in_port=2,tcp,ct_state=+trk+rel,action=1
+])
+
 AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows1.txt])
 
 OVS_START_L7([at_ns0], [ftp])
@@ -2442,6 +2453,16 @@  AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
 tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>),helper=ftp
 ])
 
+dnl Try the third set of flows, without alg specifier.
+AT_CHECK([ovs-ofctl --bundle replace-flows br0 flows3.txt])
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
+dnl FTP control requests from p0->p1 should work fine, but helper will not be assigned.
+NS_CHECK_EXEC([at_ns0], [wget ftp://10.1.1.2 --no-passive-ftp -t 3 -T 1 --retry-connrefused -v -o wget0-3.log], [4])
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
+])
+
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP