| Message ID | 20160831014735.54058-1-diproiettod@vmware.com |
|---|---|
| State | Deferred |
| Headers | show |
On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto <diproiettod@vmware.com> wrote: > When translating a set action we also unwildcard the field in question. > This is done to correctly translate set actions with the value identical > to the ingress flow, like in the following example: > > flow table: > > tcp,actions=set_field:80->tcp_dst,output:5 > > ingress packet: > > ...,tcp,tcp_dst=80 > > datapath flow > > ...,tcp(dst=80) actions:5 > > The datapath flow must exact match the target field, because the actions > do not include a set field. (Otherwise a packet coming in with a > different tcp_dst would be matched, and its port wouldn't be altered). > > Tunnel attributes behave differently: at the datapath layer, before > action processing they're cleared (we do the same at the ofproto layer > in xlate_actions()). Therefore there's no need to unwildcard them, > because a set action would always be detected (since we zero them at the > beginning of xlate_ations()). > > This fixes a problem related to the handling of Geneve options. > Unwildcarding non existing Geneve options (as done by a > set_field:X->tun_metadata<n> on a packet coming from a non-tunnel > interface) would be problematic for the datapaths: the ODP translation > functions cannot express a match on non existing Geneve options (unlike > on other attributes), and the userspace datapath wouldn't be able to > translate them to "userspace datapath format". In both cases the > installed flow would be deleted by revalidation at the first > opportunity. > > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> I think there might be some more obscure ways of triggering this problem that still exist. Basically anything that can use a register as a target is a potential issue. For example, stack_pop, bundle, and multipath all look like they have the same masking behavior. I do have a general solution in this patch (look at the bottom of xlate_actions() where it is adjusting the wildcards): http://openvswitch.org/pipermail/dev/2016-August/078484.html I didn't realize that it was addressing an existing issue though and that patch certainly isn't suitable for anything other than master. I'm also not really a big fan of the way I handled it there since it's a pretty coarse way to do it. I would be happy to drop it if we can feel comfortable that we got all of the callsites (now and in the future) using your method. Perhaps we can just create a helper function with this check builtin and then use it everywhere? That might be enough to be confident about the future.
I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. Jarno > On Aug 31, 2016, at 10:38 AM, Jesse Gross <jesse@kernel.org> wrote: > > On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > <diproiettod@vmware.com> wrote: >> When translating a set action we also unwildcard the field in question. >> This is done to correctly translate set actions with the value identical >> to the ingress flow, like in the following example: >> >> flow table: >> >> tcp,actions=set_field:80->tcp_dst,output:5 >> >> ingress packet: >> >> ...,tcp,tcp_dst=80 >> >> datapath flow >> >> ...,tcp(dst=80) actions:5 >> >> The datapath flow must exact match the target field, because the actions >> do not include a set field. (Otherwise a packet coming in with a >> different tcp_dst would be matched, and its port wouldn't be altered). >> >> Tunnel attributes behave differently: at the datapath layer, before >> action processing they're cleared (we do the same at the ofproto layer >> in xlate_actions()). Therefore there's no need to unwildcard them, >> because a set action would always be detected (since we zero them at the >> beginning of xlate_ations()). >> >> This fixes a problem related to the handling of Geneve options. >> Unwildcarding non existing Geneve options (as done by a >> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel >> interface) would be problematic for the datapaths: the ODP translation >> functions cannot express a match on non existing Geneve options (unlike >> on other attributes), and the userspace datapath wouldn't be able to >> translate them to "userspace datapath format". In both cases the >> installed flow would be deleted by revalidation at the first >> opportunity. >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > > I think there might be some more obscure ways of triggering this > problem that still exist. Basically anything that can use a register > as a target is a potential issue. For example, stack_pop, bundle, and > multipath all look like they have the same masking behavior. > > I do have a general solution in this patch (look at the bottom of > xlate_actions() where it is adjusting the wildcards): > http://openvswitch.org/pipermail/dev/2016-August/078484.html > > I didn't realize that it was addressing an existing issue though and > that patch certainly isn't suitable for anything other than master. > > I'm also not really a big fan of the way I handled it there since it's > a pretty coarse way to do it. I would be happy to drop it if we can > feel comfortable that we got all of the callsites (now and in the > future) using your method. Perhaps we can just create a helper > function with this check builtin and then use it everywhere? That > might be enough to be confident about the future. > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On 31/08/2016 10:38, "Jesse Gross" <jesse@kernel.org> wrote: >On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto ><diproiettod@vmware.com> wrote: >> When translating a set action we also unwildcard the field in question. >> This is done to correctly translate set actions with the value identical >> to the ingress flow, like in the following example: >> >> flow table: >> >> tcp,actions=set_field:80->tcp_dst,output:5 >> >> ingress packet: >> >> ...,tcp,tcp_dst=80 >> >> datapath flow >> >> ...,tcp(dst=80) actions:5 >> >> The datapath flow must exact match the target field, because the actions >> do not include a set field. (Otherwise a packet coming in with a >> different tcp_dst would be matched, and its port wouldn't be altered). >> >> Tunnel attributes behave differently: at the datapath layer, before >> action processing they're cleared (we do the same at the ofproto layer >> in xlate_actions()). Therefore there's no need to unwildcard them, >> because a set action would always be detected (since we zero them at the >> beginning of xlate_ations()). >> >> This fixes a problem related to the handling of Geneve options. >> Unwildcarding non existing Geneve options (as done by a >> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel >> interface) would be problematic for the datapaths: the ODP translation >> functions cannot express a match on non existing Geneve options (unlike >> on other attributes), and the userspace datapath wouldn't be able to >> translate them to "userspace datapath format". In both cases the >> installed flow would be deleted by revalidation at the first >> opportunity. >> >> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >I think there might be some more obscure ways of triggering this >problem that still exist. Basically anything that can use a register >as a target is a potential issue. For example, stack_pop, bundle, and >multipath all look like they have the same masking behavior. You're right, thanks. I added two extra checks (bundle and multipath share part of the code). > >I do have a general solution in this patch (look at the bottom of >xlate_actions() where it is adjusting the wildcards): >http://openvswitch.org/pipermail/dev/2016-August/078484.html > >I didn't realize that it was addressing an existing issue though and >that patch certainly isn't suitable for anything other than master. Perhaps the commit message was too specific about Geneve options, because that's how I found this originally, but the issue applies also to other tunnel metadata (tunnel id, tunnel flags, ...) > >I'm also not really a big fan of the way I handled it there since it's >a pretty coarse way to do it. I would be happy to drop it if we can >feel comfortable that we got all of the callsites (now and in the >future) using your method. Perhaps we can just create a helper >function with this check builtin and then use it everywhere? That >might be enough to be confident about the future. I agree with you, I'd prefer to fix it on the set action, if possible. Thanks for you input!
On 31/08/2016 11:32, "Jarno Rajahalme" <jarno@ovn.org> wrote: >I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. > > Jarno Agreed, done Thanks, Daniele > >> On Aug 31, 2016, at 10:38 AM, Jesse Gross <jesse@kernel.org> wrote: >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto >> <diproiettod@vmware.com> wrote: >>> When translating a set action we also unwildcard the field in question. >>> This is done to correctly translate set actions with the value identical >>> to the ingress flow, like in the following example: >>> >>> flow table: >>> >>> tcp,actions=set_field:80->tcp_dst,output:5 >>> >>> ingress packet: >>> >>> ...,tcp,tcp_dst=80 >>> >>> datapath flow >>> >>> ...,tcp(dst=80) actions:5 >>> >>> The datapath flow must exact match the target field, because the actions >>> do not include a set field. (Otherwise a packet coming in with a >>> different tcp_dst would be matched, and its port wouldn't be altered). >>> >>> Tunnel attributes behave differently: at the datapath layer, before >>> action processing they're cleared (we do the same at the ofproto layer >>> in xlate_actions()). Therefore there's no need to unwildcard them, >>> because a set action would always be detected (since we zero them at the >>> beginning of xlate_ations()). >>> >>> This fixes a problem related to the handling of Geneve options. >>> Unwildcarding non existing Geneve options (as done by a >>> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel >>> interface) would be problematic for the datapaths: the ODP translation >>> functions cannot express a match on non existing Geneve options (unlike >>> on other attributes), and the userspace datapath wouldn't be able to >>> translate them to "userspace datapath format". In both cases the >>> installed flow would be deleted by revalidation at the first >>> opportunity. >>> >>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> >> I think there might be some more obscure ways of triggering this >> problem that still exist. Basically anything that can use a register >> as a target is a potential issue. For example, stack_pop, bundle, and >> multipath all look like they have the same masking behavior. >> >> I do have a general solution in this patch (look at the bottom of >> xlate_actions() where it is adjusting the wildcards): >> http://openvswitch.org/pipermail/dev/2016-August/078484.html >> >> I didn't realize that it was addressing an existing issue though and >> that patch certainly isn't suitable for anything other than master. >> >> I'm also not really a big fan of the way I handled it there since it's >> a pretty coarse way to do it. I would be happy to drop it if we can >> feel comfortable that we got all of the callsites (now and in the >> future) using your method. Perhaps we can just create a helper >> function with this check builtin and then use it everywhere? That >> might be enough to be confident about the future. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >
Does this patch need anything more? I don't see it on master and it's still in patchwork. Same thing for patch 4/5 in this series. On Thu, Sep 01, 2016 at 01:31:06AM +0000, Daniele Di Proietto wrote: > > On 31/08/2016 11:32, "Jarno Rajahalme" <jarno@ovn.org> wrote: > > >I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. > > > > Jarno > > Agreed, done > > Thanks, > > Daniele > > > > >> On Aug 31, 2016, at 10:38 AM, Jesse Gross <jesse@kernel.org> wrote: > >> > >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > >> <diproiettod@vmware.com> wrote: > >>> When translating a set action we also unwildcard the field in question. > >>> This is done to correctly translate set actions with the value identical > >>> to the ingress flow, like in the following example: > >>> > >>> flow table: > >>> > >>> tcp,actions=set_field:80->tcp_dst,output:5 > >>> > >>> ingress packet: > >>> > >>> ...,tcp,tcp_dst=80 > >>> > >>> datapath flow > >>> > >>> ...,tcp(dst=80) actions:5 > >>> > >>> The datapath flow must exact match the target field, because the actions > >>> do not include a set field. (Otherwise a packet coming in with a > >>> different tcp_dst would be matched, and its port wouldn't be altered). > >>> > >>> Tunnel attributes behave differently: at the datapath layer, before > >>> action processing they're cleared (we do the same at the ofproto layer > >>> in xlate_actions()). Therefore there's no need to unwildcard them, > >>> because a set action would always be detected (since we zero them at the > >>> beginning of xlate_ations()). > >>> > >>> This fixes a problem related to the handling of Geneve options. > >>> Unwildcarding non existing Geneve options (as done by a > >>> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel > >>> interface) would be problematic for the datapaths: the ODP translation > >>> functions cannot express a match on non existing Geneve options (unlike > >>> on other attributes), and the userspace datapath wouldn't be able to > >>> translate them to "userspace datapath format". In both cases the > >>> installed flow would be deleted by revalidation at the first > >>> opportunity. > >>> > >>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >> > >> I think there might be some more obscure ways of triggering this > >> problem that still exist. Basically anything that can use a register > >> as a target is a potential issue. For example, stack_pop, bundle, and > >> multipath all look like they have the same masking behavior. > >> > >> I do have a general solution in this patch (look at the bottom of > >> xlate_actions() where it is adjusting the wildcards): > >> http://openvswitch.org/pipermail/dev/2016-August/078484.html > >> > >> I didn't realize that it was addressing an existing issue though and > >> that patch certainly isn't suitable for anything other than master. > >> > >> I'm also not really a big fan of the way I handled it there since it's > >> a pretty coarse way to do it. I would be happy to drop it if we can > >> feel comfortable that we got all of the callsites (now and in the > >> future) using your method. Perhaps we can just create a helper > >> function with this check builtin and then use it everywhere? That > >> might be enough to be confident about the future. > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
I'm rebasing the series and considering a different approach for this. I'll send something shortly. Thanks, Daniele On 03/10/2016 17:38, "Ben Pfaff" <blp@ovn.org> wrote: >Does this patch need anything more? I don't see it on master and it's >still in patchwork. Same thing for patch 4/5 in this series. > >On Thu, Sep 01, 2016 at 01:31:06AM +0000, Daniele Di Proietto wrote: >> >> On 31/08/2016 11:32, "Jarno Rajahalme" <jarno@ovn.org> wrote: >> >> >I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. >> > >> > Jarno >> >> Agreed, done >> >> Thanks, >> >> Daniele >> >> > >> >> On Aug 31, 2016, at 10:38 AM, Jesse Gross <jesse@kernel.org> wrote: >> >> >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto >> >> <diproiettod@vmware.com> wrote: >> >>> When translating a set action we also unwildcard the field in question. >> >>> This is done to correctly translate set actions with the value identical >> >>> to the ingress flow, like in the following example: >> >>> >> >>> flow table: >> >>> >> >>> tcp,actions=set_field:80->tcp_dst,output:5 >> >>> >> >>> ingress packet: >> >>> >> >>> ...,tcp,tcp_dst=80 >> >>> >> >>> datapath flow >> >>> >> >>> ...,tcp(dst=80) actions:5 >> >>> >> >>> The datapath flow must exact match the target field, because the actions >> >>> do not include a set field. (Otherwise a packet coming in with a >> >>> different tcp_dst would be matched, and its port wouldn't be altered). >> >>> >> >>> Tunnel attributes behave differently: at the datapath layer, before >> >>> action processing they're cleared (we do the same at the ofproto layer >> >>> in xlate_actions()). Therefore there's no need to unwildcard them, >> >>> because a set action would always be detected (since we zero them at the >> >>> beginning of xlate_ations()). >> >>> >> >>> This fixes a problem related to the handling of Geneve options. >> >>> Unwildcarding non existing Geneve options (as done by a >> >>> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel >> >>> interface) would be problematic for the datapaths: the ODP translation >> >>> functions cannot express a match on non existing Geneve options (unlike >> >>> on other attributes), and the userspace datapath wouldn't be able to >> >>> translate them to "userspace datapath format". In both cases the >> >>> installed flow would be deleted by revalidation at the first >> >>> opportunity. >> >>> >> >>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> >> >> >> >> I think there might be some more obscure ways of triggering this >> >> problem that still exist. Basically anything that can use a register >> >> as a target is a potential issue. For example, stack_pop, bundle, and >> >> multipath all look like they have the same masking behavior. >> >> >> >> I do have a general solution in this patch (look at the bottom of >> >> xlate_actions() where it is adjusting the wildcards): >> >> http://openvswitch.org/pipermail/dev/2016-August/078484.html >> >> >> >> I didn't realize that it was addressing an existing issue though and >> >> that patch certainly isn't suitable for anything other than master. >> >> >> >> I'm also not really a big fan of the way I handled it there since it's >> >> a pretty coarse way to do it. I would be happy to drop it if we can >> >> feel comfortable that we got all of the callsites (now and in the >> >> future) using your method. Perhaps we can just create a helper >> >> function with this check builtin and then use it everywhere? That >> >> might be enough to be confident about the future. >> >> _______________________________________________ >> >> dev mailing list >> >> dev@openvswitch.org >> >> http://openvswitch.org/mailman/listinfo/dev >> > >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev
OK, thanks. I'll mark this series as "superseded" in patchwork. On Tue, Oct 04, 2016 at 12:42:19AM +0000, Daniele Di Proietto wrote: > I'm rebasing the series and considering a different approach for this. > > I'll send something shortly. > > Thanks, > > Daniele > > On 03/10/2016 17:38, "Ben Pfaff" <blp@ovn.org> wrote: > > >Does this patch need anything more? I don't see it on master and it's > >still in patchwork. Same thing for patch 4/5 in this series. > > > >On Thu, Sep 01, 2016 at 01:31:06AM +0000, Daniele Di Proietto wrote: > >> > >> On 31/08/2016 11:32, "Jarno Rajahalme" <jarno@ovn.org> wrote: > >> > >> >I’d put the registers and metadata field to the ‘false’ and also maybe non-writeable fields (ether type, frags, nw_proto, etc.) in to OVS_NOT_REACHED() case, just in case. > >> > > >> > Jarno > >> > >> Agreed, done > >> > >> Thanks, > >> > >> Daniele > >> > >> > > >> >> On Aug 31, 2016, at 10:38 AM, Jesse Gross <jesse@kernel.org> wrote: > >> >> > >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto > >> >> <diproiettod@vmware.com> wrote: > >> >>> When translating a set action we also unwildcard the field in question. > >> >>> This is done to correctly translate set actions with the value identical > >> >>> to the ingress flow, like in the following example: > >> >>> > >> >>> flow table: > >> >>> > >> >>> tcp,actions=set_field:80->tcp_dst,output:5 > >> >>> > >> >>> ingress packet: > >> >>> > >> >>> ...,tcp,tcp_dst=80 > >> >>> > >> >>> datapath flow > >> >>> > >> >>> ...,tcp(dst=80) actions:5 > >> >>> > >> >>> The datapath flow must exact match the target field, because the actions > >> >>> do not include a set field. (Otherwise a packet coming in with a > >> >>> different tcp_dst would be matched, and its port wouldn't be altered). > >> >>> > >> >>> Tunnel attributes behave differently: at the datapath layer, before > >> >>> action processing they're cleared (we do the same at the ofproto layer > >> >>> in xlate_actions()). Therefore there's no need to unwildcard them, > >> >>> because a set action would always be detected (since we zero them at the > >> >>> beginning of xlate_ations()). > >> >>> > >> >>> This fixes a problem related to the handling of Geneve options. > >> >>> Unwildcarding non existing Geneve options (as done by a > >> >>> set_field:X->tun_metadata<n> on a packet coming from a non-tunnel > >> >>> interface) would be problematic for the datapaths: the ODP translation > >> >>> functions cannot express a match on non existing Geneve options (unlike > >> >>> on other attributes), and the userspace datapath wouldn't be able to > >> >>> translate them to "userspace datapath format". In both cases the > >> >>> installed flow would be deleted by revalidation at the first > >> >>> opportunity. > >> >>> > >> >>> Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > >> >> > >> >> I think there might be some more obscure ways of triggering this > >> >> problem that still exist. Basically anything that can use a register > >> >> as a target is a potential issue. For example, stack_pop, bundle, and > >> >> multipath all look like they have the same masking behavior. > >> >> > >> >> I do have a general solution in this patch (look at the bottom of > >> >> xlate_actions() where it is adjusting the wildcards): > >> >> http://openvswitch.org/pipermail/dev/2016-August/078484.html > >> >> > >> >> I didn't realize that it was addressing an existing issue though and > >> >> that patch certainly isn't suitable for anything other than master. > >> >> > >> >> I'm also not really a big fan of the way I handled it there since it's > >> >> a pretty coarse way to do it. I would be happy to drop it if we can > >> >> feel comfortable that we got all of the callsites (now and in the > >> >> future) using your method. Perhaps we can just create a helper > >> >> function with this check builtin and then use it everywhere? That > >> >> might be enough to be confident about the future. > >> >> _______________________________________________ > >> >> dev mailing list > >> >> dev@openvswitch.org > >> >> http://openvswitch.org/mailman/listinfo/dev > >> > > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> http://openvswitch.org/mailman/listinfo/dev
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h index 23f9916..f21df4b 100644 --- a/include/openvswitch/meta-flow.h +++ b/include/openvswitch/meta-flow.h @@ -2094,6 +2094,7 @@ void mf_set_flow_value_masked(const struct mf_field *, const union mf_value *mask, struct flow *); bool mf_is_tun_metadata(const struct mf_field *); +bool mf_set_must_mask(const struct mf_field *); bool mf_is_set(const struct mf_field *, const struct flow *); void mf_mask_field(const struct mf_field *, struct flow_wildcards *); void mf_mask_field_masked(const struct mf_field *, const union mf_value *mask, diff --git a/lib/meta-flow.c b/lib/meta-flow.c index 3dc2770..86a0f03 100644 --- a/lib/meta-flow.c +++ b/lib/meta-flow.c @@ -1407,6 +1407,97 @@ mf_is_tun_metadata(const struct mf_field *mf) mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; } +/* Returns true if a field 'mf' should be exact matched before being set + * by the action translation, false otherwise. Most of the fields need + * an exact match.*/ +bool +mf_set_must_mask(const struct mf_field *mf) +{ + /* Tunnel attributes don't need an exact match, because they are + * cleared by the datapath between ingress and egress. Also, an + * exact match on tunnel metadata might be problematic, because + * it is not possible to express it if the metadata didn't exist + * on ingress. */ + switch (mf->id) { + case MFF_TUN_ID: + case MFF_TUN_SRC: + case MFF_TUN_DST: + case MFF_TUN_IPV6_SRC: + case MFF_TUN_IPV6_DST: + case MFF_TUN_FLAGS: + case MFF_TUN_GBP_ID: + case MFF_TUN_GBP_FLAGS: + case MFF_TUN_TOS: + case MFF_TUN_TTL: + CASE_MFF_TUN_METADATA: + return false; + + case MFF_DP_HASH: + case MFF_RECIRC_ID: + case MFF_CONJ_ID: + case MFF_METADATA: + case MFF_IN_PORT: + case MFF_IN_PORT_OXM: + case MFF_ACTSET_OUTPUT: + case MFF_SKB_PRIORITY: + case MFF_PKT_MARK: + case MFF_CT_STATE: + case MFF_CT_ZONE: + case MFF_CT_MARK: + case MFF_CT_LABEL: + CASE_MFF_REGS: + CASE_MFF_XREGS: + CASE_MFF_XXREGS: + case MFF_ETH_SRC: + case MFF_ETH_DST: + case MFF_ETH_TYPE: + case MFF_VLAN_TCI: + case MFF_DL_VLAN: + case MFF_VLAN_VID: + case MFF_DL_VLAN_PCP: + case MFF_VLAN_PCP: + case MFF_MPLS_LABEL: + case MFF_MPLS_TC: + case MFF_MPLS_BOS: + case MFF_MPLS_TTL: + case MFF_IPV4_SRC: + case MFF_ARP_SPA: + case MFF_IPV4_DST: + case MFF_ARP_TPA: + case MFF_IPV6_SRC: + case MFF_IPV6_DST: + case MFF_IPV6_LABEL: + case MFF_IP_PROTO: + case MFF_IP_DSCP: + case MFF_IP_DSCP_SHIFTED: + case MFF_IP_ECN: + case MFF_IP_TTL: + case MFF_IP_FRAG: + case MFF_ARP_OP: + case MFF_ARP_SHA: + case MFF_ND_SLL: + case MFF_ARP_THA: + case MFF_ND_TLL: + case MFF_TCP_SRC: + case MFF_UDP_SRC: + case MFF_SCTP_SRC: + case MFF_ICMPV4_TYPE: + case MFF_ICMPV6_TYPE: + case MFF_TCP_DST: + case MFF_UDP_DST: + case MFF_SCTP_DST: + case MFF_ICMPV4_CODE: + case MFF_ICMPV6_CODE: + case MFF_TCP_FLAGS: + case MFF_ND_TARGET: + return true; + + case MFF_N_IDS: + default: + OVS_NOT_REACHED(); + } +} + /* Returns true if 'mf' has previously been set in 'flow', false if * it contains a non-default value. * @@ -1988,7 +2079,9 @@ mf_subfield_copy(const struct mf_subfield *src, if (mf_are_prereqs_ok(dst->field, flow, wc) && mf_are_prereqs_ok(src->field, flow, wc)) { unwildcard_subfield(src, wc); - unwildcard_subfield(dst, wc); + if (mf_set_must_mask(dst->field)) { + unwildcard_subfield(dst, wc); + } union mf_value src_value; union mf_value dst_value; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 893c033..0118d01 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4924,7 +4924,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, /* Set the field only if the packet actually has it. */ if (mf_are_prereqs_ok(mf, flow, wc)) { - mf_mask_field_masked(mf, &set_field->mask, wc); + if (mf_set_must_mask(mf)) { + mf_mask_field_masked(mf, &set_field->mask, wc); + } mf_set_flow_value_masked(mf, &set_field->value, &set_field->mask, flow); }
When translating a set action we also unwildcard the field in question. This is done to correctly translate set actions with the value identical to the ingress flow, like in the following example: flow table: tcp,actions=set_field:80->tcp_dst,output:5 ingress packet: ...,tcp,tcp_dst=80 datapath flow ...,tcp(dst=80) actions:5 The datapath flow must exact match the target field, because the actions do not include a set field. (Otherwise a packet coming in with a different tcp_dst would be matched, and its port wouldn't be altered). Tunnel attributes behave differently: at the datapath layer, before action processing they're cleared (we do the same at the ofproto layer in xlate_actions()). Therefore there's no need to unwildcard them, because a set action would always be detected (since we zero them at the beginning of xlate_ations()). This fixes a problem related to the handling of Geneve options. Unwildcarding non existing Geneve options (as done by a set_field:X->tun_metadata<n> on a packet coming from a non-tunnel interface) would be problematic for the datapaths: the ODP translation functions cannot express a match on non existing Geneve options (unlike on other attributes), and the userspace datapath wouldn't be able to translate them to "userspace datapath format". In both cases the installed flow would be deleted by revalidation at the first opportunity. Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> --- include/openvswitch/meta-flow.h | 1 + lib/meta-flow.c | 95 ++++++++++++++++++++++++++++++++++++++++- ofproto/ofproto-dpif-xlate.c | 4 +- 3 files changed, 98 insertions(+), 2 deletions(-)