[ovs-dev] [patch_v9 2/6] Userspace Datapath: Add ALG infra and FTP.

Darrell Ball dball at vmware.com
Sat Aug 5 14:41:23 UTC 2017


One update inline.

-----Original Message-----
From: <ovs-dev-bounces at openvswitch.org> on behalf of Darrell Ball <dball at vmware.com>
Date: Friday, August 4, 2017 at 8:14 PM
To: Ben Pfaff <blp at ovn.org>, Darrell Ball <dlu998 at gmail.com>
Cc: "dev at openvswitch.org" <dev at openvswitch.org>
Subject: Re: [ovs-dev] [patch_v9 2/6] Userspace Datapath: Add ALG infra and FTP.

    
    
    
    
    -----Original Message-----
    
    From: <ovs-dev-bounces at openvswitch.org> on behalf of Ben Pfaff <blp at ovn.org>
    
    Date: Friday, August 4, 2017 at 2:51 PM
    
    To: Darrell Ball <dlu998 at gmail.com>
    
    Cc: "dev at openvswitch.org" <dev at openvswitch.org>
    
    Subject: Re: [ovs-dev] [patch_v9 2/6] Userspace Datapath: Add ALG infra and FTP.
    
    
    
        On Thu, Aug 03, 2017 at 09:34:42PM -0700, Darrell Ball wrote:
    
        > ALG infra and FTP (both V4 and V6) support is added to the userspace
    
        > datapath.  Also, NAT support is included.
    
        > 
    
        > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    
        
    
        Thanks for implementing this.  I have only a few minor comments.
    
        
    
        struct conn_key has at least one hole in it, between 'nw_proto' and
    
        'zone'.  That makes it risky, at best, to do byte-by-byte comparisons of
    
        two instances of it with memcmp(), but expectation_lookup() does such a
    
        comparison.  It would probably be better to do a member-by-member
    
        comparison, or to carefully rearrange struct conn_key to avoid holes.
    
    
    
    I am aware of these, but care has been taken to always zero initialize the
    
    struct on allocation.
    
    This memcmp for conn_key is day 1 and I have some bigger structure
    
    changes and adjustments/optimizations on my todo list; I’d prefer to do
    
    that later, if that ok ?

[Darrell]
An update on this.
I have gone thru. and fixed any potential risks here now – thanks for pointing
this out. 

