| Message ID | 1490226487-37516-1-git-send-email-jarno@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Mar 23, 2017 at 5:18 AM, Jarno Rajahalme <jarno@ovn.org> wrote: > Conntrack original direction tuple fields depend on the conntrack > state and the type of the packet that was tracked. These dependencies > were encoded as OpenFlow prerequisites in commit daf4d3c18da4 ("odp: > Support conntrack orig tuple key."). However, having a prerequisite > from a metadata field to a packet header turned out to be problematic, > since sometimes we are decoding metadata fields alone, so that the > packet type field is not available. > > The reason for the packet type dependency is that the IP addresses in > the original direction tuple can be either IPv4 or IPv6 addresses, and > it would be invalid to match on IPv4 original direction tuple > addresses for an IPv6 packet and vica verca. Upon closer look, > however, allowing this kind of mismatched match only causes the flow > to never match anything, rather than causing more severe problems. > > This patch removes the formal prerequisite on the packet type, but > replaces that with an explicit check for the mismatch on flow install. > This way we can still return an error to the controller if it tries to > install a mismatched flow. > > Reported-by: Dong Jun <dongj@dtdream.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/ > 330052.html > Fixes: 7befb20d0f70 ("nx-match: Fix oxm decode.") > Fixes: daf4d3c18da4 ("odp: Support conntrack orig tuple key.") > Suggested-by: Numan Siddique <nusiddiq@redhat.com> > Suggested-by: Ben Pfaff <blp@ovn.org> > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > Tested-by: Numan Siddique <nusiddiq@redhat.com> Acked-by: Numan Siddique <nusiddiq@redhat.com> --- > build-aux/extract-ofp-fields | 2 -- > include/openvswitch/meta-flow.h | 10 ++++------ > lib/meta-flow.c | 6 ------ > lib/ofp-util.c | 10 ++++++++++ > 4 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields > index a26d558..af7c69b 100755 > --- a/build-aux/extract-ofp-fields > +++ b/build-aux/extract-ofp-fields > @@ -45,8 +45,6 @@ PREREQS = {"none": "MFP_NONE", > "IPv6": "MFP_IPV6", > "IPv4/IPv6": "MFP_IP_ANY", > "CT": "MFP_CT_VALID", > - "CTv4": "MFP_CTV4_VALID", > - "CTv6": "MFP_CTV6_VALID", > "MPLS": "MFP_MPLS", > "TCP": "MFP_TCP", > "UDP": "MFP_UDP", > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta- > flow.h > index 29ccadb..11852d2 100644 > --- a/include/openvswitch/meta-flow.h > +++ b/include/openvswitch/meta-flow.h > @@ -772,7 +772,7 @@ enum OVS_PACKED_ENUM mf_field_id { > * Type: be32. > * Maskable: bitwise. > * Formatting: IPv4. > - * Prerequisites: CTv4. > + * Prerequisites: CT. > * Access: read-only. > * NXM: NXM_NX_CT_NW_SRC(120) since v2.8. > * OXM: none. > @@ -791,7 +791,7 @@ enum OVS_PACKED_ENUM mf_field_id { > * Type: be32. > * Maskable: bitwise. > * Formatting: IPv4. > - * Prerequisites: CTv4. > + * Prerequisites: CT. > * Access: read-only. > * NXM: NXM_NX_CT_NW_DST(121) since v2.8. > * OXM: none. > @@ -810,7 +810,7 @@ enum OVS_PACKED_ENUM mf_field_id { > * Type: be128. > * Maskable: bitwise. > * Formatting: IPv6. > - * Prerequisites: CTv6. > + * Prerequisites: CT. > * Access: read-only. > * NXM: NXM_NX_CT_IPV6_SRC(122) since v2.8. > * OXM: none. > @@ -829,7 +829,7 @@ enum OVS_PACKED_ENUM mf_field_id { > * Type: be128. > * Maskable: bitwise. > * Formatting: IPv6. > - * Prerequisites: CTv6. > + * Prerequisites: CT. > * Access: read-only. > * NXM: NXM_NX_CT_IPV6_DST(123) since v2.8. > * OXM: none. > @@ -1824,8 +1824,6 @@ enum OVS_PACKED_ENUM mf_prereqs { > MFP_ICMPV4, > MFP_ICMPV6, > MFP_CT_VALID, /* Implies IPv4 or IPv6. */ > - MFP_CTV4_VALID, /* MFP_CT_VALID and IPv4. */ > - MFP_CTV6_VALID, /* MFP_CT_VALID and IPv6. */ > > /* L2+L3+L4 requirements. */ > MFP_ND, > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index 93fbc5b..6b97794 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -419,12 +419,6 @@ mf_are_prereqs_ok__(const struct mf_field *mf, const > struct flow *flow, > return is_ip_any(flow); > case MFP_CT_VALID: > return is_ct_valid(flow, mask, wc); > - case MFP_CTV4_VALID: > - return flow->dl_type == htons(ETH_TYPE_IP) > - && is_ct_valid(flow, mask, wc); > - case MFP_CTV6_VALID: > - return flow->dl_type == htons(ETH_TYPE_IPV6) > - && is_ct_valid(flow, mask, wc); > case MFP_TCP: > /* Matching !FRAG_LATER is not enforced (mask is not checked). */ > return is_tcp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER); > diff --git a/lib/ofp-util.c b/lib/ofp-util.c > index b2f96ea..54c83fa 100644 > --- a/lib/ofp-util.c > +++ b/lib/ofp-util.c > @@ -1724,6 +1724,16 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, > } > } > > + /* Check for mismatched conntrack original direction tuple address > fields > + * w.r.t. the IP version of the match. */ > + if (((fm->match.wc.masks.ct_nw_src || fm->match.wc.masks.ct_nw_dst) > + && fm->match.flow.dl_type != htons(ETH_TYPE_IP)) > + || ((ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_src) > + || ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_dst)) > + && fm->match.flow.dl_type != htons(ETH_TYPE_IPV6))) { > + return OFPERR_OFPBAC_MATCH_INCONSISTENT; > + } > + > if (fm->command > OFPFC_DELETE_STRICT) { > return OFPERR_OFPFMFC_BAD_COMMAND; > } > -- > 2.1.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Mar 22, 2017 at 04:48:07PM -0700, Jarno Rajahalme wrote: > Conntrack original direction tuple fields depend on the conntrack > state and the type of the packet that was tracked. These dependencies > were encoded as OpenFlow prerequisites in commit daf4d3c18da4 ("odp: > Support conntrack orig tuple key."). However, having a prerequisite > from a metadata field to a packet header turned out to be problematic, > since sometimes we are decoding metadata fields alone, so that the > packet type field is not available. > > The reason for the packet type dependency is that the IP addresses in > the original direction tuple can be either IPv4 or IPv6 addresses, and > it would be invalid to match on IPv4 original direction tuple > addresses for an IPv6 packet and vica verca. Upon closer look, > however, allowing this kind of mismatched match only causes the flow > to never match anything, rather than causing more severe problems. > > This patch removes the formal prerequisite on the packet type, but > replaces that with an explicit check for the mismatch on flow install. > This way we can still return an error to the controller if it tries to > install a mismatched flow. > > Reported-by: Dong Jun <dongj@dtdream.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/330052.html > Fixes: 7befb20d0f70 ("nx-match: Fix oxm decode.") > Fixes: daf4d3c18da4 ("odp: Support conntrack orig tuple key.") > Suggested-by: Numan Siddique <nusiddiq@redhat.com> > Suggested-by: Ben Pfaff <blp@ovn.org> > Signed-off-by: Jarno Rajahalme <jarno@ovn.org> This seems reasonable to me. Acked-by: Ben Pfaff <blp@ovn.org>
Ben & Numan, Thanks for the reviews, pushed to master. Jarno > On Mar 24, 2017, at 11:06 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Mar 22, 2017 at 04:48:07PM -0700, Jarno Rajahalme wrote: >> Conntrack original direction tuple fields depend on the conntrack >> state and the type of the packet that was tracked. These dependencies >> were encoded as OpenFlow prerequisites in commit daf4d3c18da4 ("odp: >> Support conntrack orig tuple key."). However, having a prerequisite >> from a metadata field to a packet header turned out to be problematic, >> since sometimes we are decoding metadata fields alone, so that the >> packet type field is not available. >> >> The reason for the packet type dependency is that the IP addresses in >> the original direction tuple can be either IPv4 or IPv6 addresses, and >> it would be invalid to match on IPv4 original direction tuple >> addresses for an IPv6 packet and vica verca. Upon closer look, >> however, allowing this kind of mismatched match only causes the flow >> to never match anything, rather than causing more severe problems. >> >> This patch removes the formal prerequisite on the packet type, but >> replaces that with an explicit check for the mismatch on flow install. >> This way we can still return an error to the controller if it tries to >> install a mismatched flow. >> >> Reported-by: Dong Jun <dongj@dtdream.com> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/330052.html >> Fixes: 7befb20d0f70 ("nx-match: Fix oxm decode.") >> Fixes: daf4d3c18da4 ("odp: Support conntrack orig tuple key.") >> Suggested-by: Numan Siddique <nusiddiq@redhat.com> >> Suggested-by: Ben Pfaff <blp@ovn.org> >> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> > > This seems reasonable to me. > > Acked-by: Ben Pfaff <blp@ovn.org>
diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields index a26d558..af7c69b 100755 --- a/build-aux/extract-ofp-fields +++ b/build-aux/extract-ofp-fields @@ -45,8 +45,6 @@ PREREQS = {"none": "MFP_NONE", "IPv6": "MFP_IPV6", "IPv4/IPv6": "MFP_IP_ANY", "CT": "MFP_CT_VALID", - "CTv4": "MFP_CTV4_VALID", - "CTv6": "MFP_CTV6_VALID", "MPLS": "MFP_MPLS", "TCP": "MFP_TCP", "UDP": "MFP_UDP", diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h index 29ccadb..11852d2 100644 --- a/include/openvswitch/meta-flow.h +++ b/include/openvswitch/meta-flow.h @@ -772,7 +772,7 @@ enum OVS_PACKED_ENUM mf_field_id { * Type: be32. * Maskable: bitwise. * Formatting: IPv4. - * Prerequisites: CTv4. + * Prerequisites: CT. * Access: read-only. * NXM: NXM_NX_CT_NW_SRC(120) since v2.8. * OXM: none. @@ -791,7 +791,7 @@ enum OVS_PACKED_ENUM mf_field_id { * Type: be32. * Maskable: bitwise. * Formatting: IPv4. - * Prerequisites: CTv4. + * Prerequisites: CT. * Access: read-only. * NXM: NXM_NX_CT_NW_DST(121) since v2.8. * OXM: none. @@ -810,7 +810,7 @@ enum OVS_PACKED_ENUM mf_field_id { * Type: be128. * Maskable: bitwise. * Formatting: IPv6. - * Prerequisites: CTv6. + * Prerequisites: CT. * Access: read-only. * NXM: NXM_NX_CT_IPV6_SRC(122) since v2.8. * OXM: none. @@ -829,7 +829,7 @@ enum OVS_PACKED_ENUM mf_field_id { * Type: be128. * Maskable: bitwise. * Formatting: IPv6. - * Prerequisites: CTv6. + * Prerequisites: CT. * Access: read-only. * NXM: NXM_NX_CT_IPV6_DST(123) since v2.8. * OXM: none. @@ -1824,8 +1824,6 @@ enum OVS_PACKED_ENUM mf_prereqs { MFP_ICMPV4, MFP_ICMPV6, MFP_CT_VALID, /* Implies IPv4 or IPv6. */ - MFP_CTV4_VALID, /* MFP_CT_VALID and IPv4. */ - MFP_CTV6_VALID, /* MFP_CT_VALID and IPv6. */ /* L2+L3+L4 requirements. */ MFP_ND, diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 93fbc5b..6b97794 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -419,12 +419,6 @@ mf_are_prereqs_ok__(const struct mf_field *mf, const struct flow *flow, return is_ip_any(flow); case MFP_CT_VALID: return is_ct_valid(flow, mask, wc); - case MFP_CTV4_VALID: - return flow->dl_type == htons(ETH_TYPE_IP) - && is_ct_valid(flow, mask, wc); - case MFP_CTV6_VALID: - return flow->dl_type == htons(ETH_TYPE_IPV6) - && is_ct_valid(flow, mask, wc); case MFP_TCP: /* Matching !FRAG_LATER is not enforced (mask is not checked). */ return is_tcp(flow, wc) && !(flow->nw_frag & FLOW_NW_FRAG_LATER); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index b2f96ea..54c83fa 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -1724,6 +1724,16 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm, } } + /* Check for mismatched conntrack original direction tuple address fields + * w.r.t. the IP version of the match. */ + if (((fm->match.wc.masks.ct_nw_src || fm->match.wc.masks.ct_nw_dst) + && fm->match.flow.dl_type != htons(ETH_TYPE_IP)) + || ((ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_src) + || ipv6_addr_is_set(&fm->match.wc.masks.ct_ipv6_dst)) + && fm->match.flow.dl_type != htons(ETH_TYPE_IPV6))) { + return OFPERR_OFPBAC_MATCH_INCONSISTENT; + } + if (fm->command > OFPFC_DELETE_STRICT) { return OFPERR_OFPFMFC_BAD_COMMAND; }
Conntrack original direction tuple fields depend on the conntrack state and the type of the packet that was tracked. These dependencies were encoded as OpenFlow prerequisites in commit daf4d3c18da4 ("odp: Support conntrack orig tuple key."). However, having a prerequisite from a metadata field to a packet header turned out to be problematic, since sometimes we are decoding metadata fields alone, so that the packet type field is not available. The reason for the packet type dependency is that the IP addresses in the original direction tuple can be either IPv4 or IPv6 addresses, and it would be invalid to match on IPv4 original direction tuple addresses for an IPv6 packet and vica verca. Upon closer look, however, allowing this kind of mismatched match only causes the flow to never match anything, rather than causing more severe problems. This patch removes the formal prerequisite on the packet type, but replaces that with an explicit check for the mismatch on flow install. This way we can still return an error to the controller if it tries to install a mismatched flow. Reported-by: Dong Jun <dongj@dtdream.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/330052.html Fixes: 7befb20d0f70 ("nx-match: Fix oxm decode.") Fixes: daf4d3c18da4 ("odp: Support conntrack orig tuple key.") Suggested-by: Numan Siddique <nusiddiq@redhat.com> Suggested-by: Ben Pfaff <blp@ovn.org> Signed-off-by: Jarno Rajahalme <jarno@ovn.org> --- build-aux/extract-ofp-fields | 2 -- include/openvswitch/meta-flow.h | 10 ++++------ lib/meta-flow.c | 6 ------ lib/ofp-util.c | 10 ++++++++++ 4 files changed, 14 insertions(+), 14 deletions(-)