Bug 242746 - ifconfig: Deleting (or re-setting) an IP address holds (leaks?) memory
Summary: ifconfig: Deleting (or re-setting) an IP address holds (leaks?) memory
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Mark Johnston
URL: https://reviews.freebsd.org/D22912
Keywords:
Depends on:
Blocks:
 
Reported: 2019-12-20 21:13 UTC by ghuckriede
Modified: 2020-01-25 03:31 UTC (History)
2 users (show)

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


Attachments
patch (2.11 KB, patch)
2019-12-23 19:01 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ghuckriede 2019-12-20 21:13:27 UTC
Overview:
Setting an IP address consumes 'ifaddr' typed memory.  When destroying the IP address the memory is not freed.

This memory does not appear to be collected later, as running the below script shows no recovery after continuously re-setting the ip address for 16 hours.

The leaked memory was tracked down to the ifa_alloc() function, see line 845 of https://svnweb.freebsd.org/base/head/sys/net/if.c?revision=355070&view=markup
Instrumenting the code revealed that there are more ifa_ref() calls than ifa_free() calls.  This results in the memory being held, as the system believes there is still a reference to this memory.


Steps to Reproduce:
'ifconfig <interface> inet' an address and then 'ifconfig <interface> inet delete' it.


Actual Results:
# vmstat -m | grep ifaddr
       ifaddr   141    36K       -      235  16,32,64,128,256,512,2048,4096
# 
# ifconfig em1 inet  192.168.200.44
# vmstat -m | grep ifaddr
       ifaddr   142    36K       -      236  16,32,64,128,256,512,2048,4096
# ifconfig em1 inet delete 192.168.200.44
# vmstat -m | grep ifaddr
       ifaddr   142    36K       -      236  16,32,64,128,256,512,2048,4096

# cat /tmp/loop
while true
do
        ifconfig em1 192.168.200.33 
        sleep 1 
        vmstat -m | grep ifaddr
done
# /tmp/loop
       ifaddr   135    37K       -      294  16,32,64,128,256,512,2048,4096
       ifaddr   136    38K       -      295  16,32,64,128,256,512,2048,4096
       ifaddr   137    38K       -      296  16,32,64,128,256,512,2048,4096
       ifaddr   138    39K       -      297  16,32,64,128,256,512,2048,4096
....snip (16 hours worth of logs)...
       ifaddr 1834875 917406K       -  1835088  16,32,64,128,256,512,2048,4096
       ifaddr 1834876 917407K       -  1835089  16,32,64,128,256,512,2048,4096
       ifaddr 1834877 917407K       -  1835090  16,32,64,128,256,512,2048,4096
       ifaddr 1834878 917408K       -  1835091  16,32,64,128,256,512,2048,4096
       ifaddr 1834879 917408K       -  1835092  16,32,64,128,256,512,2048,4096
^C
#


Expected Results:
Not to leak/hold onto memory.


Build Date & Hardware:
HEADr355854 on amd64 target with any networking interface
Comment 1 Mark Johnston freebsd_committer 2019-12-23 17:02:16 UTC
I'll take a look at this.
Comment 2 Mark Johnston freebsd_committer 2019-12-23 19:01:45 UTC
Created attachment 210181 [details]
patch

This patch fixes the problem in my own testing - would you be willing to try it?
Comment 3 Mark Johnston freebsd_committer 2019-12-23 19:10:20 UTC
https://reviews.freebsd.org/D22912
Comment 4 ghuckriede 2019-12-23 19:16:19 UTC
Sure I'll try it.
Comment 5 ghuckriede 2019-12-23 20:31:42 UTC
A HEAD dev environment was not available, so the patch was tested on a 12.1 based build (which was where this issue was discovered).

The reported leak was gone with the provided patch.

Also tested detaching em interface while receiving traffic.  No crash nor memory leak was observed.
Comment 6 commit-hook freebsd_committer 2019-12-27 01:13:23 UTC
A commit references this bug:

Author: markj
Date: Fri Dec 27 01:12:54 UTC 2019
New revision: 356107
URL: https://svnweb.freebsd.org/changeset/base/356107

Log:
  Plug some ifaddr refcount leaks.

  - Only take an ifaddr ref in in rt_exportinfo() if the caller explicitly
    requests it.  Take care to release it in this case.
  - Don't unconditionally take a ref in rtrequest1_fib().  rt_getifa_fib()
    will acquire a reference, in which case we would previously acquire
    two references.
  - Stop taking a reference in rtinit1() before calling rtrequest1_fib().
    rtrequest1_fib() will acquire a reference for the RTM_ADD case.

  PR:		242746
  Reviewed by:	melifaro (previous version)
  Tested by:	ghuckriede@blackberry.com
  MFC after:	1 week
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D22912

Changes:
  head/sys/net/route.c
Comment 7 commit-hook freebsd_committer 2020-01-03 00:30:03 UTC
A commit references this bug:

Author: markj
Date: Fri Jan  3 00:29:10 UTC 2020
New revision: 356312
URL: https://svnweb.freebsd.org/changeset/base/356312

Log:
  MFC r356107:
  Plug some ifaddr refcount leaks.

  PR:	242746

Changes:
_U  stable/12/
  stable/12/sys/net/route.c