Hello Iwase, Thank you again for the feedback!
We can comment out the assert here: https://github.com/osrg/ryu/blob/master/ryu/lib/packet/cfm.py#L271 however it is possible that another assert will just cause a similar issue later on. Long term it might be a good idea to consider moving away from asserts like that entirely. I think that the "host_discovery_packet_in_handler" is actually made a bit more simple this way because of the fact that the handler really only cares about packets of very specific ethertypes like "ETH_TYPE_ARP", "ETH_TYPE_IP", or "ETH_TYPE_IPV6". I believe that this solution might be more robust and efficient because it ensures that the "host_discovery_packet_in_handler" will only process packets that it needs to. Also, since the packets are now being parsed as ethernet frames first, it is made very apparent that actions will only be made on packets of specific ethertypes. In our environment we are getting CCM packets with interval=0 from a vendor specific implementation that happens to touch the networks we operate ryu on-- it appears that they do not follow the IEEE standard in their CCM implementation unfortunately. Patch v3 should be the latest version of the patch, I am not sure that I submitted it correctly as it is not in this thread. Thanks, /Evan On Thu, May 19, 2016 at 8:16 PM, Iwase Yusuke <iwase.yusu...@gmail.com> wrote: > Hi, > > Thank you for updating the patches! > > > On 2016年05月20日 07:11, Evan Gray wrote: >> >> Hello Iwase, >> >> Thank you for the feedback! >> >> You are correct, this will parse the "msg.data" as an ethernet frame >> only and I believe parsing "msg.data" as an ethernet frame before >> parsing it further can potentially be a more robust solution. >> >> We are trying to avoid causing "AssertionError" like the one here: >> https://github.com/osrg/ryu/blob/master/ryu/lib/packet/cfm.py#L271 > > > Could we comment out this assert statement instead? > Is it not appropriate? > (Indeed, IEEE standard (802.1ag - 2007) say "interval" field does not > contain the value 0 though...) > > I think if we can comment out this assert statement with some "Note:" > comments, > we can keep "host_discovery_packet_in_handler" simple. > > BTW, why did you get CCM with interval=0? Is is unavoidable in your > environment? > > >> >> Today since the "host_discovery_packet_in_handler" parses any >> "msg.data" it sees with "packet.Packet()", there is a real possibility >> of causing an exception or crash like the one that we encountered >> caused by CFM packets with an interval of 0. Causing exceptions like >> that to be raised can even slow down the execution of the rest of the >> application due to exceptions locking the entire process. >> >> In the "host_discovery_packet_in_handler" it appears that the parsed >> packet data is not needed until much later in the handler where it >> might not even need to be parsed at all depending on the ethertype of >> the frame here: >> https://github.com/osrg/ryu/blob/master/ryu/topology/switches.py#L870-L883 >> >> After reading your feedback and thinking about this a little bit more, >> I believe that we should delay parsing "msg.data" as more than an >> ethernet frame until just before the parsed version of the packet is >> needed and we know that the contents of "msg.data" are of an ethertype >> that "host_discovery_packet_in_handler" cares about like >> "ETH_TYPE_ARP", "ETH_TYPE_IP", or "ETH_TYPE_IPV6". >> >> I will submit a revised patch including these changes. >> >> Thank you, >> >> /Evan >> >> >> On Wed, May 18, 2016 at 9:16 PM, Iwase Yusuke <iwase.yusu...@gmail.com> >> wrote: >>> >>> >>> Hi, >>> >>> Pardon me for jumping in. >>> >>> >>> >>> On 2016年05月19日 10:30, evanscottg...@gmail.com wrote: >>>> >>>> >>>> From: Evan Gray <evanscottg...@gmail.com> >>>> >>>> This commit will allow the host_discovery_packet_in_handler to ignore >>>> invalid >>>> cfm packets. Ryu will fail to parse cfm packets with an interval of 0 -- >>>> We >>>> discovered this when one of our systems sent cfm packets with an >>>> interval of 0. >>>> >>>> Signed-off-by: Evan Gray <evanscottg...@gmail.com> >>>> --- >>>> ryu/topology/switches.py | 20 +++++++++----------- >>>> 1 file changed, 9 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/ryu/topology/switches.py b/ryu/topology/switches.py >>>> index 333407a..7df3220 100644 >>>> --- a/ryu/topology/switches.py >>>> +++ b/ryu/topology/switches.py >>>> @@ -31,8 +31,8 @@ from ryu.lib.dpid import dpid_to_str, str_to_dpid >>>> from ryu.lib.port_no import port_no_to_str >>>> from ryu.lib.packet import packet, ethernet >>>> from ryu.lib.packet import lldp, ether_types >>>> -from ryu.lib.packet import arp, ipv4, ipv6 >>>> from ryu.ofproto.ether import ETH_TYPE_LLDP >>>> +from ryu.ofproto.ether import ETH_TYPE_CFM >>>> from ryu.ofproto import nx_match >>>> from ryu.ofproto import ofproto_v1_0 >>>> from ryu.ofproto import ofproto_v1_2 >>>> @@ -832,11 +832,10 @@ class Switches(app_manager.RyuApp): >>>> @set_ev_cls(ofp_event.EventOFPPacketIn, MAIN_DISPATCHER) >>>> def host_discovery_packet_in_handler(self, ev): >>>> msg = ev.msg >>>> - pkt = packet.Packet(msg.data) >>>> - eth = pkt.get_protocols(ethernet.ethernet)[0] >>>> + eth, pkt_type, pkt_data = ethernet.ethernet.parser(msg.data) >>> >>> >>> >>> "packet.Packet()" will parse the entire packet data while estimating >>> protocol types >>> and stores the parsed packet instance into "pkt.protocols". >>> https://github.com/osrg/ryu/blob/master/ryu/lib/packet/packet.py#L24 >>> >>> If we change to "ethernet.ethernet.parser()", "msg.data" will be parsed >>> as the >>> ethernet frame only. >>> >>> >>>> >>>> - # ignore lldp packet >>>> - if eth.ethertype == ETH_TYPE_LLDP: >>>> + # ignore lldp and cfm packets >>>> + if eth.ethertype in (ETH_TYPE_LLDP, ETH_TYPE_CFM): >>>> return >>>> >>>> datapath = msg.datapath >>>> @@ -865,21 +864,20 @@ class Switches(app_manager.RyuApp): >>>> ev = event.EventHostAdd(host) >>>> self.send_event_to_observers(ev) >>>> >>>> + pkt = pkt_type.parser(pkt_data) >>>> + >>> >>> >>> >>> As I mentioned above, "packet.Packet()" have already parsed the packet >>> data. >>> I think we need not parse "pkt_data here". >>> >>> And, if "pkt_data" is the malformed data, "pkt_type.parser()" can raise >>> an exception, >>> so this may cause this App crash. >>> >>> >>> Thanks, >>> Iwase >>> >>> >>>> # arp packet, update ip address >>>> if eth.ethertype == ether_types.ETH_TYPE_ARP: >>>> - arp_pkt = pkt.get_protocols(arp.arp)[0] >>>> - self.hosts.update_ip(host, ip_v4=arp_pkt.src_ip) >>>> + self.hosts.update_ip(host, ip_v4=pkt.src_ip) >>>> >>>> # ipv4 packet, update ipv4 address >>>> elif eth.ethertype == ether_types.ETH_TYPE_IP: >>>> - ipv4_pkt = pkt.get_protocols(ipv4.ipv4)[0] >>>> - self.hosts.update_ip(host, ip_v4=ipv4_pkt.src) >>>> + self.hosts.update_ip(host, ip_v4=pkt.src) >>>> >>>> # ipv6 packet, update ipv6 address >>>> elif eth.ethertype == ether_types.ETH_TYPE_IPV6: >>>> # TODO: need to handle NDP >>>> - ipv6_pkt = pkt.get_protocols(ipv6.ipv6)[0] >>>> - self.hosts.update_ip(host, ip_v6=ipv6_pkt.src) >>>> + self.hosts.update_ip(host, ip_v6=pkt.src) >>>> >>>> def send_lldp_packet(self, port): >>>> try: >>>> >> >> >> ------------------------------------------------------------------------------ >> Mobile security can be enabling, not merely restricting. Employees who >> bring their own devices (BYOD) to work are irked by the imposition of MDM >> restrictions. Mobile Device Manager Plus allows you to control only the >> apps on BYO-devices by containerizing them, leaving personal data >> untouched! >> https://ad.doubleclick.net/ddm/clk/304595813;131938128;j >> _______________________________________________ >> Ryu-devel mailing list >> Ryu-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/ryu-devel >> > ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ Ryu-devel mailing list Ryu-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ryu-devel
