Hi KK The patch looks right. I will push that to SNAC too. Thanks Srini.
On Fri, Jan 14, 2011 at 10:47 AM, kk yap <[email protected]> wrote: > Okay, I think the high level point is we should expose malformed > packets and let others decide how to handle it. Can someone review > this patch before I push? > > Regards > KK > > On 14 January 2011 01:25, Rob Sherwood <[email protected]> wrote: >> Fwiw, I agree with what both Masa and KK. >> >> Masa's point: OFPP_TABLE shouldn't be a special case: i.e., it should >> be able to generate packet_in's on the second pss >> >> KK's point: this should be more explicitly called out in the spec. >> >> I think if you were to suggest a specific wording in the next week, >> this could still make it into the 1.1 spec. >> >> - Rob >> . >> >> >> >> On Thu, Jan 13, 2011 at 7:06 PM, Masayoshi Kobayashi >> <[email protected]> wrote: >>> KK, >>> >>> I think the implementer will read the spec the other way around. >>> Spec requires nothing special about OFPP_TABLE action (it does not say >>> "don't generate pkt_in, if there is no match"). So the switch >>> just follows the default behavior, i.e., pkt_in will be generated. >>> >>> I would expect the reference design also does the same. >>> >>> Masa >>> >>> On 01/13/2011 06:38 PM, kk yap wrote: >>>>> >>>>> Because the action of pkt_out is "OFPP_TABLE". >>>>> (the packet in pkt_out does not match to the entry that is installed by >>>>> flow_mod, since the matching entry says nw_proto=0). >>>> >>>> Is there anything in the spec that says that the switch should send >>>> another packet-in if there is no matching entry for a OFPP_TABLE? I >>>> am failing to find that. Can someone point to why this behavior is >>>> specified by the spec? >>>> >>>> Regards >>>> KK >>>> >>>> On 13 January 2011 18:28, Masayoshi Kobayashi<[email protected]> >>>> wrote: >>>>> >>>>> KK, >>>>> >>>>>> I thought about it a little. Why is the switch doing step 7? >>>>> >>>>> Because the action of pkt_out is "OFPP_TABLE". >>>>> (the packet in pkt_out does not match to the entry that is installed by >>>>> flow_mod, since the matching entry says nw_proto=0). >>>>> >>>>> Masa >>>>> >>>>> On 01/13/2011 06:06 PM, kk yap wrote: >>>>>> >>>>>> Hi Srini, >>>>>> >>>>>> I thought about it a little. Why is the switch doing step 7? >>>>>> >>>>>> Anyway, I do agree that NOX is not handling malformed packets right. >>>>>> I have included an invalid field in the struct flow, and created a >>>>>> component that ignore invalid packets. If anyone will double-check >>>>>> and test the patches attached, I will push it. >>>>>> >>>>>> Regards >>>>>> KK >>>>>> >>>>>> On 13 January 2011 16:13, Srini Seetharaman<[email protected]> >>>>>> wrote: >>>>>>> >>>>>>> I explained this to KK in person. For others, here is the sequence of >>>>>>> events: >>>>>>> >>>>>>> 1. Packet arrives with (nw_proto=6, tp_src=0, tp_dst=0). Store in bufid >>>>>>> 'X' >>>>>>> 2. flow.cc identifies that the arrived TCP packet is corrupted, and >>>>>>> generates >>>>>>> pkt_in event with flow structure having (nw_proto=0, tp_src=0, >>>>>>> tp_dst=0) >>>>>>> 3. Authenticator generates a flow_in with flow_in.flow being same as >>>>>>> above >>>>>>> 3. routing.cc generates a flow_mod for the flow_in with the match >>>>>>> pattern >>>>>>> defined using the fields of the flow_in.flow >>>>>>> 4. Switch inserts a flow table entry for matching (nw_proto=0, >>>>>>> tp_src=0, tp_dst=0) >>>>>>> 5. routing.cc generates a pkt_out for the bufid 'X' with action = >>>>>>> OFPP_TABLE >>>>>>> 6. Switch notices that the packet in bufid 'X' has no matching flow >>>>>>> table >>>>>>> entry, >>>>>>> because there is a mismatch on the nw_proto field >>>>>>> 7. Switch generates a new pkt_in event >>>>>>> 8. Go to step (2) >>>>>>> >>>>>>> This is the infinite loop. >>>>>>> >>>>>>> Srini. >>>>>>> >>>>>>> On Thu, Jan 13, 2011 at 1:08 PM, kk yap<[email protected]> wrote: >>>>>>>> >>>>>>>> Hi Srini, >>>>>>>> >>>>>>>> I think you are fixing this in the wrong place. Putting nw_proto=0 >>>>>>>> does not cause an infinite loop. Where is the loop happening? Can >>>>>>>> you provide more detailed NOX output so that we can even start looking >>>>>>>> at this. >>>>>>>> >>>>>>>> Regards >>>>>>>> KK >>>>>>>> >>>>>>>> On 13 January 2011 11:02, Srini Seetharaman<[email protected]> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> We don't know who sent it, but it came from outside our network. If >>>>>>>>> it >>>>>>>>> is easy to take down a network by just sending 1 invalid packet, I'd >>>>>>>>> be worried! >>>>>>>>> >>>>>>>>> On Thu, Jan 13, 2011 at 10:59 AM, kk yap<[email protected]> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Srini, >>>>>>>>>> >>>>>>>>>> What is this packet? The length of TCP is zero?!?! I wish to >>>>>>>>>> understand the circumstance for which we are getting the packet >>>>>>>>>> before >>>>>>>>>> commenting on the right way to handle this. >>>>>>>>>> >>>>>>>>>> Regards >>>>>>>>>> KK >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 13 January 2011 10:38, Srini Seetharaman<[email protected]> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> When someone sends the attached packet to a switch, it generates an >>>>>>>>>>> infinite loop of packet_ins in our production network. This is >>>>>>>>>>> because >>>>>>>>>>> this incoming tcp packet has nw_proto=6 and tcp port numbers of >>>>>>>>>>> "0", >>>>>>>>>>> but outgoing flow_mod has nw_proto of "0" and tcp port numbers of >>>>>>>>>>> "0". >>>>>>>>>>> So, the packet_out generates a new packet_in and this loop >>>>>>>>>>> continues >>>>>>>>>>> forever. >>>>>>>>>>> >>>>>>>>>>> I see the following code in src/lib/flow.cc (both in NOX-Zaku and >>>>>>>>>>> SNAC). I believe this is what is causing the nw_proto to be "0" in >>>>>>>>>>> the >>>>>>>>>>> flow_mod. I'm not sure who wrote that piece of code. This is not >>>>>>>>>>> handling corrupted packets well and rejecting this packet as a >>>>>>>>>>> invalid >>>>>>>>>>> TCP packet. Does anyone see problems with removing the "else" >>>>>>>>>>> clause? >>>>>>>>>>> >>>>>>>>>>> if (nw_proto == ip_::proto::TCP) { >>>>>>>>>>> const tcp_header *tcp = pull_tcp(b); >>>>>>>>>>> if (tcp) { >>>>>>>>>>> tp_src = tcp->tcp_src; >>>>>>>>>>> tp_dst = tcp->tcp_dst; >>>>>>>>>>> } else { >>>>>>>>>>> /* Avoid tricking other code into thinking that >>>>>>>>>>> * this packet has an L4 header. */ >>>>>>>>>>> nw_proto = 0; >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> FYI, pull_tcp is defined as below: >>>>>>>>>>> static const tcp_header * pull_tcp(Buffer& b) >>>>>>>>>>> { >>>>>>>>>>> if (const tcp_header *tcp = b.try_at<tcp_header>(0)) { >>>>>>>>>>> int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4; >>>>>>>>>>> if (tcp_len>= sizeof *tcp) { >>>>>>>>>>> return reinterpret_cast<const >>>>>>>>>>> tcp_header*>(b.try_pull(tcp_len)); >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> return 0; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> nox-dev mailing list >>>>>>>>>>> [email protected] >>>>>>>>>>> http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> _______________________________________________ >>>>>>> nox-dev mailing list >>>>>>> [email protected] >>>>>>> http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org >>>>> >>>>> -- >>>>> Masayoshi Kobayashi >>>>> Visiting Scholar at Stanford University >>>>> System Platform Research Labs, NEC Corporation. >>>>> T/M: 650-714-3154 >>>>> >>>>> _______________________________________________ >>>>> nox-dev mailing list >>>>> [email protected] >>>>> http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org >>>>> >>> >>> -- >>> Masayoshi Kobayashi >>> Visiting Scholar at Stanford University >>> System Platform Research Labs, NEC Corporation. >>> T/M: 650-714-3154 >>> >>> _______________________________________________ >>> nox-dev mailing list >>> [email protected] >>> http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org >>> >> > > _______________________________________________ > nox-dev mailing list > [email protected] > http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org > > _______________________________________________ nox-dev mailing list [email protected] http://noxrepo.org/mailman/listinfo/nox-dev_noxrepo.org
