[ovs-dev] [PATCH v2 1/5] ofproto-dpif-xlate: Don't unwildcard tunnel attributes on set.

[Ben Pfaff]

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 at 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 at 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 at kernel.org> wrote:
> >> >> 
> >> >> On Tue, Aug 30, 2016 at 6:47 PM, Daniele Di Proietto
> >> >> <diproiettod at 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 at 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 at openvswitch.org
> >> >> http://openvswitch.org/mailman/listinfo/dev
> >> >
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev