On Thu, Nov 2, 2017 at 10:32 AM, Eric Garver <[email protected]> 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 <[email protected]> >> Cc: Eric Garver <[email protected]> >> Cc: Gurucharan Shetty <[email protected]> >> --- >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
