[ovs-dev] lldp: Fix for OVS crashes when a LLDP-enabled port is deleted
diff mbox series

Message ID 1571641922-23074-1-git-send-email-rudrasurya.r@altencalsoftlabs.com
State New
Headers show
Series
  • [ovs-dev] lldp: Fix for OVS crashes when a LLDP-enabled port is deleted
Related show

Commit Message

Nitin katiyar via dev Oct. 21, 2019, 7:12 a.m. UTC
Issue:
When LLDP is enabled on a port, a structure to hold LLDP related state
is created and that structure has a reference to the port. The ofproto
monitor thread accesses the LLDP structure to periodically send packets
over the associated port. When the port is deleted, the LLDP structure
is not cleaned up and it continues to refer to the deleted port.

When the monitor thread attempts to access the deleted port OVS crashes.
Crash can happen with bridge delete and bond delete also.

Fix:
Remove all references to the LLDP structure and free it when
the port is deleted.

Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>
---
 ofproto/ofproto-dpif.c | 10 +++++-----
 ofproto/ofproto.c      |  3 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

Ben Pfaff Oct. 24, 2019, 7:57 p.m. UTC | #1
On Mon, Oct 21, 2019 at 12:42:02PM +0530, Surya Rudra via dev wrote:
> Issue:
> When LLDP is enabled on a port, a structure to hold LLDP related state
> is created and that structure has a reference to the port. The ofproto
> monitor thread accesses the LLDP structure to periodically send packets
> over the associated port. When the port is deleted, the LLDP structure
> is not cleaned up and it continues to refer to the deleted port.
> 
> When the monitor thread attempts to access the deleted port OVS crashes.
> Crash can happen with bridge delete and bond delete also.
> 
> Fix:
> Remove all references to the LLDP structure and free it when
> the port is deleted.
> 
> Signed-off-by: Surya Rudra <rudrasurya.r@altencalsoftlabs.com>

Thank you for the bug fix.  I applied this to master and I am in the
process of backporting it to older branches.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 496a16c..ab5f487 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2374,24 +2374,24 @@  set_lldp(struct ofport *ofport_,
          const struct smap *cfg)
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
     int error = 0;
 
     if (cfg) {
         if (!ofport->lldp) {
-            struct ofproto_dpif *ofproto;
-
-            ofproto = ofproto_dpif_cast(ofport->up.ofproto);
             ofproto->backer->need_revalidate = REV_RECONFIGURE;
             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
         }
 
         if (!lldp_configure(ofport->lldp, cfg)) {
+            lldp_unref(ofport->lldp);
+            ofport->lldp = NULL;
             error = EINVAL;
         }
-    }
-    if (error) {
+    } else if (ofport->lldp) {
         lldp_unref(ofport->lldp);
         ofport->lldp = NULL;
+        ofproto->backer->need_revalidate = REV_RECONFIGURE;
     }
 
     ofproto_dpif_monitor_port_update(ofport,
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index b4249b0..3aaa45a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2556,6 +2556,9 @@  ofproto_port_unregister(struct ofproto *ofproto, ofp_port_t ofp_port)
 {
     struct ofport *port = ofproto_get_port(ofproto, ofp_port);
     if (port) {
+        if (port->ofproto->ofproto_class->set_lldp) {
+            port->ofproto->ofproto_class->set_lldp(port, NULL);
+        }
         if (port->ofproto->ofproto_class->set_stp_port) {
             port->ofproto->ofproto_class->set_stp_port(port, NULL);
         }