Hello Iwase, We are still experiencing the issues that this patch is intended to address-- have you had a moment to look at Patch v3 and the previous message in this thread?
Here are the patches that I am referring to: - https://sourceforge.net/p/ryu/mailman/message/35102100/ - https://sourceforge.net/p/ryu/mailman/message/35102101/ Thanks, /Evan On Tue, May 31, 2016 at 2:55 PM, Evan Gray <evanscottg...@gmail.com> wrote: > 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 >>> >> ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape _______________________________________________ Ryu-devel mailing list Ryu-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ryu-devel
