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 <je...@kernel.org> wrote: > > 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev