Bug 242727 - MII device leaks memory on detach
Summary: MII device leaks memory on detach
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-12-19 15:31 UTC by ghuckriede
Modified: 2020-01-13 16:11 UTC (History)
1 user (show)

See Also:


Attachments
patch (1.34 KB, patch)
2019-12-19 15:31 UTC, ghuckriede
no flags Details | Diff
before and after test results (3.60 KB, text/plain)
2019-12-19 15:32 UTC, ghuckriede
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description ghuckriede 2019-12-19 15:31:41 UTC
Created attachment 210065 [details]
patch

Overview:
The 'args' and 'ivars' malloc()s do not have associated free()s.  See lines 397 and 488 of https://svnweb.freebsd.org/base/head/sys/dev/mii/mii.c?annotate=326255.

Here is a proposed fix for this.  

The 'ivars' leak is simple to fix.  Just adding a free in the miibus_detach() solved the issue.

The 'args' leak is a little more complicated.  This is malloc()ed for each phy on the bus "child".  There is currently no detach function for the child.  Added miibus_child_detached() to free the 'args'. 


Steps to Reproduce:
Attach and detach a module with mii devices.  Observe leaks.
# devinfo -v | grep re0
            re0 pnpinfo vendor=0x10ec device=0x8168 subvendor=0x10ec subdevice=0x0123 class=0x020000 at slot=0 function=0 dbsf=pci0:3:0:0 handle=\_SB_.PCI0.RP02.PXSX
# 
# devctl detach pci0:3:0:0
#
# vmstat -m | grep devbuf
       devbuf 34770 70136K       -    35686  16,32,64,128,256,512,1024,2048,4096,8192,65536
# devctl attach pci0:3:0:0 
# vmstat -m | grep devbuf
       devbuf 35307 70188K       -    36223  16,32,64,128,256,512,1024,2048,4096,8192,65536
# devctl detach pci0:3:0:0
# vmstat -m | grep devbuf
       devbuf 34779 70122K       -    36223  16,32,64,128,256,512,1024,2048,4096,8192,65536
# 


Actual Results: 
're' LEAKED 9 times of type 'devbuf' (M_DEVBUF).  2 of these leaks are from the 'mii' device.


Expected Results:
Not to leak (or in the 're' case leak less).


Build Date & Hardware:
HEADr355854 on amd64 target with any hardware with mii device(s)
Comment 1 ghuckriede 2019-12-19 15:32:50 UTC
Created attachment 210066 [details]
before and after test results

Testing details with the patch applied
Comment 2 commit-hook freebsd_committer freebsd_triage 2019-12-20 20:11:27 UTC
A commit references this bug:

Author: markj
Date: Fri Dec 20 20:10:27 UTC 2019
New revision: 355941
URL: https://svnweb.freebsd.org/changeset/base/355941

Log:
  mii(4): Fix ivars leak when the bus device or bus children detach.

  PR:		242727
  Submitted by:	ghuckriede@blackberry.com
  MFC after:	1 week

Changes:
  head/sys/dev/mii/mii.c
Comment 3 commit-hook freebsd_committer freebsd_triage 2019-12-27 00:50:12 UTC
A commit references this bug:

Author: markj
Date: Fri Dec 27 00:49:49 UTC 2019
New revision: 356105
URL: https://svnweb.freebsd.org/changeset/base/356105

Log:
  MFC r355941:
  mii(4): Fix ivars leak when the bus device or bus children detach.

  PR:	242727

Changes:
_U  stable/12/
  stable/12/sys/dev/mii/mii.c
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2019-12-27 00:51:48 UTC
Thanks for the patch.
Comment 5 ghuckriede 2020-01-02 14:03:56 UTC
(In reply to Mark Johnston from comment #4)

Sure.  I have a question though... 
Shouldn't there also be a device_set_ivars(,NULL) after the frees in this patch?
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2020-01-02 14:09:30 UTC
(In reply to ghuckriede from comment #5)
I was thinking about that too when testing the patch, but I don't believe it's necessary when the device is going to be freed imminently.  Do you suspect otherwise?
Comment 7 ghuckriede 2020-01-02 15:50:09 UTC
Not really anything specific, it is just best practice.  I'll try it and see if I can get a SIGSEGV on detach.
Comment 8 ghuckriede 2020-01-13 16:11:58 UTC
Found https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=243324 before I could really test the ivars.