malloc(9): extend contigmalloc(9) by a "slab cookie"

[bz]

Summary

Extend kern_malloc.c internals to also cover contigmalloc(9) by a
"slab cookie" and not just malloc/malloc_large. This allows us to
call free(9) even on contigmalloc(9) addresses.
Extend the contigmalloc(9) man page with a small additional note that
free(9) also works now.

The implementation also adds additional chacks now under INVARIANTS;
e.g. validity of a cookie (allocation type) in various places as well
as size and allocation type checks for contigfree(9).

The way this is done (free working for contigmalloc) will hide the
UMA/VM bits from a consumer which may otherwise need to know whether
the original allocation was by malloc or contigmalloc by looking at
the cookie (likely via an accessor function). This simplifies
the implementation of consumers of mixed environments a lot.

This is preliminary work to allow LinuxKPI to be adjusted to better
play by the rules Linux puts out for various allocations.
Most of this was described/explained to me by jhb.

One may observe that realloc(9) is currently unchanged (and contrary
to [contig]malloc/[contig]free an implementation may need access
the "slab cookie" information given it will likely be implementation
dependent which allocation to use if size changes beyond the usable
size of the initial allocation).

Described by: jhb
Sponsored by: The FreeBSD Foundation
MFC after: 3 days

Test Plan

I've been running with this + LinuxKPI changes for a good month
and a bit now.
I wonder whom to ask to micro-benchmark this?

[markj]

share/man/man9/contigmalloc.9

121

Can't we always use free() now? If so, I would prefer to eliminate contigfree().

sys/kern/kern_malloc.c

472

KASSERT strings don't need a newline.

491

This line is too long. It would be good to introduce a macro to provide this cookie instead of manually constructing it.

536

Panic strings shouldn't end with a newline or a period. I see a couple of preexisting strings which have them but that's just a bug.

951

If you introduce a macro to extract the cookie type, the code will be a bit neater and then you don't need the slab_cookie helper variable.

952

It would be preferable to keep the branch hint. I believe you can annotate case statements the same way, i.e., case __predict_true(SLAB_COOKIE_SLAB_PTR):.

1021

There's no need to do anything special for KASAN here, it'll be handled in the kmem_* layer.

1181

I'd suggest simply replacing this case with __assert_unreachable();. You lose the panic string, but the info there is not very useful anyway.

[bz]

Mark state to address Mark's comments.

[bz]

I've locally implemented all the suggestions (Thanks a lot!).

I'll update the review in a bit; the rebase takes a bits longer to recompile after having been mostly offline for a bit.

share/man/man9/contigmalloc.9

121

I'll mark it "deprecated" in this change and we can do the sweep over the tree in a separate pass (I am all for it if we easily can).

[bz]

Try to address all the comments from @markj

[kib]

sys/kern/kern_malloc.c

1022

IMO it makes sense to bzero whole page(s) from contigmalloc, not only the allocated size, if bzero-ing.

BTW why do we bzero the stuff on deallocation?

[bz]

sys/kern/kern_malloc.c

1022

I was wondering while updating the patch but left it for simplicity. I'll adjust it.
Would it be save to assume passing the rouded up size into kmem_free() is okay (given kmem_free does the same rounding as the very first thing)?

I assume zfree was introduced to wipe sensitive data right away when it is no longer needed.

[markj]

Looks good, I like this change.

sys/kern/kern_malloc.c

123

Extra newline.

127

I think this line needs to be wrapped.

[kib]

sys/kern/kern_malloc.c

1022

I was wondering while updating the patch but left it for simplicity. I'll adjust it.
Would it be save to assume passing the rouded up size into kmem_free() is okay (given kmem_free does the same rounding as the very first thing)?

Sure.

I assume zfree was introduced to wipe sensitive data right away when it is no longer needed.

But most data is not sensitive, so it is extra unneeded work for common case.

[bz]

Address comments from @markj and @kib

[bz]

sys/kern/kern_malloc.c

1022

It's zfree not free, so the caller decides. It's just now also supporting contigmalloc based allocations on free. All the other bits been there since 45035becfe8aed04d0da70585e7c8bc07a385980 .

[kib]

sys/kern/kern_malloc.c

1022

I see. The code is copy/pasted between free and zfree.

Could we then provide static __always_inline zfree1() that takes bool argument to indicate if the memory needs zeroing? Then free() would be zfree1(..., false) and zfree be zfree1(,,,, true). Clang is usually quite good at removing the unused code bracketed by if (false).

Perhaps this should be a preliminary commit.

[bz]

If this version is still good it would be great to just get that checkbox again and I'll put the 3 suggested follow-up changes out as well.

sys/kern/kern_malloc.c

1022

I'll be happy to have multiple follow-up commits (I'll put them out on top of this):

• remove contigfree from the tree
• cleanup the other KASSERT/panic strings '\n' or '.' as @markj had pointed out
• help folding zfree and free together with a small wrapper (like contigfree is now or even as a macro?) -- I think doing it afterwards is easier as the switch() makes the code more similar?

All of this is not needed for this to proceed I would say though. But this blocks some LinuxKPI fixes which need to be done for WiFi.