[ovs-dev] [PATCH] Decode the NXT_RESUME message with a loose match.

[Jarno Rajahalme]

> On Mar 22, 2017, at 9:05 PM, Numan Siddique <nusiddiq at redhat.com> wrote:
> 
> 
> 
> On Thu, Mar 23, 2017 at 5:19 AM, Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>> wrote:
> On Mar 22, 2017, at 1:49 PM, Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>> wrote:
> >
> >
> >> On Mar 22, 2017, at 1:43 PM, Ben Pfaff <blp at ovn.org <mailto:blp at ovn.org>> wrote:
> >>
> >> On Wed, Mar 22, 2017 at 01:40:10PM -0700, Jarno Rajahalme wrote:
> >>>
> >>>> On Mar 22, 2017, at 1:21 PM, Ben Pfaff <blp at ovn.org <mailto:blp at ovn.org>> wrote:
> >>>>
> >>>> On Wed, Mar 22, 2017 at 09:30:36PM +0530, nusiddiq at redhat.com <mailto:nusiddiq at redhat.com> wrote:
> >>>>> From: Numan Siddique <nusiddiq at redhat.com <mailto:nusiddiq at redhat.com>>
> >>>>>
> >>>>> When ovs-vswitchd sends the NX_PACKET_IN2 message, it may not
> >>>>> encode ETH_TYPE of the packet. And with the commit daf4d3c18da
> >>>>> ("odp: Support conntrack orig tuple key."), the conntrack fields
> >>>>> are encoded, if set. After the commit 7befb20d0f70
> >>>>> ("nx-match: Fix oxm decode."), ovs-vswitchd is sending OFPBMC_BAD_PREREQ
> >>>>> message to the controller for the resumed packets (having conntract
> >>>>> fields).
> >>>>>
> >>>>> With the loose match criteria set to false when
> >>>>> ofputil_decode_packet_in_private() is called, the prerequisite check
> >>>>> for the mf field "ct_nw_src" is failing as ETH_TYPE is not set.
> >>>>
> >>>> The original design for NXT_RESUME was that the switch would only be
> >>>> decoding a flow that it had itself decoded and therefore any failure to
> >>>> decode is a bug.  I don't think this design has changed (although I'm
> >>>> happy to be corrected) so you may be pointing out a bug that we should
> >>>> fix by fixing the flow encoder or decoder.
> >>>
> >>> OVS still gets back only (metadata) fields that it itself sent. The issue is that ETH_TYPE is not metadata, hence it is not encoded as part of metadata. The root of the problem is that I made the conntrack original direction tuple (which is metadata) to have the packet’s ether type as a prerequisite. Even if the controller would send down exactly the same metadata as OVS sent out, the controller may still be allowed to do whatever with the packet, for example, add MPLS headers, thus changing the packet to not be an IP packet any more. In this scenario the conntrack original direction tuple (and conntrack state in general) becomes suspect, as an MPLS packet is untrackable by conntrack.
> >>>
> >>> Maybe we should make the NXT_RESUME decoder clear out conntrack metadata if the packet is not an IP packet (anymore), or if IPv4 metadata is present on an IPv6 packet or the other way around? To do this we could make the decoding loose as proposed here, but then explicitly check the packet type and clear the conntrack metadata if needed. No other metadata has any prerequisites, and if the controller happened to add metadata that OVS does not understand it might be OK to ignore those.
> >>
> >> Do we think that the prerequisite on the conntrack metadata is a
> >> valuable one?  If it is not, then it would be simple to eliminate the
> >> prerequisite.
> >
> > I’ll look into relaxing that. However, for the kernel datapath this prerequisite is essential, and we need to be sure we are not sending down flows that match on conntrack original direction tuple for non-IP or mismatched IP packets.
> >
> 
> I just posted a patch:
> 
> https://patchwork.ozlabs.org/patch/742385/ <https://patchwork.ozlabs.org/patch/742385/>
> 
> 
> 
> Thanks for the patch. I tested it and it is fixing the issue.
> 

Thanks for testing! Maybe Ben could look over the general approach in this patch?

  Jarno

> Numan
>  
> >  Jarno
> >