[ovs-dev,v2] bridge: Fix controller status update to passive connections

Message ID 1505333152-11087-1-git-send-email-azhou@ovn.org
State New
Headers show
Series
  • [ovs-dev,v2] bridge: Fix controller status update to passive connections
Related show

Commit Message

Andy Zhou Sept. 13, 2017, 8:05 p.m.
The bug can cause ovs-vswitchd to crash (due to assert) when it is
set up with a passive controller connection. Since only active
connections are kept, the passive connection status update should be
ignored and not trigger asserts.

Fixes: 85c55772a453 ("bridge: Fix controller status update")
Reported-by: Josh Bailey <josh@faucet.nz>
Signed-off-by: Andy Zhou <azhou@ovn.org>

---
v1->v2:  Set defaults when cinfo is NULL.
---
 AUTHORS.rst       |  1 +
 vswitchd/bridge.c | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

Comments

Joe Stringer Sept. 14, 2017, 3:30 a.m. | #1
On 13 September 2017 at 13:05, Andy Zhou <azhou@ovn.org> wrote:
> The bug can cause ovs-vswitchd to crash (due to assert) when it is
> set up with a passive controller connection. Since only active
> connections are kept, the passive connection status update should be
> ignored and not trigger asserts.
>
> Fixes: 85c55772a453 ("bridge: Fix controller status update")
> Reported-by: Josh Bailey <josh@faucet.nz>
> Signed-off-by: Andy Zhou <azhou@ovn.org>

Acked-by: Joe Stringer <joe@ovn.org>
Andy Zhou Sept. 14, 2017, 7:43 p.m. | #2
On Wed, Sep 13, 2017 at 8:30 PM, Joe Stringer <joe@ovn.org> wrote:
> On 13 September 2017 at 13:05, Andy Zhou <azhou@ovn.org> wrote:
>> The bug can cause ovs-vswitchd to crash (due to assert) when it is
>> set up with a passive controller connection. Since only active
>> connections are kept, the passive connection status update should be
>> ignored and not trigger asserts.
>>
>> Fixes: 85c55772a453 ("bridge: Fix controller status update")
>> Reported-by: Josh Bailey <josh@faucet.nz>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>
> Acked-by: Joe Stringer <joe@ovn.org>

Thanks for the review. Pushed to master and branch-2.8.

Patch

diff --git a/AUTHORS.rst b/AUTHORS.rst
index 4b9e78f07f87..5d8b723f62e6 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -390,6 +390,7 @@  Ben Basler                      bbasler@nicira.com
 Bhargava Shastry                bshastry@sec.t-labs.tu-berlin.de
 Bob Ball                        bob.ball@citrix.com
 Brad Hall                       brad@nicira.com
+Brailey Josh                    josh@faucet.nz
 Brandon Heller                  brandonh@stanford.edu
 Brendan Kelley                  bkelley@nicira.com
 Brent Salisbury                 brent.salisbury@gmail.com
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a8cbae78cb23..eec9689e6dbd 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2720,11 +2720,17 @@  refresh_controller_status(void)
             struct ofproto_controller_info *cinfo =
                 shash_find_data(&info, cfg->target);
 
-            ovs_assert(cinfo);
-            ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
-            const char *role = ofp12_controller_role_to_str(cinfo->role);
-            ovsrec_controller_set_role(cfg, role);
-            ovsrec_controller_set_status(cfg, &cinfo->pairs);
+            /* cinfo is NULL when 'cfg->target' is a passive connection.  */
+            if (cinfo) {
+                ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
+                const char *role = ofp12_controller_role_to_str(cinfo->role);
+                ovsrec_controller_set_role(cfg, role);
+                ovsrec_controller_set_status(cfg, &cinfo->pairs);
+            } else {
+                ovsrec_controller_set_is_connected(cfg, false);
+                ovsrec_controller_set_role(cfg, NULL);
+                ovsrec_controller_set_status(cfg, NULL);
+            }
         }
 
         ofproto_free_ofproto_controller_info(&info);