[ovs-dev] bridge: fix crash when more than 32767 ports added

Message ID 1522290836-52146-1-git-send-email-lidejun1@huawei.com
State New
Headers show
Series
  • [ovs-dev] bridge: fix crash when more than 32767 ports added
Related show

Commit Message

lidejun1@huawei.com March 29, 2018, 2:33 a.m.
From: l00397770 <lidejun1@huawei.com>

When creating a port using ovs-vsctl, its Openflow port number is automatically
assigned, and the max value is 32767. After adding 32767 ports, subsequent ports'
number are all 65535, and they are all inserted into a bridge's ifaces hmap,
but when these ports are deleted, their hamp_nodes are not removed by iface_destroy__,
this can cause ovs crash in the following call path:
bridge_reconfigure ---> bridge_add_ports ---> bridge_add_ports__ ---> iface_create
---> hmap_insert_at ---> hmap_expand_at ---> resize ---> hmap_insert_fast,
the bridge's ifaces hmap bucket is corrupted.

To fix the above issue, we check Openflow port number in iface_do_create: if the port
number is 65535, report an error and do not add this port into a bridge.

Signed-off-by: lidejun1@huawei.com
Acked-By: jerry.lilijun@huawei.com
---
 vswitchd/bridge.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ben Pfaff April 4, 2018, 5:27 p.m. | #1
On Thu, Mar 29, 2018 at 10:33:56AM +0800, lidejun1@huawei.com wrote:
> From: l00397770 <lidejun1@huawei.com>
> 
> When creating a port using ovs-vsctl, its Openflow port number is automatically
> assigned, and the max value is 32767. After adding 32767 ports, subsequent ports'
> number are all 65535, and they are all inserted into a bridge's ifaces hmap,
> but when these ports are deleted, their hamp_nodes are not removed by iface_destroy__,
> this can cause ovs crash in the following call path:
> bridge_reconfigure ---> bridge_add_ports ---> bridge_add_ports__ ---> iface_create
> ---> hmap_insert_at ---> hmap_expand_at ---> resize ---> hmap_insert_fast,
> the bridge's ifaces hmap bucket is corrupted.
> 
> To fix the above issue, we check Openflow port number in iface_do_create: if the port
> number is 65535, report an error and do not add this port into a bridge.
> 
> Signed-off-by: lidejun1@huawei.com
> Acked-By: jerry.lilijun@huawei.com

Thank you for the bug fix.

This adds a new check to the caller of ofproto_port_add().  That
function is documented to set the ofp_port argument to OFPP_NONE only if
there is an error.  The caller already checks for an error return.
Therefore, it sounds like there is a bug in ofproto_port_add() that can
cause it to set the port to OFPP_NONE even without reporting an error.
Would you mind trying to find and fix the error in ofproto_port_add(),
instead of covering it up in the caller?

Thanks,

Ben.

Patch

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d90997e..6f6314c 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1802,6 +1802,11 @@  iface_do_create(const struct bridge *br,
                       iface_cfg->name, ovs_strerror(error));
         goto error;
     }
+    if (*ofp_portp == OFPP_NONE) {
+        VLOG_WARN("could not add network device %s to ofproto", iface_cfg->name);
+        error = ERANGE;
+        goto error;
+    }
 
     VLOG_INFO("bridge %s: added interface %s on port %d",
               br->name, iface_cfg->name, *ofp_portp);