From ba4aaf6ae71ad674fc10a2b7ac69c444c2038afa Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Tue, 11 Feb 2014 08:24:16 -0800 Subject: [PATCH] ofproto-dpif-xlate: Make flows that match ICMP fields revalidate correctly. ICMPv4 and ICMPv6 have 8-bit "type" and "code" fields. struct flow uses the low 8 bits of the 16-bit tp_src and tp_dst members to represent these fields. The datapath interface, on the other hand, represents them with just 8 bits each. This means that if the high 8 bits of the masks for these fields somehow become set (meaning to match on the nonexistent "high bits" of these fields) during translation, then they will get chopped off by a round trip through the datapath, and revalidation will spot that as an inconsistency and delete the flow. This commit avoids the problem by making sure that only the low 8 bits of either field can be unwildcarded for ICMP. This seems like the minimal fix for this problem, appropriate for backporting to earlier branches. The root of the issue is that these high bits can get set in the match at all. I have some leads on that, but they require more invasive changes elsewhere. Bug #23320. Reported-by: Krishna Miriyala Signed-off-by: Ben Pfaff Acked-by: Andy Zhou --- lib/meta-flow.c | 14 -------------- lib/packets.h | 14 +++++++++++++- ofproto/ofproto-dpif-xlate.c | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 96e0efe69..463e01ccf 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1019,20 +1019,6 @@ mf_is_mask_valid(const struct mf_field *mf, const union mf_value *mask) OVS_NOT_REACHED(); } -static bool -is_icmpv4(const struct flow *flow) -{ - return (flow->dl_type == htons(ETH_TYPE_IP) - && flow->nw_proto == IPPROTO_ICMP); -} - -static bool -is_icmpv6(const struct flow *flow) -{ - return (flow->dl_type == htons(ETH_TYPE_IPV6) - && flow->nw_proto == IPPROTO_ICMPV6); -} - /* Returns true if 'flow' meets the prerequisites for 'mf', false otherwise. */ bool mf_are_prereqs_ok(const struct mf_field *mf, const struct flow *flow) diff --git a/lib/packets.h b/lib/packets.h index d291c1445..f3e1dfb7b 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -616,6 +616,18 @@ static inline bool is_ip_any(const struct flow *flow) return dl_type_is_ip_any(flow->dl_type); } +static inline bool is_icmpv4(const struct flow *flow) +{ + return (flow->dl_type == htons(ETH_TYPE_IP) + && flow->nw_proto == IPPROTO_ICMP); +} + +static inline bool is_icmpv6(const struct flow *flow) +{ + return (flow->dl_type == htons(ETH_TYPE_IPV6) + && flow->nw_proto == IPPROTO_ICMPV6); +} + void format_ipv6_addr(char *addr_str, const struct in6_addr *addr); void print_ipv6_addr(struct ds *string, const struct in6_addr *addr); void print_ipv6_masked(struct ds *string, const struct in6_addr *addr, diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index bb85b89d8..0a71da729 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3014,6 +3014,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout) struct xlate_ctx ctx; size_t ofpacts_len; bool tnl_may_send; + bool is_icmp; COVERAGE_INC(xlate_actions); @@ -3068,6 +3069,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout) if (is_ip_any(flow)) { wc->masks.nw_frag |= FLOW_NW_FRAG_MASK; } + is_icmp = is_icmpv4(flow) || is_icmpv6(flow); tnl_may_send = tnl_xlate_init(&ctx.base_flow, flow, wc); if (ctx.xbridge->netflow) { @@ -3228,6 +3230,21 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout) * use non-header fields as part of the cache. */ flow_wildcards_clear_non_packet_fields(wc); + /* ICMPv4 and ICMPv6 have 8-bit "type" and "code" fields. struct flow uses + * the low 8 bits of the 16-bit tp_src and tp_dst members to represent + * these fields. The datapath interface, on the other hand, represents + * them with just 8 bits each. This means that if the high 8 bits of the + * masks for these fields somehow become set, then they will get chopped + * off by a round trip through the datapath, and revalidation will spot + * that as an inconsistency and delete the flow. Avoid the problem here by + * making sure that only the low 8 bits of either field can be unwildcarded + * for ICMP. + */ + if (is_icmp) { + wc->masks.tp_src &= htons(UINT8_MAX); + wc->masks.tp_dst &= htons(UINT8_MAX); + } + out: rule_actions_unref(actions); rule_dpif_unref(rule); -- 2.20.1