[ovs-dev] [PATCH] ofproto-dpif: Delete system tunnel interface when remove ovs bridge

[Ben Pfaff]

On Thu, Jan 25, 2018 at 09:22:56AM -0500, Eric Garver wrote:
> On Wed, Jan 24, 2018 at 12:48:38PM -0800, Ben Pfaff wrote:
> > On Wed, Jan 24, 2018 at 09:31:32AM -0500, Eric Garver wrote:
> > > On Tue, Jan 23, 2018 at 07:46:47PM -0800, Ben Pfaff wrote:
> > > > On Thu, Oct 26, 2017 at 10:24:46AM -0400, Eric Garver wrote:
> > > > > On Wed, Oct 25, 2017 at 11:41:27AM +0800, juyan at redhat.com wrote:
> > > > > > When there is only one bridge,create tunnel in the bridge,
> > > > > > then delete the bridge directly. the system tunnel interface
> > > > > > still in.
> > > > > > 
> > > > > > Cause of only one bridge, backer->refcount values 1, when
> > > > > > delete bridge, "close_dpif_backer" will delete the backer,
> > > > > > so type_run will return directly, doesn't delete the interface.
> > > > > > This patch delete the system interface before free the backer.
> > > > > 
> > > > > I'll add a bit more explanation..
> > > > > 
> > > > > This occurs when a tunnel is created with rtnetlink. With the compat API
> > > > > the tunnel is created via the vport tunnel interface, so it can be
> > > > > implicitly cleaned up by the kernel when the dp is closed or the module
> > > > > unloaded.  But with rtnetlink the kernel module is not involved with the
> > > > > tunnel device creation (it's added to OVS as a netdev vport), so
> > > > > userspace needs to explicitly clean up the tunnel backers - type_run
> > > > > can't garbage collect them if the dpif is already deleted.
> > > > 
> > > > I guess this has been due to be applied for a long time!  I am sorry
> > > > that I missed it.
> > > > 
> > > > The code looks OK but I don't yet understand the consequences.  What
> > > > problem does this patch solve?
> > > > 
> > > 
> > > Bugzilla reference:
> > > 
> > >   https://bugzilla.redhat.com/show_bug.cgi?id=1505776
> > > 
> > > IIRC, there is a race between close_dpif_backer() and tnl_backers
> > > garbage collection which is done in the "if (backer->need_revalidate)"
> > > block of type_run(). Non-tunnel port types are immediately cleaned up by
> > > port_del(), but tunnel ports are only cleaned up during dpif
> > > revalidation. My theory is that we never noticed it in the compat
> > > interface because the openvswitch kernel module implicitly cleans up all
> > > the interfaces when the dpif is closed. In the rtnetlink case, the
> > > tunnel interfaces are created by OVS process not the openvswitch kernel
> > > module, so it doesn't know about them.
> > > 
> > > I think this may also be closing a tunnel port memory leak that occurs
> > > for both compat and rtnetlink when deleting the dpif backer. This occurs
> > > since the tunnel ports weren't garbage collected before the dpif backer
> > > disappeared.
> > > 
> > > Sorry if my explanation isn't that good. I'm not all that familiar with
> > > this code.
> > 
> > OK, thanks.
> > 
> > Is the following a fair description for the commit message?
> > 
> >         When a user adds the first tunnel of a given type (e.g. the
> >         first VXLAN tunnel) to an OVS bridge, OVS adds a vport of the
> >         same type to the kernel datapath that backs the bridge.  There
> >         is the corresponding expectation that, when the last tunnel of
> >         that type is removed from the OVS bridges, OVS would remove the
> >         vport that represents it from the backing kernel datapath, but
> >         OVS was not doing that.  This commit fixes the problem.
> > 
> > What isn't clear to me is the higher-level consequence of failing to
> > delete the kernel datapath vport.  The bugzilla report doesn't say and I
> > don't see anything else that does.  Can anyone help me out?  I'm willing
> 
> short answer: I don't think there is any major concern about the
> lingering tunnel interface.
> 
> Longer answer follows:
> 
> The rtnetlink create will attempt to reuse a tunnel interface if it
> already exists. If that interface has the wrong parameters we attempt to
> delete and recreate it. So the lingering kernel interface is a non-issue
> for OVS. However, OVS shouldn't leave the interface hanging around if
> it's not using it anymore (this is what this patch fixes).
> 
> Tunnels created via rtnetlink are attached to the openvswitch kernel
> module by NETDEV vports. When the backer is closed those NETDEV vports
> are cleaned up by the kernel module. As such, they are disconnected from
> OVS so we won't get any wild packets appearing.
> 
> > to apply this patch without that information, but having it would allow
> > me to better understand the importance of the fix.
> 
> Thanks Ben.

Thanks for the extra info.  I applied this to master.