Hi Evan, I'm very sorry for delay.
On 2016年06月29日 04:36, Evan Gray wrote: > 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/ I have no comments for your v3 Patch. It looks good to me. Thanks, Iwase > > 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 > ------------------------------------------------------------------------------ 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
