Message ID | 1509649277-49291-1-git-send-email-u9012063@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,PATCHv3] dpif-netlink-rtnl: Fix ovs_geneve probing after restart. | expand |
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 <u9012063@gmail.com> > Cc: Eric Garver <e@erig.me> > Cc: Gurucharan Shetty <guru@ovn.org> > --- > 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", &netdev); > 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, &reply); > + 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 <e@erig.me>
On 2 November 2017 at 12:01, William Tu <u9012063@gmail.com> 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> > 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", &netdev); > 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, &reply); > + 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 > >
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", &netdev); 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, &reply); + 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