[ovs-dev,patch_v4,4/8] Userspace Datapath: Add ALG infra and FTP.

On Wed, Jul 05, 2017 at 09:32:22PM -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@gmail.com>

Thanks a lot for working on this.  It will be a valuable feature that
brings the userspace datapath closer to the kernel datapath features.

I have some comments.  I'm not too familiar with this area, so a lot of
my comments are ones that should help to make the code easier to
understand not just for me but for other newbies to ALG implementation.

The data structures introduced or modified in this patch are almost
comment-free.  The reader is left to guess important information like
the purpose of the structure, as well as per-member info like what lists
or hmaps a structure belongs to, what a "master" is, and so on.  Please
add some comments.

The same seems warranted for the collection of macros that this
defines.  For example, what does FTP_MAX_PORT mean?  (If it's just the
maximum TCP port, what makes it FTP specific? etc.)

In the name "alg_exp_node", I don't know what an exp is.

I don't know what "delinate" means.

What OS X problem does this allude to?
+    /* CT_IPPORT_FTP is used in lieu of IPPORT_FTP as a workaround
+     * to handle OSX. */
+#define CT_IPPORT_FTP 21

Usually we put the conversion on the constant side in comparisons like
the following, because compilers are better at optimizing it:
+    return (ip_proto == IPPROTO_TCP &&
+            (ntohs(th->tcp_src) == CT_IPPORT_FTP ||
+             ntohs(th->tcp_dst) == CT_IPPORT_FTP));

I guess that not everyone knows what "tuple space" is.  I had to think
about it for a while.  I think you basically mean the ports available
for NAT.  Maybe a more user friendly term could be used (at least in
user messages)?  Why is exhaustion "likely a user error"?  (I would have
guessed that it is more likely from some kind of DoS or port scan or
equivalent.)  How should a user respond?

process_one() has a variable alg_nat_repl_addr that it zeros and then
appears to never use again.

Also in process_one(), I think that this memcpy:
            memcpy(&alg_exp_entry, alg_exp, sizeof alg_exp_entry);
can be written as just:
            alg_exp_entry = *alg_exp;
although I don't know whether you have some expectation for padding etc.

process_one() uses the expression "conn && is_ftp_ctl(pkt)" twice, and
the latter function is nontrivial.  Can it be evaluated just once?

In conntrack_execute(), it seems odd to move the loop index declaration
out of the for statement.

conn_to_ct_dpif_entry() does an xstrdup but I wasn't able to spot where
the string gets freed.

I can't see how get_v4_byte_be() works properly on both big-endian and
little-endian systems.

get_v4_byte_be() shouldn't need the "& 0xff", since it returns a
uint8_t.

In replace_substring(), it seems odd to use 8-bit quantities for sizes.
Also, it looks like 'delta' can be negative, in which case it should not
be plain char (which can be signed or unsigned given an ASCII character
set): if you really want 8-bit, use "signed char" or int8_t.

The expression "remain_substring + delta" in replace_substring() kind of
threw me for a loop.  If you expand this based on variable definitions,
you get:

        remain_substring + delta
     == (substr + substr_size) + (rep_str_size - substr_size)
     == substr + rep_str_size

In other words, the substr_size cancels out entirely, so that I think
the first four statements
+    char delta = rep_str_size - substr_size;
+    size_t move_size = total_size - substr_size;
+    char *remain_substring = substr + substr_size;
+    memmove(remain_substring + delta, remain_substring,
+            move_size);
might as well be written:
     memmove(substr + rep_str_size, substr + substr_size,
             total_size - substr_size);
which looks a lot simpler to me.

In repl_ftp_v4_addr(), don't replace_size and rc have the same value?
And there are two calls to strlen(rep_str).

I think that repl_ftp_v4_addr() would be easier to read if variables
were declared at first use, more like this:

Also in repl_ftp_v4_addr(), it seems odd that we're dealing with
maintaining a minimum frame length at this low level; I'd expect that to
happen at a higher level.  At least that's what I assume the MAX(...,
64) is for; maybe ETH_TOTAL_MIN would be better?

Instead of FTP_EPRT_CMD_SIZE (etc.), it might be better to just write
strlen(FTP_EPRT_CMD).  The meaning is clear at a glance and modern
compilers will optimize such an expression to a constant.

I found the callers of detect_ftp_ctl_mode() puzzling at first because
this function does two different things but each of its callers only
care about one of them.  How about dividing it into two functions, like
this?

This:
    /* Find first space. */
    while ((*ftp != ' ') && (*ftp != 0)) {
        ftp++;
    }
    if (*ftp != ' ') {
        return CT_FTP_CTL_INVALID;
    }
can be written as just:
    /* Find first space. */
    ftp = strchr(ftp, ' ');
    if (!ftp) {
        return CT_FTP_CTL_INVALID;
    }

I see 3 casts to integer types in process_ftp_ctl_v4() but none of them
appears to be necessary.  The (OVS_FORCE ovs_be16) cast seems especially
odd since htons() actually returns an ovs_be16.  Same in
process_ftp_ctl_v6().  Similarly for htonl(), twice, in handle_ftp_ctl().

I wonder whether the parsing in process_ftp_ctl_v4() would be easier
using sscanf().

Again, thanks a lot for implementing this feature!

Ben

[ovs-dev,patch_v4,4/8] Userspace Datapath: Add ALG infra and FTP.

Commit Message

Comments

Patch