a crash is seen in "netdev_ports_remove" when an interface is deleted and added
back in the system and when the interface is part of a bridge configuration.
e.g. steps:
create a tap0 interface using "ip tuntap add.."
add the tap0 interface to br0 using "ovs-vsctl add-port.."
delete the tap0 interface from system using "ip tuntap del.."
add the tap0 interface back in system using "ip tuntap add.."
(this changes the ifindex of the interface)
delete tap0 from br0 using "ovs-vsctl del-port.."
In the function "netdev_ports_insert", two hmap entries were created for
mapping "portnum -> netdev" and "ifindex -> portnum".
When the interface is deleted from the system, the "netdev_ports_remove"
function is not getting called and the old ifindex entry is not getting
cleaned up from the "ifindex_to_port" hmap.
As part of the fix, added function "dpif_port_remove" which will call
"netdev_ports_remove" in the path where the interface deletion from the system
is detected.
Also, in "netdev_ports_remove", added the code where the "ifindex_to_port_data"
(ifindex -> portnum map node) is getting freed when the ifindex is not
available any more. (as the interface is already deleted.)
VMware-BZ: #1975788
Signed-off-by: Ashish Varma <[email protected]>
---
v1-v2:
refactoring the function "dpif_port_del" to add an option for local delete
to remove the duplicated call to "netdev_ports_remove".
changes to adhere to the OvS coding standard.
---
AUTHORS.rst | 1 +
lib/dpctl.c | 2 +-
lib/dpif.c | 22 ++++++++++++----------
lib/dpif.h | 2 +-
lib/netdev.c | 22 +++++++++++++++-------
ofproto/ofproto-dpif.c | 16 +++++++++++-----
6 files changed, 41 insertions(+), 24 deletions(-)
diff --git a/AUTHORS.rst b/AUTHORS.rst
index 3d61c04..7eecd54 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -62,6 +62,7 @@ Ariel Tubaltsev [email protected]
Arnoldo Lutz [email protected]
Arun Sharma [email protected]
Aryan TaheriMonfared [email protected]
+Ashish Varma [email protected]
Ashwin Swaminathan [email protected]
Babu Shanmugam [email protected]
Ben Pfaff [email protected]
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 8951d6e..03b239c 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -472,7 +472,7 @@ dpctl_del_if(int argc, const char *argv[], struct
dpctl_params *dpctl_p)
continue;
}
- error = dpif_port_del(dpif, port);
+ error = dpif_port_del(dpif, port, false);
if (error) {
dpctl_error(dpctl_p, error, "deleting port %s from %s failed",
name, argv[1]);
diff --git a/lib/dpif.c b/lib/dpif.c
index 3233bc8..3f0742d 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -605,21 +605,23 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev,
odp_port_t *port_nop)
/* Attempts to remove 'dpif''s port number 'port_no'. Returns 0 if successful,
* otherwise a positive errno value. */
int
-dpif_port_del(struct dpif *dpif, odp_port_t port_no)
+dpif_port_del(struct dpif *dpif, odp_port_t port_no, bool local_delete)
{
- int error;
+ int error = 0;
COVERAGE_INC(dpif_port_del);
- error = dpif->dpif_class->port_del(dpif, port_no);
- if (!error) {
- VLOG_DBG_RL(&dpmsg_rl, "%s: port_del(%"PRIu32")",
- dpif_name(dpif), port_no);
-
- netdev_ports_remove(port_no, dpif->dpif_class);
- } else {
- log_operation(dpif, "port_del", error);
+ if (!local_delete) {
+ error = dpif->dpif_class->port_del(dpif, port_no);
+ if (!error) {
+ VLOG_DBG_RL(&dpmsg_rl, "%s: port_del(%"PRIu32")",
+ dpif_name(dpif), port_no);
+ } else {
+ log_operation(dpif, "port_del", error);
+ }
}
+
+ netdev_ports_remove(port_no, dpif->dpif_class);
return error;
}
diff --git a/lib/dpif.h b/lib/dpif.h
index d9ded8b..ab898f4 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -451,7 +451,7 @@ int dpif_get_dp_stats(const struct dpif *, struct
dpif_dp_stats *);
const char *dpif_port_open_type(const char *datapath_type,
const char *port_type);
int dpif_port_add(struct dpif *, struct netdev *, odp_port_t *port_nop);
-int dpif_port_del(struct dpif *, odp_port_t port_no);
+int dpif_port_del(struct dpif *, odp_port_t port_no, bool local_delete);
/* A port within a datapath.
*
diff --git a/lib/netdev.c b/lib/netdev.c
index 704b38f..2075491 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -2179,6 +2179,7 @@ struct ifindex_to_port_data {
struct hmap_node node;
int ifindex;
odp_port_t port;
+ const struct dpif_class *dpif_class;
};
#define NETDEV_PORTS_HASH_INT(port, dpif) \
@@ -2228,6 +2229,7 @@ netdev_ports_insert(struct netdev *netdev, const struct
dpif_class *dpif_class,
ifidx = xzalloc(sizeof *ifidx);
ifidx->ifindex = ifindex;
ifidx->port = dpif_port->port_no;
+ ifidx->dpif_class = dpif_class;
hmap_insert(&port_to_netdev, &data->node, hash);
hmap_insert(&ifindex_to_port, &ifidx->node, ifidx->ifindex);
@@ -2266,10 +2268,9 @@ netdev_ports_remove(odp_port_t port_no, const struct
dpif_class *dpif_class)
if (data) {
int ifindex = netdev_get_ifindex(data->netdev);
+ struct ifindex_to_port_data *ifidx = NULL;
if (ifindex > 0) {
- struct ifindex_to_port_data *ifidx = NULL;
-
HMAP_FOR_EACH_WITH_HASH (ifidx, node, ifindex, &ifindex_to_port) {
if (ifidx->port == port_no) {
hmap_remove(&ifindex_to_port, &ifidx->node);
@@ -2279,11 +2280,18 @@ netdev_ports_remove(odp_port_t port_no, const struct
dpif_class *dpif_class)
}
ovs_assert(ifidx);
} else {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-
- VLOG_WARN_RL(&rl, "netdev ports map has dpif port %"PRIu32
- " but netdev has no ifindex: %s", port_no,
- ovs_strerror(ifindex));
+ /* case where the interface is already deleted form the datapath
+ * (e.g. tunctl -d or ip tuntap del), then the ifindex is not
+ * valid anymore. Traverse the HMAP for the port number. */
+ HMAP_FOR_EACH (ifidx, node, &ifindex_to_port) {
+ if (ifidx->port == port_no &&
+ ifidx->dpif_class == dpif_class) {
+ hmap_remove(&ifindex_to_port, &ifidx->node);
+ free(ifidx);
+ break;
+ }
+ }
+ ovs_assert(ifidx);
}
dpif_port_destroy(&data->dpif_port);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 4bc942d..e8d2665 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -414,7 +414,7 @@ type_run(const char *type)
}
SIMAP_FOR_EACH (node, &tmp_backers) {
- dpif_port_del(backer->dpif, u32_to_odp(node->data));
+ dpif_port_del(backer->dpif, u32_to_odp(node->data), false);
}
simap_destroy(&tmp_backers);
@@ -571,7 +571,7 @@ process_dpif_port_change(struct dpif_backer *backer, const
char *devname)
} else if (!ofproto) {
/* The port was added, but we don't know with which
* ofproto we should associate it. Delete it. */
- dpif_port_del(backer->dpif, port.port_no);
+ dpif_port_del(backer->dpif, port.port_no, false);
} else {
struct ofport_dpif *ofport;
@@ -762,7 +762,7 @@ open_dpif_backer(const char *type, struct dpif_backer
**backerp)
dpif_port_dump_done(&port_dump);
LIST_FOR_EACH_POP (garbage, list_node, &garbage_list) {
- dpif_port_del(backer->dpif, garbage->odp_port);
+ dpif_port_del(backer->dpif, garbage->odp_port, false);
free(garbage);
}
@@ -1921,7 +1921,13 @@ port_destruct(struct ofport *port_, bool del)
* assumes that removal of attached ports will happen as part of
* destruction. */
if (!port->is_tunnel) {
- dpif_port_del(ofproto->backer->dpif, port->odp_port);
+ dpif_port_del(ofproto->backer->dpif, port->odp_port, false);
+ }
+ } else if (del) {
+ /* The underlying device is already deleted (e.g. tunctl -d).
+ * Calling dpif_port_remove to do local cleanup for the netdev */
+ if (!port->is_tunnel) {
+ dpif_port_del(ofproto->backer->dpif, port->odp_port, true);
}
}
@@ -3697,7 +3703,7 @@ port_del(struct ofproto *ofproto_, ofp_port_t ofp_port)
netdev_get_name(ofport->up.netdev));
ofproto->backer->need_revalidate = REV_RECONFIGURE;
if (!ofport->is_tunnel && !netdev_vport_is_patch(ofport->up.netdev)) {
- error = dpif_port_del(ofproto->backer->dpif, ofport->odp_port);
+ error = dpif_port_del(ofproto->backer->dpif, ofport->odp_port, false);
if (!error) {
/* The caller is going to close ofport->up.netdev. If this is a
* bonded port, then the bond is using that netdev, so remove it
--
2.7.4
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
