> On Jul 16, 2018, at 7:39 PM, Darrell Ball <dlu...@gmail.com> wrote:
>
> +static void
> +ipf_expiry_list_add(struct ipf_list *ipf_list, long long now)
> + OVS_REQUIRES(ipf_lock)
> +{
> + enum {
> + IPF_FRAG_LIST_TIMEOUT_DEFAULT = 15000,
> + };
> +
> + ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT_DEFAULT;
> + ovs_list_push_back(&frag_exp_list, &ipf_list->list_node);
> +}
This was brought up in the previous review, and I think we should drop
"_DEFAULT", since it's not configurable.
> +static bool
> +ipf_return_invalid(struct dp_packet *pkt)
> +{
> + pkt->md.ct_state = CS_INVALID;
> + return false;
> +}
This function looks kind of odd to me. I think the same thing could be
accomplished more clearly with a goto in the functions that need it. I've
implemented it in my incremental, below.
> +
> +static bool
> +ipf_is_valid_v4_frag(struct dp_packet *pkt)
> +{
> + if (OVS_UNLIKELY(dp_packet_ip_checksum_bad(pkt))) {
> + return ipf_return_invalid(pkt);
> + }
Do you think it would make sense to provide a counter when a packet gets marked
as invalid?
> +static void
> +ipf_extract_frags_from_batch(struct dp_packet_batch *pb, ovs_be16 dl_type,
> + uint16_t zone, long long now, uint32_t
> hash_basis)
> +{
> + const size_t pb_cnt = dp_packet_batch_size(pb);
> + int pb_idx; /* Index in a packet batch. */
> + struct dp_packet *pkt;
> +
> + DP_PACKET_BATCH_REFILL_FOR_EACH (pb_idx, pb_cnt, pkt, pb) {
> +
> + if (OVS_UNLIKELY((dl_type == htons(ETH_TYPE_IP) &&
> + ipf_is_valid_v4_frag(pkt)) ||
> + (dl_type == htons(ETH_TYPE_IPV6) &&
> + ipf_is_valid_v6_frag(pkt)))) {
Is this case unlikely?
> diff --git a/lib/ipf.h b/lib/ipf.h
> new file mode 100644
> index 0000000..212d1b3
> --- /dev/null
> +++ b/lib/ipf.h
> ...
> +struct ipf_status {
> + bool ifp_v4_enabled;
> + unsigned int min_v4_frag_size;
> + unsigned int nfrag_max;
> + unsigned int nfrag;
> + unsigned int n4frag_accepted;
> + unsigned int n4frag_completed_sent;
> + unsigned int n4frag_expired_sent;
> + unsigned int n4frag_too_small;
> + unsigned int n4frag_overlap;
> + bool ifp_v6_enabled;
> + unsigned int min_v6_frag_size;
> + unsigned int n6frag_accepted;
> + unsigned int n6frag_completed_sent;
> + unsigned int n6frag_expired_sent;
> + unsigned int n6frag_too_small;
> + unsigned int n6frag_overlap;
> +};
This isn't used until a later patch. Let's wait to introduce it until then.
> +/* Collects and reassembles fragments which are to be sent through
> + * conntrack, if fragment processing is enabled or fragments are
> + * in flight. */
> +void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
> + ovs_be16 dl_type, uint16_t zone,
> + uint32_t hash_basis);
> +
> +/* Updates the state of fragments associated with reassembled packets and
> + * sends out fragments that are either associated with completed
> + * packets or expired, if fragment processing is enabled or fragments are
> + * in flight. */
> +void ipf_postprocess_conntrack(struct dp_packet_batch *pb, long long now,
> + ovs_be16 dl_type);
These functions are also described in "ipf.c". Let's just use the ones there
so that we don't have to worry about them drifting apart.
I'd like to commit it with the incremental at the bottom of this message.
--Justin
-=-=-=-=-=-=-=-=-=-=-
diff --git a/lib/ipf.c b/lib/ipf.c
index 1169a8a5e60c..bb2e2d692036 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -106,12 +106,12 @@ struct ipf_list {
struct ovs_list list_node;
struct ipf_frag *frag_list;
struct ipf_list_key key;
- struct dp_packet *reass_execute_ctx; /* reassembled packet. */
- long long expiration;
- int last_sent_idx; /* last sent fragment idx. */
- int last_inuse_idx; /* last inuse fragment idx. */
- int size; /* fragment list size. */
- uint8_t state; /* frag list state; see ipf_list_state. */
+ struct dp_packet *reass_execute_ctx; /* Reassembled packet. */
+ long long expiration; /* In milliseconds. */
+ int last_sent_idx; /* Last sent fragment idx. */
+ int last_inuse_idx; /* Last inuse fragment idx. */
+ int size; /* Fragment list size. */
+ uint8_t state; /* Frag list state; see ipf_list_state. */
};
struct reassembled_pkt {
@@ -266,10 +266,10 @@ ipf_expiry_list_add(struct ipf_list *ipf_list, long long
now)
OVS_REQUIRES(ipf_lock)
{
enum {
- IPF_FRAG_LIST_TIMEOUT_DEFAULT = 15000,
+ IPF_FRAG_LIST_TIMEOUT = 15000,
};
- ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT_DEFAULT;
+ ipf_list->expiration = now + IPF_FRAG_LIST_TIMEOUT;
ovs_list_push_back(&frag_exp_list, &ipf_list->list_node);
}
@@ -447,7 +447,7 @@ ipf_reassemble_v4_frags(struct ipf_list *ipf_list)
len += add_len;
if (len > IPV4_PACKET_MAX_SIZE) {
ipf_print_reass_packet(
- "Unsupported big reassmbled v4 packet; v4 hdr:", l3);
+ "Unsupported big reassembled v4 packet; v4 hdr:", l3);
dp_packet_delete(pkt);
return NULL;
}
@@ -488,7 +488,7 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list)
pl += add_len;
if (pl > IPV6_PACKET_MAX_DATA) {
ipf_print_reass_packet(
- "Unsupported big reassmbled v6 packet; v6 hdr:", l3);
+ "Unsupported big reassembled v6 packet; v6 hdr:", l3);
dp_packet_delete(pkt);
return NULL;
}
@@ -510,7 +510,7 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list)
if (!parse_ipv6_ext_hdrs(&data, &datasize, &nw_proto, &nw_frag, &frag_hdr)
|| !nw_frag || !frag_hdr) {
- ipf_print_reass_packet("Unparsed reassmbled v6 packet; v6 hdr:", l3);
+ ipf_print_reass_packet("Unparsed reassembled v6 packet; v6 hdr:", l3);
dp_packet_delete(pkt);
return NULL;
}
@@ -594,32 +594,25 @@ ipf_list_state_transition(struct ipf_list *ipf_list, bool
ff, bool lf,
ipf_list->state = next_state;
}
-static bool
-ipf_return_invalid(struct dp_packet *pkt)
-{
- pkt->md.ct_state = CS_INVALID;
- return false;
-}
-
static bool
ipf_is_valid_v4_frag(struct dp_packet *pkt)
{
if (OVS_UNLIKELY(dp_packet_ip_checksum_bad(pkt))) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
const struct eth_header *l2 = dp_packet_eth(pkt);
const struct ip_header *l3 = dp_packet_l3(pkt);
if (OVS_UNLIKELY(!l2 || !l3)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
const char *tail = dp_packet_tail(pkt);
uint8_t pad = dp_packet_l2_pad_size(pkt);
size_t size = tail - (char *)l3 - pad;
if (OVS_UNLIKELY(size < IP_HEADER_LEN)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
if (!(IP_IS_FRAGMENT(l3->ip_frag_off))) {
@@ -628,20 +621,20 @@ ipf_is_valid_v4_frag(struct dp_packet *pkt)
uint16_t ip_tot_len = ntohs(l3->ip_tot_len);
if (OVS_UNLIKELY(ip_tot_len != size)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
size_t ip_hdr_len = IP_IHL(l3->ip_ihl_ver) * 4;
if (OVS_UNLIKELY(ip_hdr_len < IP_HEADER_LEN)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
if (OVS_UNLIKELY(size < ip_hdr_len)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
if (OVS_UNLIKELY(!dp_packet_ip_checksum_valid(pkt)
&& csum(l3, ip_hdr_len) != 0)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
uint32_t min_v4_frag_size_;
@@ -649,9 +642,13 @@ ipf_is_valid_v4_frag(struct dp_packet *pkt)
bool lf = ipf_is_last_v4_frag(pkt);
if (OVS_UNLIKELY(!lf && dp_packet_size(pkt) < min_v4_frag_size_)) {
ipf_count(false, IPF_COUNTER_NFRAGS_TOO_SMALL);
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
return true;
+
+invalid_pkt:
+ pkt->md.ct_state = CS_INVALID;
+ return false;
}
static bool
@@ -686,7 +683,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED)
const char *l4 = dp_packet_l4(pkt);
if (OVS_UNLIKELY(!l2 || !l3 || !l4)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
const char *tail = dp_packet_tail(pkt);
@@ -695,7 +692,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED)
size_t l3_hdr_size = sizeof *l3;
if (OVS_UNLIKELY(l3_size < l3_hdr_size)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
uint8_t nw_frag = 0;
@@ -710,7 +707,7 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED)
int pl = ntohs(l3->ip6_plen);
if (OVS_UNLIKELY(pl + l3_hdr_size != l3_size)) {
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
ovs_be16 ip6f_offlg = frag_hdr->ip6f_offlg;
@@ -724,10 +721,14 @@ ipf_is_valid_v6_frag(struct dp_packet *pkt OVS_UNUSED)
if (OVS_UNLIKELY(!lf && dp_packet_size(pkt) < min_v6_frag_size_)) {
ipf_count(true, IPF_COUNTER_NFRAGS_TOO_SMALL);
- return ipf_return_invalid(pkt);
+ goto invalid_pkt;
}
return true;
+
+invalid_pkt:
+ pkt->md.ct_state = CS_INVALID;
+ return false;
}
static void
@@ -983,6 +984,7 @@ ipf_dp_packet_batch_add(struct dp_packet_batch *pb , struct
dp_packet *pkt,
* reason known right now is a mempool source check, which exists due to DPDK
* support, where packets are no longer being received on any port with a
* source matching the fragment.
+ *
* Returns true if the list was purged. */
static bool
ipf_purge_list_check(struct ipf_list *ipf_list, long long now)
@@ -1201,7 +1203,7 @@ ipf_post_execute_reass_pkts(struct dp_packet_batch *pb,
bool v6)
/* Extracts any fragments from the batch and reassembles them when a
* complete packet is received. Completed packets are attempted to
- * be added to the batch to be sent thru. conntrack. */
+ * be added to the batch to be sent through conntrack. */
void
ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
ovs_be16 dl_type, uint16_t zone, uint32_t hash_basis)
@@ -1216,7 +1218,7 @@ ipf_preprocess_conntrack(struct dp_packet_batch *pb, long
long now,
}
/* Updates fragments based on the processing of the reassembled packet sent
- * thru. conntrack and adds these fragments to any batches seen. Expired
+ * through conntrack and adds these fragments to any batches seen. Expired
* fragments are marked as invalid and also added to the batches seen
* with low priority. Reassembled packets are freed. */
void
diff --git a/lib/ipf.h b/lib/ipf.h
index 212d1b3458f9..040031f0190c 100644
--- a/lib/ipf.h
+++ b/lib/ipf.h
@@ -20,41 +20,14 @@
#include "dp-packet.h"
#include "openvswitch/types.h"
-struct ipf_status {
- bool ifp_v4_enabled;
- unsigned int min_v4_frag_size;
- unsigned int nfrag_max;
- unsigned int nfrag;
- unsigned int n4frag_accepted;
- unsigned int n4frag_completed_sent;
- unsigned int n4frag_expired_sent;
- unsigned int n4frag_too_small;
- unsigned int n4frag_overlap;
- bool ifp_v6_enabled;
- unsigned int min_v6_frag_size;
- unsigned int n6frag_accepted;
- unsigned int n6frag_completed_sent;
- unsigned int n6frag_expired_sent;
- unsigned int n6frag_too_small;
- unsigned int n6frag_overlap;
-};
-
-/* Collects and reassembles fragments which are to be sent through
- * conntrack, if fragment processing is enabled or fragments are
- * in flight. */
void ipf_preprocess_conntrack(struct dp_packet_batch *pb, long long now,
ovs_be16 dl_type, uint16_t zone,
uint32_t hash_basis);
-/* Updates the state of fragments associated with reassembled packets and
- * sends out fragments that are either associated with completed
- * packets or expired, if fragment processing is enabled or fragments are
- * in flight. */
void ipf_postprocess_conntrack(struct dp_packet_batch *pb, long long now,
ovs_be16 dl_type);
void ipf_init(void);
-
void ipf_destroy(void);
#endif /* ipf.h */
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 556a6a0db2d0..822122093944 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2140,7 +2140,7 @@ packet-out in_port=1,
packet=50540000000a505400000009080045000030000100310011a48
])
AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
-# There is one byte of overlap, hence the no packet gets thru. conntrack.
+# There is one byte of overlap, hence the no packet gets through conntrack.
AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
])
@@ -2164,7 +2164,7 @@ packet-out in_port=1,
packet=50540000000a5054000000090800450001a4000120000011834
])
AT_CHECK([ovs-ofctl bundle br0 bundle.txt])
-# There is one byte of overlap, hence the no packet gets thru. conntrack.
+# There is one byte of overlap, hence the no packet gets through conntrack.
AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
])
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
