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 > ------------------------------------------------------------------------------ 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
