> On Mar 22, 2017, at 9:05 PM, Numan Siddique <[email protected]> wrote: > > > > On Thu, Mar 23, 2017 at 5:19 AM, Jarno Rajahalme <[email protected] > <mailto:[email protected]>> wrote: > On Mar 22, 2017, at 1:49 PM, Jarno Rajahalme <[email protected] > <mailto:[email protected]>> wrote: > > > > > >> On Mar 22, 2017, at 1:43 PM, Ben Pfaff <[email protected] > >> <mailto:[email protected]>> wrote: > >> > >> On Wed, Mar 22, 2017 at 01:40:10PM -0700, Jarno Rajahalme wrote: > >>> > >>>> On Mar 22, 2017, at 1:21 PM, Ben Pfaff <[email protected] > >>>> <mailto:[email protected]>> wrote: > >>>> > >>>> On Wed, Mar 22, 2017 at 09:30:36PM +0530, [email protected] > >>>> <mailto:[email protected]> wrote: > >>>>> From: Numan Siddique <[email protected] <mailto:[email protected]>> > >>>>> > >>>>> 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
