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

[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 <blp at ovn.org> 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 <ashishvarma.ovs at gmail.com>
>
> 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.
>