Re: [ovs-dev] [PATCHv3] dpif-netlink-rtnl: Fix ovs_geneve probing after restart.

[Guru Shetty]

On 2 November 2017 at 12:01, 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 
> Cc: Eric Garver 
> Cc: Gurucharan Shetty 
>

Thanks. I also tested this for a good reproduction case and applied to
master and 2.8.


> ---
> v2->v3:
> Add return code checking for netlink parsing
> Fix memory leak
> v1->v2:
> Add detection of existing module, instead of unconditionally
> remote it and create.
> ---
>  lib/dpif-netlink-rtnl.c | 39 +++
>  1 file changed, 39 insertions(+)
>
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..fe9c8ed7104f 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", );
>  if (!error) {
> +struct ofpbuf *reply;
>  const struct netdev_tunnel_config *tnl_cfg;
>
>  tnl_cfg = netdev_get_tunnel_config(netdev);
> @@ -448,6 +449,44 @@ 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, );
> +if (!error) {
> +
> +struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
> +struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
> +const char *kind;
> +
> +if (!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))) {
> +VLOG_ABORT("Error fetching Geneve tunnel device %s "
> +   "linkinfo", name);
> +}
> +
> +kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
> +
> +if (!strcmp(kind, "ovs_geneve")) {
> +out_of_tree = true;
> +} else if (!strcmp(kind, "geneve")) {
> +out_of_tree = false;
> +} else {
> +VLOG_ABORT("Geneve tunnel device %s with kind %s"
> +   " not supported", name, kind);
> +}
> +
> +ofpbuf_delete(reply);
> +netdev_close(netdev);
> +
> +return out_of_tree;
> +}
> +
>  error = dpif_netlink_rtnl_create(tnl_cfg, name,
> OVS_VPORT_TYPE_GENEVE,
>   "ovs_geneve",
>   (NLM_F_REQUEST | NLM_F_ACK
> --
> 2.7.4
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv3] dpif-netlink-rtnl: Fix ovs_geneve probing after restart.

[Eric Garver]

On Thu, Nov 02, 2017 at 12:01:17PM -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 
> Cc: Eric Garver 
> Cc: Gurucharan Shetty 
> ---
> v2->v3:
> Add return code checking for netlink parsing
> Fix memory leak
> v1->v2:
> Add detection of existing module, instead of unconditionally
> remote it and create.
> ---
>  lib/dpif-netlink-rtnl.c | 39 +++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..fe9c8ed7104f 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", );
>  if (!error) {
> +struct ofpbuf *reply;
>  const struct netdev_tunnel_config *tnl_cfg;
>  
>  tnl_cfg = netdev_get_tunnel_config(netdev);
> @@ -448,6 +449,44 @@ 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, );
> +if (!error) {
> +
> +struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
> +struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
> +const char *kind;
> +
> +if (!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))) {
> +VLOG_ABORT("Error fetching Geneve tunnel device %s "
> +   "linkinfo", name);
> +}
> +
> +kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
> +
> +if (!strcmp(kind, "ovs_geneve")) {
> +out_of_tree = true;
> +} else if (!strcmp(kind, "geneve")) {
> +out_of_tree = false;
> +} else {
> +VLOG_ABORT("Geneve tunnel device %s with kind %s"
> +   " not supported", name, kind);
> +}
> +
> +ofpbuf_delete(reply);
> +netdev_close(netdev);
> +
> +return out_of_tree;
> +}
> +
>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
> OVS_VPORT_TYPE_GENEVE,
>   "ovs_geneve",
>   (NLM_F_REQUEST | NLM_F_ACK

This looks better. Thanks!

Acked-by: Eric Garver 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

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 
Cc: Eric Garver 
Cc: Gurucharan Shetty 
---
v2->v3:
Add return code checking for netlink parsing
Fix memory leak
v1->v2:
Add detection of existing module, instead of unconditionally
remote it and create.
---
 lib/dpif-netlink-rtnl.c | 39 +++
 1 file changed, 39 insertions(+)

diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
index 0c32e7d8ccb4..fe9c8ed7104f 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", );
 if (!error) {
+struct ofpbuf *reply;
 const struct netdev_tunnel_config *tnl_cfg;
 
 tnl_cfg = netdev_get_tunnel_config(netdev);
@@ -448,6 +449,44 @@ 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, );
+if (!error) {
+
+struct nlattr *linkinfo[ARRAY_SIZE(linkinfo_policy)];
+struct nlattr *rtlink[ARRAY_SIZE(rtlink_policy)];
+const char *kind;
+
+if (!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))) {
+VLOG_ABORT("Error fetching Geneve tunnel device %s "
+   "linkinfo", name);
+}
+
+kind = nl_attr_get_string(linkinfo[IFLA_INFO_KIND]);
+
+if (!strcmp(kind, "ovs_geneve")) {
+out_of_tree = true;
+} else if (!strcmp(kind, "geneve")) {
+out_of_tree = false;
+} else {
+VLOG_ABORT("Geneve tunnel device %s with kind %s"
+   " not supported", name, kind);
+}
+
+ofpbuf_delete(reply);
+netdev_close(netdev);
+
+return out_of_tree;
+}
+
 error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
  "ovs_geneve",
  (NLM_F_REQUEST | NLM_F_ACK
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev