Bug 242270 - if_vlan(4): Network stack leaks ifnet references when creating VLAN
Summary: if_vlan(4): Network stack leaks ifnet references when creating VLAN
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Alexander V. Chernikov
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-11-27 18:35 UTC by Andrew Boyer
Modified: 2021-01-16 22:03 UTC (History)
4 users (show)

See Also:
koobs: mfc-stable12?
koobs: mfc-stable11?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Boyer 2019-11-27 18:35:23 UTC
When a VLAN is created on a struct ifnet, one extra leaked reference is added on the ifnet. This reference is never removed and results in a leak of the ifnet once it is forgotten by the network stack.
Roughly:
 * if_clone_create() -> vlan_clone_match() -> vlan_clone_match_ethervid() takes a ref (1)
 * if_clone_createif() -> vlan_clone_create() -> vlan_clone_match_ethervid() takes a ref (2)
 * if_clone_createif() -> vlan_clone_create() -> vlan_config() takes a ref (3)
 * if_clone_createif() -> vlan_clone_create() drops a ref (3)
 * One ref is dropped when the VLAN is destroyed (2)
 * Ref (1) is leaked
The first pass, if_clone_create(), is just to verify that an ifp named "foo0" is present in the system. When if_clone_createif() goes back for the second pass, it does the check again and errors out cleanly if the name is not present. The refcounting was added to this code after it had been in the kernel for years; prior to the refcounting, it would have been necessary for the second pass to re-confirm that the port exists.
To me, it seems clear that the first pass was not intended to return with a reference held. It doesn't return an ifp pointer or anything - just '1' to indicate that the named interface exists. It would be a much larger redesign to combine everything into one pass. Instead we should just fix the reference leak.

(This might only affect foo0.N style VLANs)

This is a fix:
--- a/sys/net/if_vlan.c
+++ b/sys/net/if_vlan.c
@@ -957,10 +957,14 @@ vlan_clone_match_ethervid(const char *name, int *vidp)
 static int
 vlan_clone_match(struct if_clone *ifc, const char *name)
 {
+       struct ifnet *ifp;
        const char *cp;
 
-       if (vlan_clone_match_ethervid(name, NULL) != NULL)
+       ifp = vlan_clone_match_ethervid(name, NULL);
+       if (ifp != NULL) {
+               if_rele(ifp);
                return (1);
+       }
 
        if (strncmp(vlanname, name, strlen(vlanname)) != 0)
                return (0);
Comment 1 commit-hook freebsd_committer freebsd_triage 2020-01-29 18:42:08 UTC
A commit references this bug:

Author: melifaro
Date: Wed Jan 29 18:41:35 UTC 2020
New revision: 357263
URL: https://svnweb.freebsd.org/changeset/base/357263

Log:
  Plug parent iface refcount leak on <ifname>.X vlan creation.

  PR:		kern/242270
  Submitted by:	Andrew Boyer <aboyer at pensando.io>
  MFC after:	2 weeks

Changes:
  head/sys/net/if_vlan.c
Comment 2 Alexander V. Chernikov freebsd_committer freebsd_triage 2020-01-29 18:56:58 UTC
Committed to -HEAD.

Thank you for the detailed analysis&patch!