Bug 260347 - after kldunload memory allocated by the module stays wired
Summary: after kldunload memory allocated by the module stays wired
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.0-RELEASE
Hardware: amd64 Any
: --- Affects Many People
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-11 21:39 UTC by martin
Modified: 2021-12-28 15:02 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description martin 2021-12-11 21:39:18 UTC
I have a very simple kernel module, textbook example, that uses static buffer. It seems memory stays wired even if module is unloaded. If I do it enough times system runs out of memory. Not reproducible on FreeBSD 12.2 amd64.

I'm opening the PR based on the forums discussion: https://forums.freebsd.org/threads/kldunload-failed-to-release-memory.83286/

I don't agree with the replies there (mine are _martin); if I'm mistaken I apologies for wasting everybody's time (and will rethink opening PR in the future).

Simple kernel module:

/*
 * KLD Skeleton
 * Inspired by Andrew Reiter's Daemonnews article
 */

#include <sys/types.h>
#include <sys/module.h>
#include <sys/systm.h>  /* uprintf */
#include <sys/errno.h>
#include <sys/param.h>  /* defines used in kernel.h */
#include <sys/kernel.h> /* types used in module initialization */

#define BUFFER_SIZE 10*1000*1024 // 10 MB
static char gBuffer[BUFFER_SIZE];

static int demo_init (void)
{
  for (int i=0; i<BUFFER_SIZE; i++)
  {
    gBuffer[i] = 'A';
  }
  return 0;
}

static void demo_exit (void)
{
  for (int i=0; i<BUFFER_SIZE; i++)
  {
    gBuffer[i] += 1;
  }
}

/*
 * Load handler that deals with the loading and unloading of a KLD.
 */

static int
skel_loader(struct module *m, int what, void *arg)
{
  int err = 0;

  switch (what) {
  case MOD_LOAD:                /* kldload */
    uprintf("Skeleton KLD loaded.\n");
    demo_init();
    break;
  case MOD_UNLOAD:
    uprintf("Skeleton KLD unloaded.\n");
    demo_exit();
    break;
  default:
    err = EOPNOTSUPP;
    break;
  }
  return(err);
}

/* Declare this module to the rest of the kernel */

static moduledata_t skel_mod = {
  "skel",
  skel_loader,
  NULL
};

DECLARE_MODULE(skeleton, skel_mod, SI_SUB_KLD, SI_ORDER_ANY);

Makefile:

SRCS=skeleton.c
KMOD=skeleton

.include <bsd.kmod.mk>

I compile the module and test it with the loop:
sh
while true ; do kldload ./test.ko && kldunload ./test.ko ; done

In the other terminal I watch the memory utilization with top. Wired memory increases to the point where system freezes. OOM killer kills processes and system is available after a while. If I relaunch the top again I see wired memory is still almost all used, as if module leaves memory leak. 

This behavior is not reproducible on FreeBSD 12.2 as I mentioned above.
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2021-12-13 15:04:44 UTC
Thank you, this is a good find.  It seems that, somehow, the pages backing the KLD mapping are getting wired twice, and the second wiring doesn't get released anywhere.
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2021-12-13 17:36:14 UTC
https://reviews.freebsd.org/D33416
Comment 3 martin 2021-12-13 20:46:46 UTC
Thank you.

Interestingly enough this code doesn't trigger the bug on i386, the same src version.
As my knowledge on the subject is poor I wanted to share this just as interesting result in testing.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2021-12-13 20:54:14 UTC
(In reply to martin from comment #3)
amd64 is somewhat special in that its kernel modules are linked as object files (ELF type ET_REL).  This is just an optimization to avoid the use of a PLT.  The other platforms link kernel modules as full DSOs, and we have separate linkers for the two file types.  Only one of them, the one used on amd64, will trigger the bug.
Comment 5 martin 2021-12-13 21:19:20 UTC
This much I can follow, thank you very much for the explanation.
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-12-14 20:22:17 UTC
A commit in branch main references this bug:

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

commit 88642d978a999aaa3752e86d2f54b1a6aba7fc85
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2021-12-14 20:10:46 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2021-12-14 20:10:46 +0000

    vm_fault: Fix vm_fault_populate()'s handling of VM_FAULT_WIRE

    vm_map_wire() works by calling vm_fault(VM_FAULT_WIRE) on each page in
    the rage.  (For largepage mappings, it calls vm_fault() once per large
    page.)

    A pager's populate method may return more than one page to be mapped.
    If VM_FAULT_WIRE is also specified, we'd wire each page in the run, not
    just the fault page.  Consider an object with two pages mapped in a
    vm_map_entry, and suppose vm_map_wire() is called on the entry.  Then,
    the first vm_fault() would allocate and wire both pages, and the second
    would encounter a valid page upon lookup and wire it again in the
    regular fault handler.  So the second page is wired twice and will be
    leaked when the object is destroyed.

    Fix the problem by modify vm_fault_populate() to wire only the fault
    page.  Also modify the error handler for pmap_enter(psind=1) to not test
    fs->wired, since it must be false.

    PR:             260347
    Reviewed by:    alc, kib
    MFC after:      2 weeks
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D33416

 sys/vm/vm_fault.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-12-28 00:44:45 UTC
A commit in branch stable/13 references this bug:

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

commit 0fc6eebbf76334602c418d3e7bd780dd28b11507
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2021-12-14 20:10:46 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2021-12-28 00:36:07 +0000

    vm_fault: Fix vm_fault_populate()'s handling of VM_FAULT_WIRE

    vm_map_wire() works by calling vm_fault(VM_FAULT_WIRE) on each page in
    the rage.  (For largepage mappings, it calls vm_fault() once per large
    page.)

    A pager's populate method may return more than one page to be mapped.
    If VM_FAULT_WIRE is also specified, we'd wire each page in the run, not
    just the fault page.  Consider an object with two pages mapped in a
    vm_map_entry, and suppose vm_map_wire() is called on the entry.  Then,
    the first vm_fault() would allocate and wire both pages, and the second
    would encounter a valid page upon lookup and wire it again in the
    regular fault handler.  So the second page is wired twice and will be
    leaked when the object is destroyed.

    Fix the problem by modify vm_fault_populate() to wire only the fault
    page.  Also modify the error handler for pmap_enter(psind=1) to not test
    fs->wired, since it must be false.

    PR:             260347
    Reviewed by:    alc, kib
    Sponsored by:   The FreeBSD Foundation

    (cherry picked from commit 88642d978a999aaa3752e86d2f54b1a6aba7fc85)

 sys/vm/vm_fault.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Comment 8 Mark Johnston freebsd_committer freebsd_triage 2021-12-28 15:02:18 UTC
Thanks for the report.