Re: [ovs-dev] [PATCH] netdev, dpif: fix the crash/assert on port delete

2017-11-06 Thread Ashish Varma 
Hi Ben,

I have refactored the function "dpif_port_del" and removed the new function
"dpif_port_remove" added in the v1 patch.
Removed the braces around the if compare.

Sending v2 patch now.

Thanks for the review.

-Ashish

On Fri, Nov 3, 2017 at 2:26 PM, Ben Pfaff  wrote:

> On Fri, Nov 03, 2017 at 07:36:41AM -0700, Ashish Varma wrote:
> > 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 
>
> Thanks for the patch.  It's good to fix a bug and especially to fix a
> crash.  I'm still not entirely certain about the actual need for this
> hmap, but I guess that this fixes a real problem.
>
> This introduces new code in netdev_ports_remove() to handle the new
> case.  The new case duplicates some code that I believe should be
> possible to avoid duplicating.  Can you try to refactor slightly to
> avoid the code duplication?
>
> I see that this writes an expression like: (a == b) && (c == d).
> Usually, in Open vSwitch, we don't add the extra parentheses unless they
> are needed or clear up some genuine confusion, so that we would instead
> write a == b && c == d.
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev, dpif: fix the crash/assert on port delete

2017-11-03 Thread Ben Pfaff 
On Fri, Nov 03, 2017 at 07:36:41AM -0700, Ashish Varma wrote:
> 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 

Thanks for the patch.  It's good to fix a bug and especially to fix a
crash.  I'm still not entirely certain about the actual need for this
hmap, but I guess that this fixes a real problem.

This introduces new code in netdev_ports_remove() to handle the new
case.  The new case duplicates some code that I believe should be
possible to avoid duplicating.  Can you try to refactor slightly to
avoid the code duplication?

I see that this writes an expression like: (a == b) && (c == d).
Usually, in Open vSwitch, we don't add the extra parentheses unless they
are needed or clear up some genuine confusion, so that we would instead
write a == b && c == d.

Thanks,

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

[ovs-dev] [PATCH] netdev, dpif: fix the crash/assert on port delete

2017-11-03 Thread Ashish Varma 
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 
---
 AUTHORS.rst|  1 +
 lib/dpif.c |  6 ++
 lib/dpif.h |  1 +
 lib/netdev.c   | 22 +++---
 ofproto/ofproto-dpif.c |  6 ++
 5 files changed, 29 insertions(+), 7 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 atubalt...@vmware.com
 Arnoldo Lutzarnoldo.lutz.guev...@hpe.com
 Arun Sharma arun.sha...@calsoftinc.com
 Aryan TaheriMonfaredaryan.taherimonfa...@uis.no
+Ashish Varmaashishvarma@gmail.com
 Ashwin Swaminathan  ashwi...@arista.com
 Babu Shanmugam  bscha...@redhat.com
 Ben Pfaff   b...@ovn.org
diff --git a/lib/dpif.c b/lib/dpif.c
index 3233bc8..6b2734b 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -623,6 +623,12 @@ dpif_port_del(struct dpif *dpif, odp_port_t port_no)
 return error;
 }
 
+int
+dpif_port_remove(struct dpif *dpif, odp_port_t port_no)
+{
+return netdev_ports_remove(port_no, dpif->dpif_class);
+}
+
 /* Makes a deep copy of 'src' into 'dst'. */
 void
 dpif_port_clone(struct dpif_port *dst, const struct dpif_port *src)
diff --git a/lib/dpif.h b/lib/dpif.h
index d9ded8b..c2d1c61 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -452,6 +452,7 @@ 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_remove(struct dpif *, odp_port_t port_no);
 
 /* A port within a datapath.
  *
diff --git a/lib/netdev.c b/lib/netdev.c
index 704b38f..954da92 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(_to_netdev, >node, hash);
 hmap_insert(_to_port, >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, _to_port) {
 if (ifidx->port == port_no) {
 hmap_remove(_to_port, >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(, "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, _to_port) {
+if ((ifidx->port == port_no) &&
+(ifidx->dpif_class == dpif_class)) {
+