On Thu, Mar 23, 2017 at 5:19 AM, Jarno Rajahalme <[email protected]> wrote:
> On Mar 22, 2017, at 1:49 PM, Jarno Rajahalme <[email protected]> wrote: > > > > > >> On Mar 22, 2017, at 1:43 PM, Ben Pfaff <[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]> wrote: > >>>> > >>>> On Wed, Mar 22, 2017 at 09:30:36PM +0530, [email protected] wrote: > >>>>> From: Numan Siddique <[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/ > > Thanks for the patch. I tested it and it is fixing the issue. Numan > > Jarno > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
