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

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

Commit Message

Andy Zhou Sept. 6, 2017, 10:24 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.

Reported-by: Josh Bailey <josh@faucet.nz>
Signed-off-by: Andy Zhou <azhou@ovn.org>
---
 AUTHORS.rst       |  1 +
 vswitchd/bridge.c | 12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

Joe Stringer Sept. 7, 2017, 6:59 a.m. | #1
On 6 September 2017 at 15:24, 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>
> ---

This bug was reported on v2.8.0, so will need backport to branch-2.8.

>  AUTHORS.rst       |  1 +
>  vswitchd/bridge.c | 12 +++++++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/AUTHORS.rst b/AUTHORS.rst
> index cb0cd91b21ba..e304609b2b27 100644
> --- a/AUTHORS.rst
> +++ b/AUTHORS.rst
> @@ -389,6 +389,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..00f86182c820 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2720,11 +2720,13 @@ 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);
> +            }

Prior to commit 85c55772a453f, the following lines were in the alternative case:
            ovsrec_controller_set_is_connected(cfg, false);
            ovsrec_controller_set_role(cfg, NULL);
            ovsrec_controller_set_status(cfg, NULL);

Should we update the records in the else case here like was previously done?
Andy Zhou Sept. 7, 2017, 6:35 p.m. | #2
On Wed, Sep 6, 2017 at 11:59 PM, Joe Stringer <joe@ovn.org> wrote:
> On 6 September 2017 at 15:24, 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")

Thanks. Will add to the commit message.
>
>> Reported-by: Josh Bailey <josh@faucet.nz>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> ---
>
> This bug was reported on v2.8.0, so will need backport to branch-2.8.

Will keep this in mind when pushing.
>
>>  AUTHORS.rst       |  1 +
>>  vswitchd/bridge.c | 12 +++++++-----
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>> index cb0cd91b21ba..e304609b2b27 100644
>> --- a/AUTHORS.rst
>> +++ b/AUTHORS.rst
>> @@ -389,6 +389,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..00f86182c820 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -2720,11 +2720,13 @@ 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);
>> +            }
>
> Prior to commit 85c55772a453f, the following lines were in the alternative case:
>             ovsrec_controller_set_is_connected(cfg, false);
>             ovsrec_controller_set_role(cfg, NULL);
>             ovsrec_controller_set_status(cfg, NULL);
>
> Should we update the records in the else case here like was previously done?

I don't think it is necessary. Those are the default values and we are
not changing them.

Patch

diff --git a/AUTHORS.rst b/AUTHORS.rst
index cb0cd91b21ba..e304609b2b27 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -389,6 +389,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..00f86182c820 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2720,11 +2720,13 @@  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);
+            }
         }
 
         ofproto_free_ofproto_controller_info(&info);