diff mbox series

[ovs-dev,PATCHv2] dpif-netlink-rtnl: Fix ovs_geneve probing after restart.

Message ID 1509564940-29100-1-git-send-email-u9012063@gmail.com
State Superseded
Headers show
Series [ovs-dev,PATCHv2] dpif-netlink-rtnl: Fix ovs_geneve probing after restart. | expand

Commit Message

William Tu Nov. 1, 2017, 7:35 p.m. UTC 
When using the out-of-tree (openvswitch compat) geneve module,
the first time oot tunnel probing returns true (correct).
Without unloading the geneve module, if the userspace ovs-vswitchd
restarts, because the 'geneve_sys_6081' still exists, the probing
incorrectly returns false and loads the in-tree (upstream kernel)
geneve module.

The patch fixes it by querying the geneve device's kind when exists.
The out-of-tree modules uses kind string as 'ovs_geneve', while the
in-tree module uses 'geneve'.  To reproduce the issue, start the ovs
> /etc/init.d/openvswitch-switch start
> creat a bridge and attach a geneve port using out-of-tree geneve
> /etc/init.d/openvswitch-switch restart

Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides used interface")
Signed-off-by: William Tu <u9012063@gmail.com>
Cc: Eric Garver <e@erig.me>
Cc: Gurucharan Shetty <guru@ovn.org>
---
v1->v2:
    Add detection of existing module, instead of unconditionally
    remote it and create.
---
 lib/dpif-netlink-rtnl.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Eric Garver Nov. 2, 2017, 5:32 p.m. UTC | #1 
On Wed, Nov 01, 2017 at 12:35:40PM -0700, William Tu wrote:
> When using the out-of-tree (openvswitch compat) geneve module,
> the first time oot tunnel probing returns true (correct).
> Without unloading the geneve module, if the userspace ovs-vswitchd
> restarts, because the 'geneve_sys_6081' still exists, the probing
> incorrectly returns false and loads the in-tree (upstream kernel)
> geneve module.
> 
> The patch fixes it by querying the geneve device's kind when exists.
> The out-of-tree modules uses kind string as 'ovs_geneve', while the
> in-tree module uses 'geneve'.  To reproduce the issue, start the ovs
> > /etc/init.d/openvswitch-switch start
> > creat a bridge and attach a geneve port using out-of-tree geneve
> > /etc/init.d/openvswitch-switch restart
> 
> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides used interface")
> Signed-off-by: William Tu <u9012063@gmail.com>
> Cc: Eric Garver <e@erig.me>
> Cc: Gurucharan Shetty <guru@ovn.org>
> ---
> v1->v2:
>     Add detection of existing module, instead of unconditionally
>     remote it and create.
> ---
>  lib/dpif-netlink-rtnl.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..b19b862d308b 100644
> --- a/lib/dpif-netlink-rtnl.c
> +++ b/lib/dpif-netlink-rtnl.c
> @@ -440,6 +440,7 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>  
>      error = netdev_open("ovs-system-probe", "geneve", &netdev);
>      if (!error) {
> +        struct ofpbuf *reply;
>          const struct netdev_tunnel_config *tnl_cfg;
>  
>          tnl_cfg = netdev_get_tunnel_config(netdev);
> @@ -448,6 +449,36 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>          }
>  
>          name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
> +
> +        /* The geneve module exists when ovs-vswitchd crashes
> +         * and restarts, handle the case here.
> +         */
> +        error = dpif_netlink_rtnl_getlink(name, &reply);
> +        if (!error) {
> +
> +            struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
> +            struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
> +            const char *kind;
> +
> +            nl_policy_parse(reply, NLMSG_HDRLEN + sizeof(struct ifinfomsg),
> +                            rtlink_policy, rtlink,
> +                            ARRAY_SIZE(rtlink_policy));
> +            nl_parse_nested(rtlink[IFLA_LINKINFO], linkinfo_policy,
> +                            linkinfo, ARRAY_SIZE(linkinfo_policy));

Should check the return code of these two.

> +            kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
> +
> +            if (!strcmp(kind, "ovs_geneve")) {
> +                out_of_tree = true;
> +                goto exit;
> +            } else if (!strcmp(kind, "geneve")) {
> +                out_of_tree = false;
> +                goto exit;
> +            } else {
> +                VLOG_ABORT("Geneve tunnel device %s with kind %s"
> +                           " not supported", name, kind);
> +            }

Need to free reply somewhere, ofpbuf_delete(reply)

> +        }
> +
>          error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
>                                           "ovs_geneve",
>                                           (NLM_F_REQUEST | NLM_F_ACK
> @@ -458,6 +489,10 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>              }
>              out_of_tree = true;
>          }
> +    }
> +
> +exit:
> +    if (!error) {
>          netdev_close(netdev);
>      }

