Page MenuHomeFreeBSD

Don't free the cursor boundary tag during vmem_destroy().
ClosedPublic

Authored by jhb on Wed, Oct 9, 12:26 AM.

Details

Summary

The cursor boundary tag is statically allocated in the vmem instead
of from the vmem_bt_zone. Avoid placing it on the freetags list
when removing segments from the vmem in vmem_destroy so that isn't
freed to the vmem_bt_zone.

Test Plan
  • kldunload of if_cxgbe no longer panics under a kernel with INVARIANTS
  • I wrote a simple kernel module that did vmem_create on load and vmem_destroy on unload. It was able to provoke the panic (uma_zfree complained about a misaligned pointer) and no longer panics on unload.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Wed, Oct 9, 12:26 AM
jhb added inline comments.Wed, Oct 9, 12:27 AM
sys/kern/subr_vmem.c
847 ↗(On Diff #63069)

There are some other ways to handle this such as doing TAILQ_REMOVE instead of bt_remseg and keeping the while loop instead of TAILQ_FOREACH_SAFE. I'm open to fixing it however you prefer.

jhb added a comment.Wed, Oct 9, 12:27 AM

Test kernel module:

#include <sys/param.h>
#include <sys/kernel.h>
#include <sys/malloc.h>
#include <sys/module.h>
#include <sys/vmem.h>

static vmem_t *test_vmem;

static int
vmem_load(void)
{

	test_vmem = vmem_create("T4TLS key map", 0x258aaa80, 67108864,
		    32, 0, M_FIRSTFIT | M_WAITOK);
	if (test_vmem == NULL)
		return (ENOMEM);
	return (0);
}

static int
vmem_unload(void)
{

	if (test_vmem)
		vmem_destroy(test_vmem);
	return (0);
}

static int
mod_event(struct module *mod, int what, void *arg)
{

	switch (what) {
	case MOD_LOAD:
		return (vmem_load());
	case MOD_UNLOAD:
		return (vmem_unload());
	default:
		return (EOPNOTSUPP);
	}
}

static moduledata_t vmem_mod = {
	"vmem_test",
	mod_event,
	NULL
};

DECLARE_MODULE(vmem_test, vmem_mod, SI_SUB_LAST, SI_ORDER_ANY);
markj accepted this revision.Wed, Oct 9, 12:35 AM
markj added inline comments.
sys/kern/subr_vmem.c
847 ↗(On Diff #63069)

I think I would remove the cursor before the loop, if only to avoid an extra local var, but I'm happy with the change either way.

This revision is now accepted and ready to land.Wed, Oct 9, 12:35 AM
jhb added inline comments.Wed, Oct 9, 4:06 PM
sys/kern/subr_vmem.c
847 ↗(On Diff #63069)

Yeah that thought occurred to me this morning as well. It would be a smaller diff by just adding a single line before the loop. I'll test it.

jhb updated this revision to Diff 63091.Wed, Oct 9, 7:21 PM
  • Simplify the vmem fix.
This revision now requires review to proceed.Wed, Oct 9, 7:21 PM
jhb marked an inline comment as done.Wed, Oct 9, 7:22 PM
markj accepted this revision.Wed, Oct 9, 7:24 PM
This revision is now accepted and ready to land.Wed, Oct 9, 7:24 PM
This revision was automatically updated to reflect the committed changes.