On Tue, Apr 01, 2014 at 12:21:03PM +0900, Simon Horman wrote:
> On Thu, Mar 20, 2014 at 10:05:44AM -0700, Ben Pfaff wrote:
> > On Fri, Mar 14, 2014 at 04:19:52PM +0900, Simon Horman wrote:
> > > When creating a flow in the datapath as the result of an upcall
> > > the match itself is the match supplied in the upcall while
> > > the mask of the match, if supplied, is generated based on the
> > > flow and mask composed during action translation.
> > >
> > > In the case of, for example a UDP packet, the match will include
> > > of L2, L3 and L4 fields. However, if the flow is cleared in
> > > flow_push_mpls() then the mask that is synthesised from it will
> > > not include L3 and L4 fields. This seems incorrect and the kernel
> > > datapath complains about this mismatch.
> > >
> > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >
> > The goal of clearing the fields is to ensure that later flow tables
> > can't match on fields that aren't visible anymore. That's important for
> > accurate OpenFlow implementation, so I'd rather not change it. On the
> > other hand, I see the point you're making, but I don't immediately
> > understand why it happens that way. After all, I can change fields with
> > OpenFlow actions and the datapath flows work out OK, why doesn't this
> > work out OK too? Do you understand the reason?
>
> Hi Ben,
>
> I have taken a closer look at this and I think that I now understand the
> problem. I would like to propose the following:
Hi Ben,
I'd be most grateful if you could take another moment to look over this.
>
>
> From: Simon Horman <ho...@verge.net.au>
>
> odp-util: Do not set port mask of non-IP packets
>
> In the case that an flow for an IP packet has an mpls_push action applied
> the L3 and L4 portions of the flow will be cleared in flow_push_mpls().
>
> Without this change commit_set_port_action() will set the tp_src and tp_dst
> mask for the flow to all-ones because the base and flow port values no
> longer match. Even though there will be no corresponding set action for the
> ports; because the flow is no longer IP.
>
> In this case where nw_proto is not part of the match this manifests
> in a problem because the userspace datapath rejects flows whose masks
> have non-zero values for tp_src or dp_dst if the nw_proto mask is
> not all-ones.
>
> This patch resolves this problem by having commit_set_port_action() return
> without doing anything if flow->nw_proto is zero. The same logic is present
> in commit_set_nw_action().
>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 956fef1..b40f9bc 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -3778,6 +3778,11 @@ static void
> commit_set_port_action(const struct flow *flow, struct flow *base,
> struct ofpbuf *odp_actions, struct flow_wildcards *wc)
> {
> + /* Check if 'flow' really has an L3 header. */
> + if (!flow->nw_proto) {
> + return;
> + }
> +
> if (!is_ip_any(base) || (!base->tp_src && !base->tp_dst)) {
> return;
> }
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
