Bug 256714

Summary: net: Multiple memory leaks when detaching a device
Product: Base System Reporter: jcaplan
Component: binAssignee: Alexander V. Chernikov <melifaro>
Status: Closed FIXED    
Severity: Affects Many People CC: emaste, markj, melifaro, net
Priority: Normal Keywords: needs-qa
Version: 13.0-STABLEFlags: koobs: mfc-stable13?
Hardware: Any   
OS: Any   
Attachments:
Description Flags
proposed patch
none
updated in6 patch
none
proposed patch none

Description jcaplan 2021-06-19 11:05:33 UTC
Created attachment 225929 [details]
proposed patch

Overview
--------

I identified a few memory leaks that occur when detaching a device. The routetbl leak depends on the FIB_ALGO option.

There are 3 new leaks when unmounting devices compared to 12.2.

- pfil_head_unregister() should free the ph. This is a typical use, implying that memory should be freed and it's not the callers responsibility:

		pfil_head_unregister(priv->pfil);
		priv->pfil = NULL;


- in6_purgeifaddr() was removing an item from a list, and then deleting it. The only problem is that `nd6_prefix_del()` does reference counting and this may not be the last reference. instead leave it on the list so it can be still properly deleted later on by nd6_purge().

- the new in6_fib_algo.c code uses the rn_* radix tree functions. rn_detachhead() doesn't free all rn_mklist objects.

Steps to Reproduce
------------------


# vmstat -m | grep -E 'ip6ndp | routetbl | pfil'
         pfil    17     2K       -       17  64,128
     routetbl   126    14K       -      425  32,64,128,256,512,1024,2048,4096,8192
       ip6ndp    10     2K       -       18  64,256
# devctl attach pci0:3:0:0
# ifconfig vmx0 172.16.129.201/24 up
# devctl detach pci0:3:0:0
# vmstat -m | grep -E 'ip6ndp | routetbl | pfil'
         pfil    18     2K       -       18  64,128
     routetbl   151    14K       -      509  32,64,128,256,512,1024,2048,4096,8192
       ip6ndp    11     2K       -       21  64,256
 

Actual Results
--------------

Increase in networking device related memory after attaching and detaching a device

Expected Results
----------------

All memory is freed


Build Date & Hardware
---------------------
FreeBSD freebsd 13.0-RELEASE FreeBSD 13.0-RELEASE #6 releng/13.0-n244733-ea31abc261f: Sat Jun 19 06:23:53 UTC 2021     jcaplan@freebsd:/usr/obj/usr/src/amd64.amd64/sys/DEBUG  amd64


Additional Information
----------------------

Diff attached to fix leaks.


For the lradix6_destroy() issue, it's a workaround because passing the root of the tree to rn_detachhead() is leaking the memory allocated in rn_new_radix_mask(). However, I'm not sure it's possible to safely free that memory in a single walk of the radix tree and address the leak from within rn_delete(). If it turns out to be necessary to do a second pass, doing so on the flattened list of nodes in lr->radix_mem is convenient. It could also be done from inside rn_detachhead() with an extra rn_walktree().
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2021-06-19 14:04:45 UTC
^Triage: patch is against the networking internals.
Comment 2 jcaplan 2021-06-21 18:04:13 UTC
Created attachment 225968 [details]
updated in6 patch

I think there are scenarios where in6_purgeifaddr() removes the last reference so it should check the refcount and remove if it is the last ref e.g. if a new address is allocated rather than detaching the interface
Comment 3 jcaplan 2021-08-12 18:44:01 UTC
Created attachment 227138 [details]
proposed patch

update to nd6 prefix issue

in6_ifattach_linklocal() was only calling nd6_prefix_rele() in the case where nd6_prefix_lookup() has returned a prefix, which increments the reference count. The fix is to also call it in the case where nd6_prelist_add() has added a new prefix as this prefix should really only have one reference to it.
Comment 4 commit-hook freebsd_committer freebsd_triage 2023-12-21 18:56:01 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c1c55da49fd55c01771f8cf1f7255a37b79735d7

commit c1c55da49fd55c01771f8cf1f7255a37b79735d7
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2023-12-21 18:53:49 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2023-12-21 18:53:49 +0000

    pfil: don't leak pfil_head_t on interface detach

    PR:             256714
    Submitted by:   jcaplan@blackberry.com

 sys/net/pfil.c | 1 +
 1 file changed, 1 insertion(+)
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2023-12-25 15:06:07 UTC
^Triage: committed.  Only the mfc-stable13 flag's value is still appropriate.
Comment 6 commit-hook freebsd_committer freebsd_triage 2024-01-09 00:30:17 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=7cfc847aba1356c34ea12d73f6ac72793056fcde

commit 7cfc847aba1356c34ea12d73f6ac72793056fcde
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2023-12-21 18:53:49 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-01-09 00:29:05 +0000

    pfil: don't leak pfil_head_t on interface detach

    PR:             256714
    Submitted by:   jcaplan@blackberry.com

    (cherry picked from commit c1c55da49fd55c01771f8cf1f7255a37b79735d7)

 sys/net/pfil.c | 1 +
 1 file changed, 1 insertion(+)
Comment 7 commit-hook freebsd_committer freebsd_triage 2024-01-09 00:33:20 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f7459ca1e768d76f6abe640f7dbcdc8f1aac0edb

commit f7459ca1e768d76f6abe640f7dbcdc8f1aac0edb
Author:     Gleb Smirnoff <glebius@FreeBSD.org>
AuthorDate: 2023-12-21 18:53:49 +0000
Commit:     Gleb Smirnoff <glebius@FreeBSD.org>
CommitDate: 2024-01-09 00:31:38 +0000

    pfil: don't leak pfil_head_t on interface detach

    PR:             256714
    Submitted by:   jcaplan@blackberry.com

    (cherry picked from commit c1c55da49fd55c01771f8cf1f7255a37b79735d7)

 sys/net/pfil.c | 1 +
 1 file changed, 1 insertion(+)