|Summary:||net: Multiple memory leaks when detaching a device|
|Component:||bin||Assignee:||freebsd-net (Nobody) <net>|
|Severity:||Affects Many People||CC:||emaste, markj, melifaro, net|
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 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.