Mike Pattrick via dev <[email protected]> writes:
> Currently ipf will inject expired fragments into the odp execution
> pipeline in an indeterminate location. Previously this was changed to
> segregate by IP version. However, there was nothing to stop fragments
> from being inserted into different tables, zones, or being sent in
> different directions.
>
> The kernel module just terminates these fragments, and userspace should
> as well. There are also new coverage counters to indicate how fragments
> expired.
>
> Reported-at: https://issues.redhat.com/browse/FDP-1052
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
Thanks Mike - I've applied this and backported down to 3.2.
> lib/ipf.c | 49 +++++++++++++++-------------------
> tests/ofproto-dpif.at | 61 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 82 insertions(+), 28 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index b76181e79..53dfe6664 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -35,6 +35,7 @@
> #include "util.h"
>
> VLOG_DEFINE_THIS_MODULE(ipf);
> +COVERAGE_DEFINE(ipf_stuck_frag_list_expired);
> COVERAGE_DEFINE(ipf_stuck_frag_list_purged);
> COVERAGE_DEFINE(ipf_l3csum_err);
>
> @@ -77,7 +78,7 @@ enum {
> enum ipf_counter_type {
> IPF_NFRAGS_ACCEPTED,
> IPF_NFRAGS_COMPL_SENT,
> - IPF_NFRAGS_EXPD_SENT,
> + IPF_NFRAGS_EXPIRED,
> IPF_NFRAGS_TOO_SMALL,
> IPF_NFRAGS_OVERLAP,
> IPF_NFRAGS_PURGED,
> @@ -1030,8 +1031,7 @@ ipf_purge_list_check(struct ipf *ipf, struct ipf_list
> *ipf_list,
> * with 'ipf_send_completed_frags()' and 'ipf_send_expired_frags()'. */
> static bool
> ipf_send_frags_in_list(struct ipf *ipf, struct ipf_list *ipf_list,
> - struct dp_packet_batch *pb,
> - enum ipf_list_type list_type, bool v6, long long now)
> + struct dp_packet_batch *pb, bool v6, long long now)
> OVS_REQUIRES(ipf->ipf_lock)
> {
> if (ipf_purge_list_check(ipf, ipf_list, now)) {
> @@ -1045,12 +1045,7 @@ ipf_send_frags_in_list(struct ipf *ipf, struct
> ipf_list *ipf_list,
> ipf_list->last_sent_idx++;
> atomic_count_dec(&ipf->nfrag);
>
> - if (list_type == IPF_FRAG_COMPLETED_LIST) {
> - ipf_count(ipf, v6, IPF_NFRAGS_COMPL_SENT);
> - } else {
> - ipf_count(ipf, v6, IPF_NFRAGS_EXPD_SENT);
> - pkt->md.ct_state = CS_INVALID;
> - }
> + ipf_count(ipf, v6, IPF_NFRAGS_COMPL_SENT);
>
> if (ipf_list->last_sent_idx == ipf_list->last_inuse_idx) {
> return true;
> @@ -1080,8 +1075,7 @@ ipf_send_completed_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
> if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
> continue;
> }
> - if (ipf_send_frags_in_list(ipf, ipf_list, pb,
> IPF_FRAG_COMPLETED_LIST,
> - v6, now)) {
> + if (ipf_send_frags_in_list(ipf, ipf_list, pb, v6, now)) {
> ipf_completed_list_clean(&ipf->frag_lists, ipf_list);
> } else {
> break;
> @@ -1091,12 +1085,9 @@ ipf_send_completed_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
> ovs_mutex_unlock(&ipf->ipf_lock);
> }
>
> -/* Conservatively adds fragments associated with a expired fragment list to
> - * a packet batch to be processed by the calling application, typically
> - * conntrack. Also cleans up the list context when it is empty.*/
> +/* Remove expired fragment lists and clean up the list context. */
> static void
> -ipf_send_expired_frags(struct ipf *ipf, struct dp_packet_batch *pb,
> - long long now, bool v6)
> +ipf_delete_expired_frags(struct ipf *ipf, long long now)
> {
> enum {
> /* Very conservative, due to DOS probability. */
> @@ -1113,21 +1104,23 @@ ipf_send_expired_frags(struct ipf *ipf, struct
> dp_packet_batch *pb,
> size_t lists_removed = 0;
>
> LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_exp_list) {
> - if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
> - continue;
> - }
> if (now <= ipf_list->expiration ||
> lists_removed >= IPF_FRAG_LIST_MAX_EXPIRED) {
> break;
> }
>
> - if (ipf_send_frags_in_list(ipf, ipf_list, pb, IPF_FRAG_EXPIRY_LIST,
> - v6, now)) {
> - ipf_expiry_list_clean(&ipf->frag_lists, ipf_list);
> - lists_removed++;
> - } else {
> - break;
> + while (ipf_list->last_sent_idx < ipf_list->last_inuse_idx) {
> + struct dp_packet * pkt
> + = ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt;
> + dp_packet_delete(pkt);
> + atomic_count_dec(&ipf->nfrag);
> + COVERAGE_INC(ipf_stuck_frag_list_expired);
> + ipf_count(ipf, ipf_list->key.dl_type == htons(ETH_TYPE_IPV6),
> + IPF_NFRAGS_EXPIRED);
> + ipf_list->last_sent_idx++;
> }
> + ipf_expiry_list_clean(&ipf->frag_lists, ipf_list);
> + lists_removed++;
> }
>
> ovs_mutex_unlock(&ipf->ipf_lock);
> @@ -1275,7 +1268,7 @@ ipf_postprocess_conntrack(struct ipf *ipf, struct
> dp_packet_batch *pb,
> bool v6 = dl_type == htons(ETH_TYPE_IPV6);
> ipf_post_execute_reass_pkts(ipf, pb, v6);
> ipf_send_completed_frags(ipf, pb, now, v6);
> - ipf_send_expired_frags(ipf, pb, now, v6);
> + ipf_delete_expired_frags(ipf, now);
> }
> }
>
> @@ -1448,7 +1441,7 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
> *ipf_status)
> &ipf_status->v4.nfrag_accepted);
> atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_COMPL_SENT],
> &ipf_status->v4.nfrag_completed_sent);
> - atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_EXPD_SENT],
> + atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_EXPIRED],
> &ipf_status->v4.nfrag_expired_sent);
> atomic_read_relaxed(&ipf->n4frag_cnt[IPF_NFRAGS_TOO_SMALL],
> &ipf_status->v4.nfrag_too_small);
> @@ -1464,7 +1457,7 @@ ipf_get_status(struct ipf *ipf, struct ipf_status
> *ipf_status)
> &ipf_status->v6.nfrag_accepted);
> atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_COMPL_SENT],
> &ipf_status->v6.nfrag_completed_sent);
> - atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_EXPD_SENT],
> + atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_EXPIRED],
> &ipf_status->v6.nfrag_expired_sent);
> atomic_read_relaxed(&ipf->n6frag_cnt[IPF_NFRAGS_TOO_SMALL],
> &ipf_status->v6.nfrag_too_small);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 6f7cc166f..a35b80077 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5312,6 +5312,67 @@
> recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - fragment handling - reassembly])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 90
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 tcp actions=ct(commit, table=1)
> +table=1 tcp actions=2
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow11 replace-flows br0 flows.txt])
> +
> +dnl Test frag expiry.
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
> +ovs-appctl time/warp 10000
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 0014 0001 8192 40 06 3316 ac11370d ac11370b"])
> +
> +dnl Test that no packets flow.
> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | sed
> s'/recirc(.*)/recirc(X)/'], [0], [dnl
> +flow-dump from the main thread:
> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later),
> packets:0, bytes:0, used:never, actions:ct(commit),recirc(X)
> +])
> +
> +dnl Expire second frag.
> +ovs-appctl time/warp 10000
> +
> +dnl Test frag purge
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 0014 0002 8192 40 06 3315 ac11370d ac11370b"])
> +ovs-appctl time/warp 33000
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 0014 0003 8192 40 06 3314 ac11370d ac11370b"])
> +
> +dnl Test that no packets flow.
> +AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\) | sed
> s'/recirc(.*)/recirc(X)/'], [0], [dnl
> +flow-dump from the main thread:
> +recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later),
> packets:0, bytes:0, used:never, actions:ct(commit),recirc(X)
> +])
> +
> +dnl Purge second frag
> +ovs-appctl time/warp 33000
> +
> +dnl Make sure all four packets are counted properly in the coverage.
> +AT_CHECK([ovs-appctl coverage/show | grep -c "^ipf.*total: 2"], [0], [2
> +])
> +
> +zero1208=$(printf '0%.0s' $(seq 2416))
> +dnl Test that reassembled packets flow.
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 04c4 0002 2000 40 06 8ff7 ac11370d ac11370b dnl
> +0000 0001 00000000 00000000 50 10 8000 604c 0000 dnl
> +$zero1208"])
> +AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
> 0800 4500 04c4 0002 0096 40 06 af61 ac11370d ac11370b dnl
> +$zero1208"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/recirc[[^)]]*)/recirc()/g' |
> strip_used | sort], [0], [dnl
> +flow-dump from the main thread:
> +recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=first),
> packets:0, bytes:0, used:never, actions:2
> +recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=first),
> packets:0, bytes:0, used:never, actions:ct(commit),recirc()
> +recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later),
> packets:0, bytes:0, used:never, actions:2
> +recirc(),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=later),
> packets:0, bytes:0, used:never, actions:ct(commit),recirc()
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([ofproto-dpif - handling of malformed TCP packets])
> OVS_VSWITCHD_START
> add_of_ports br0 1 90
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
