Bug 256714

Summary: net: Multiple memory leaks when detaching a device
Product: Base System Reporter: jcaplan
Component: binAssignee: freebsd-net (Nobody) <net>
Status: Open ---    
Severity: Affects Many People CC: emaste, markj, melifaro, net
Priority: Normal Keywords: needs-qa
Version: 13.0-STABLEFlags: koobs: mfc-stable13?
koobs: mfc-stable12-
koobs: mfc-stable11-
Hardware: Any   
OS: Any   
Description Flags
proposed patch
updated in6 patch
proposed patch none

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


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:

		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 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.