One update inline.
-----Original Message-----
From: <ovs-dev-boun...@openvswitch.org> on behalf of Darrell Ball
<db...@vmware.com>
Date: Friday, August 4, 2017 at 8:14 PM
To: Ben Pfaff <b...@ovn.org>, Darrell Ball <dlu...@gmail.com>
Cc: "d...@openvswitch.org" <d...@openvswitch.org>
Subject: Re: [ovs-dev] [patch_v9 2/6] Userspace Datapath: Add ALG infra and FTP.
-----Original Message-----
From: <ovs-dev-boun...@openvswitch.org> on behalf of Ben Pfaff
<b...@ovn.org>
Date: Friday, August 4, 2017 at 2:51 PM
To: Darrell Ball <dlu...@gmail.com>
Cc: "d...@openvswitch.org" <d...@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 <dlu...@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
d...@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
d...@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=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
