On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto <diproiet...@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 <diproiet...@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