This is not right. netdev_open() may succeed, but
dpif_netlink_rtnl_create() can fail. Which means netdev will not be
freed here.
William Tu Nov. 2, 2017, 7:09 p.m. UTC | #2 
On Thu, Nov 2, 2017 at 10:32 AM, Eric Garver <e@erig.me> wrote:
> On Wed, Nov 01, 2017 at 12:35:40PM -0700, William Tu wrote:
>> When using the out-of-tree (openvswitch compat) geneve module,
>> the first time oot tunnel probing returns true (correct).
>> Without unloading the geneve module, if the userspace ovs-vswitchd
>> restarts, because the 'geneve_sys_6081' still exists, the probing
>> incorrectly returns false and loads the in-tree (upstream kernel)
>> geneve module.
>>
>> The patch fixes it by querying the geneve device's kind when exists.
>> The out-of-tree modules uses kind string as 'ovs_geneve', while the
>> in-tree module uses 'geneve'.  To reproduce the issue, start the ovs
>> > /etc/init.d/openvswitch-switch start
>> > creat a bridge and attach a geneve port using out-of-tree geneve
>> > /etc/init.d/openvswitch-switch restart
>>
>> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides used interface")
>> Signed-off-by: William Tu <u9012063@gmail.com>
>> Cc: Eric Garver <e@erig.me>
>> Cc: Gurucharan Shetty <guru@ovn.org>
>> ---
>> v1->v2:
>>     Add detection of existing module, instead of unconditionally
>>     remote it and create.
>> ---
>>  lib/dpif-netlink-rtnl.c | 35 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
>> index 0c32e7d8ccb4..b19b862d308b 100644
>> --- a/lib/dpif-netlink-rtnl.c
>> +++ b/lib/dpif-netlink-rtnl.c
>> @@ -440,6 +440,7 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>>
>>      error = netdev_open("ovs-system-probe", "geneve", &netdev);
>>      if (!error) {
>> +        struct ofpbuf *reply;
>>          const struct netdev_tunnel_config *tnl_cfg;
>>
>>          tnl_cfg = netdev_get_tunnel_config(netdev);
>> @@ -448,6 +449,36 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>>          }
>>
>>          name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>> +
>> +        /* The geneve module exists when ovs-vswitchd crashes
>> +         * and restarts, handle the case here.
>> +         */
>> +        error = dpif_netlink_rtnl_getlink(name, &reply);
>> +        if (!error) {
>> +
>> +            struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
>> +            struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
>> +            const char *kind;
>> +
>> +            nl_policy_parse(reply, NLMSG_HDRLEN + sizeof(struct ifinfomsg),
>> +                            rtlink_policy, rtlink,
>> +                            ARRAY_SIZE(rtlink_policy));
>> +            nl_parse_nested(rtlink[IFLA_LINKINFO], linkinfo_policy,
>> +                            linkinfo, ARRAY_SIZE(linkinfo_policy));
>
> Should check the return code of these two.
>
>> +            kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
>> +
>> +            if (!strcmp(kind, "ovs_geneve")) {
>> +                out_of_tree = true;
>> +                goto exit;
>> +            } else if (!strcmp(kind, "geneve")) {
>> +                out_of_tree = false;
>> +                goto exit;
>> +            } else {
>> +                VLOG_ABORT("Geneve tunnel device %s with kind %s"
>> +                           " not supported", name, kind);
>> +            }
>
> Need to free reply somewhere, ofpbuf_delete(reply)
>
>> +        }
>> +
>>          error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
>>                                           "ovs_geneve",
>>                                           (NLM_F_REQUEST | NLM_F_ACK
>> @@ -458,6 +489,10 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>>              }
>>              out_of_tree = true;
>>          }
>> +    }
>> +
>> +exit:
>> +    if (!error) {
>>          netdev_close(netdev);
>>      }
>
> This is not right. netdev_open() may succeed, but
> dpif_netlink_rtnl_create() can fail. Which means netdev will not be
> freed here.

Hi Eric,

Thanks for the feedback. I've sent v3 patch.

William
diff mbox series

Patch

diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
index 0c32e7d8ccb4..b19b862d308b 100644
--- a/lib/dpif-netlink-rtnl.c
+++ b/lib/dpif-netlink-rtnl.c
@@ -440,6 +440,7 @@  dpif_netlink_rtnl_probe_oot_tunnels(void)
 
     error = netdev_open("ovs-system-probe", "geneve", &netdev);
     if (!error) {
+        struct ofpbuf *reply;
         const struct netdev_tunnel_config *tnl_cfg;
 
         tnl_cfg = netdev_get_tunnel_config(netdev);
@@ -448,6 +449,36 @@  dpif_netlink_rtnl_probe_oot_tunnels(void)
         }
 
         name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
+
+        /* The geneve module exists when ovs-vswitchd crashes
+         * and restarts, handle the case here.
+         */
+        error = dpif_netlink_rtnl_getlink(name, &reply);
+        if (!error) {
+
+            struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
+            struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
+            const char *kind;
+
+            nl_policy_parse(reply, NLMSG_HDRLEN + sizeof(struct ifinfomsg),
+                            rtlink_policy, rtlink,
+                            ARRAY_SIZE(rtlink_policy));
+            nl_parse_nested(rtlink[IFLA_LINKINFO], linkinfo_policy,
+                            linkinfo, ARRAY_SIZE(linkinfo_policy));
+            kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
+
+            if (!strcmp(kind, "ovs_geneve")) {
+                out_of_tree = true;
+                goto exit;
+            } else if (!strcmp(kind, "geneve")) {
+                out_of_tree = false;
+                goto exit;
+            } else {
+                VLOG_ABORT("Geneve tunnel device %s with kind %s"
+                           " not supported", name, kind);
+            }
+        }
+
         error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
                                          "ovs_geneve",
                                          (NLM_F_REQUEST | NLM_F_ACK
@@ -458,6 +489,10 @@  dpif_netlink_rtnl_probe_oot_tunnels(void)
             }
             out_of_tree = true;
         }
+    }
+
+exit:
+    if (!error) {
         netdev_close(netdev);
     }