//////////////////////////

    
    
    
        It doesn't make sense to me to have a strcasestr_s() prototype in
    
        conntrack.c.  I think it can be removed.
    
    
    
    agreed, I responded in patch 1
    
        
    
        As a possible direction for the future, usually the need to read-lock a
    
        data structure can be eliminated by using RCU.
    
    
    
    I have some improvements along these lines.
    
        
    
        I'm appending some minor suggestions that help to better conform to the
    
        usual OVS style or that made the code easier for me to read and
    
        understand.
    
    
    
    Thanks very much; I will fold in.
    
    
    
        
    
        --8<--------------------------cut here-------------------------->8--
    
        
    
        diff --git a/lib/conntrack.c b/lib/conntrack.c
    
        index be7d0623b24f..a05c54019bc9 100644
    
        --- a/lib/conntrack.c
    
        +++ b/lib/conntrack.c
    
        @@ -66,8 +66,6 @@ enum ct_alg_mode {
    
             CT_FTP_MODE_PASSIVE,
    
         };
    
         
    
        -char *strcasestr_s(const char *str, const char *substr);
    
        -
    
         static bool conn_key_extract(struct conntrack *, struct dp_packet *,
    
                                      ovs_be16 dl_type, struct conn_lookup_ctx *,
    
                                      uint16_t zone);
    
        @@ -333,7 +331,6 @@ write_ct_md(struct dp_packet *pkt, uint16_t zone, const struct conn *conn,
    
         static uint8_t
    
         get_ip_proto(const struct dp_packet *pkt)
    
         {
    
        -
    
             uint8_t ip_proto;
    
             struct eth_header *l2 = dp_packet_eth(pkt);
    
             if (l2->eth_type == htons(ETH_TYPE_IPV6)) {
    
        @@ -1178,18 +1175,16 @@ sweep_bucket(struct conntrack *ct, struct conntrack_bucket *ctb,
    
                 }
    
             }
    
         
    
        -#define MAX_ALG_EXP_TO_EXPIRE 1000
    
        +    enum { MAX_ALG_EXP_TO_EXPIRE = 1000 };
    
             size_t alg_exp_count = hmap_count(&ct->alg_expectations);
    
             /* XXX: revisit this. */
    
        -    size_t max_to_expire =
    
        -        MAX(alg_exp_count/10, MAX_ALG_EXP_TO_EXPIRE);
    
        +    size_t max_to_expire = MAX(alg_exp_count / 10, MAX_ALG_EXP_TO_EXPIRE);
    
             count = 0;
    
             ct_rwlock_wrlock(&ct->resources_lock);
    
             struct alg_exp_node *alg_exp_node, *alg_exp_node_next;
    
             LIST_FOR_EACH_SAFE (alg_exp_node, alg_exp_node_next,
    
                                 exp_node, &ct->alg_exp_list) {
    
        -        if (now < alg_exp_node->expiration ||
    
        -            count >= max_to_expire) {
    
        +        if (now < alg_exp_node->expiration || count >= max_to_expire) {
    
                     min_expiration = MIN(min_expiration, alg_exp_node->expiration);
    
                     break;
    
                 }
    
        @@ -2355,7 +2350,6 @@ static struct alg_exp_node *
    
         expectation_lookup(struct hmap *alg_expectations,
    
                            const struct conn_key *key, uint32_t basis)
    
         {
    
        -
    
             struct conn_key check_key = *key;
    
             check_key.src.port = ALG_WC_SRC_PORT;
    
             struct alg_exp_node *alg_exp_node;
    
        @@ -2410,7 +2404,7 @@ expectation_create(struct conntrack *ct,
    
             alg_exp_node->master_mark = master_conn->mark;
    
             alg_exp_node->master_label = master_conn->label;
    
             alg_exp_node->master_key = master_conn->key;
    
        -    alg_exp_node->passive_mode = mode == CT_FTP_MODE_PASSIVE ? true : false;
    
        +    alg_exp_node->passive_mode = mode == CT_FTP_MODE_PASSIVE;
    
             /* Take the write lock here because it is almost 100%
    
              * likely that the lookup will fail and
    
              * expectation_create() will be called below. */
    
        @@ -2458,7 +2452,7 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep,
    
                          char *ftp_data_start,
    
                          size_t addr_offset_from_ftp_data_start)
    
         {
    
        -#define MAX_FTP_V4_NAT_DELTA 8
    
        +    enum { MAX_FTP_V4_NAT_DELTA = 8 };
    
         
    
             /* Do conservative check for pathological MTU usage. */
    
             uint32_t orig_used_size = dp_packet_size(pkt);
    
        @@ -2475,26 +2469,25 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep,
    
         
    
             int overall_delta = 0;
    
             char *byte_str = ftp_data_start + addr_offset_from_ftp_data_start;
    
        -    char *next_delim;
    
        -    size_t substr_size;
    
        -    uint8_t rep_byte;
    
        -    char rep_str[4];
    
        -    size_t replace_size;
    
         
    
        +    /* Replace the existing IPv4 address by the new one. */
    
             for (uint8_t i = 0; i < 4; i++) {
    
        -        memset(rep_str, 0 , sizeof rep_str);
    
        -        next_delim = memchr(byte_str,',',4);
    
        +        /* Find the end of the string for this octet. */
    
        +        char *next_delim = memchr(byte_str, ',', 4);
    
                 ovs_assert(next_delim);
    
        -        substr_size = next_delim - byte_str;
    
        +        int substr_size = next_delim - byte_str;
    
                 remain_size -= substr_size;
    
        -        rep_byte = get_v4_byte_be(v4_addr_rep, i);
    
        -        replace_size = sprintf(rep_str, "%d", rep_byte);
    
        -        ovs_assert(replace_size == strlen(rep_str));
    
        +
    
        +        /* Compose the new string for this octet, and replace it. */
    
        +        char rep_str[4];
    
        +        uint8_t rep_byte = get_v4_byte_be(v4_addr_rep, i);
    
        +        int replace_size = sprintf(rep_str, "%d", rep_byte);
    
                 replace_substring(byte_str, substr_size, remain_size,
    
                                   rep_str, replace_size);
    
         
    
        -        overall_delta += (int) replace_size - (int) substr_size;
    
        -        /* Add 1 to skip the ',' character. */
    
        +        overall_delta += replace_size - substr_size;
    
        +
    
        +        /* Advance past the octet and the following comma. */
    
                 byte_str += replace_size + 1;
    
             }
    
         
    
        @@ -2505,7 +2498,7 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep,
    
         static char *
    
         skip_non_digits(char *str)
    
         {
    
        -    while ((!isdigit(*str)) && (*str != 0)) {
    
        +    while (!isdigit(*str) && *str != 0) {
    
                 str++;
    
             }
    
             return str;
    
        @@ -2603,9 +2596,9 @@ process_ftp_ctl_v4(struct conntrack *ct,
    
             *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg;
    
             uint8_t comma_count = 0;
    
         
    
        -    while ((comma_count < 4) && (*ftp != 0)) {
    
        +    while (comma_count < 4 && *ftp) {
    
                 if (*ftp == ',') {
    
        -            comma_count ++;
    
        +            comma_count++;
    
                     if (comma_count == 4) {
    
                         *ftp = 0;
    
                     } else {
    
        @@ -2687,8 +2680,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
    
             ftp_ipv4_addr = ip_addr.s_addr;
    
             /* Although most servers will block this exploit, there may be some
    
              * less well managed. */
    
        -    if ((ftp_ipv4_addr != conn_ipv4_addr) &&
    
        -        (ftp_ipv4_addr != *v4_addr_rep)) {
    
        +    if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) {
    
                 return CT_FTP_CTL_INVALID;
    
             }
    
         
    
        @@ -2699,7 +2691,7 @@ process_ftp_ctl_v4(struct conntrack *ct,
    
         static char *
    
         skip_ipv6_digits(char *str)
    
         {
    
        -    while (isxdigit(*str) || (*str == ':') || (*str == '.')) {
    
        +    while (isxdigit(*str) || *str == ':' || *str == '.') {
    
                 str++;
    
             }
    
             return str;
    
        @@ -2728,7 +2720,7 @@ process_ftp_ctl_v6(struct conntrack *ct,
    
             if (!strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD))) {
    
                 ftp = ftp_msg + strlen(FTP_EPRT_CMD);
    
                 ftp = skip_non_digits(ftp);
    
        -        if ((*ftp != FTP_AF_V6) || isdigit(*(ftp + 1))) {
    
        +        if (*ftp != FTP_AF_V6 || isdigit(ftp[1])) {
    
                     return CT_FTP_CTL_INVALID;
    
                 }
    
                 /* Jump over delimiter. */
    
        @@ -2761,7 +2753,7 @@ process_ftp_ctl_v6(struct conntrack *ct,
    
             }
    
         
    
             char *save_ftp = ftp;
    
        -    ftp = terminate_number_str(ftp , MAX_EXT_FTP_PORT_DGTS);
    
        +    ftp = terminate_number_str(ftp, MAX_EXT_FTP_PORT_DGTS);
    
             if (!ftp) {
    
                 return CT_FTP_CTL_INVALID;
    
             }
    
        @@ -2804,8 +2796,8 @@ repl_ftp_v6_addr(struct dp_packet *pkt, struct ct_addr v6_addr_rep,
    
                          size_t addr_offset_from_ftp_data_start,
    
                          size_t addr_size, enum ct_alg_mode mode)
    
         {
    
        -/* This is slightly bigger than really possible. */
    
        -#define MAX_FTP_V6_NAT_DELTA 45
    
        +    /* This is slightly bigger than really possible. */
    
        +    enum { MAX_FTP_V6_NAT_DELTA = 45 };
    
         
    
             if (mode == CT_FTP_MODE_PASSIVE) {
    
                 return 0;
    
        _______________________________________________
    
        dev mailing list
    
        dev at 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=slnaGm2FfgVFUXoMAtwexhupDrGoTgnTbkIr61-2zZc&s=h_18XnXwXiAbBXlSLdk1sHsTej2xidpMu8i6_7GCXAs&e= 
    
        
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    
    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=FbX-QDY2V0W_pOsdK6nFNN0vxQRW55OXjz9vr6qooBg&s=0113f_psdl-25Ycy5haeVsl6_wWXbR69VIASk56lGvc&e= 
    



More information about the dev mailing list