malloc: Refactor redzone and KASAN handling

[markj]

Summary

malloc_large() duplicates redzone and KASAN handling that is also
present in malloc() and malloc_domainset(). Refactor the
implementations to reduce this a bit.

Reported by: Alexander Sideropoulos <[e-mail]>

[rlibby]

sys/kern/kern_malloc.c

110–111

Now unused.

732

DOMAINSET_RR() -> ds seems to be a functional change. Should it be separated?

741–743

I get why you're deleting this, because you now have malloc_domainset_exec just call this, but API-wise we lose the requirement that no one call malloc_domainset directly with M_EXEC. Is that the intention?

759–762

This didn't use to happen for malloc_large(), and now it does. Over in malloc(), it still doesn't happen for malloc_large(). I'm not understanding the difference.

[markj]

sys/kern/kern_malloc.c

732

Oops, yes, that's a logically separate change.

741–743

The reason for disallowing M_EXEC in plain malloc() is to avoid the overhead of handling a rare case in a hot path, i.e., an extra branch on M_EXEC. That reasoning doesn't really apply to malloc_domainset(), at least in my opinion: it's generally only called for long-lived allocations, so this kind of micro-optimization doesn't make sense.

759–762

I can't remember offhand why discrepancy exists, and there's probably no good reason. In general we want to call kmsan_orig() as close as possible to the consumer, as the use of KMSAN_RET_ADDR helps identify the origin of the allocation. I'll think about it some more and fix it, probably in a separate patch.

[markj]

Address review feedback

[rlibby]

sys/kern/kern_malloc.c

759–762

First, I do not know much about kmsan, so caveat lector.

Yes, there's a discrepancy between how malloc and malloc_domainset are structured, and that is leading to a new discrepancy in whether kmsan_mark and kmsan_orig are called on the malloc_large result.

It used to be that malloc_large did kasan_mark and not kmsan_mark or kmsan_orig, and then we returned immediately after malloc_large from all callers (malloc, malloc_domainset, malloc_domainset_exec).

Now we have net new calls to kmsan_mark and kmsan_orig in the malloc_domainset path (and via it, in malloc_domainset_exec and malloc_exec), but not in malloc.

It looks like we used to (and still do) have a path to kmsan_mark and kmsan_orig via malloc_large / kmem_malloc_domainset / kmem_malloc_domain / kmem_back_domain / kmem_alloc_san. So in malloc_large we have already called kmsan_mark, and kmsan_orig (but as you say, deeper in the stack). Now in this patch we call kmsan_mark and kmsan_orig again in malloc_domainset. The new kmsan_mark call appears to be redundant for malloc_large, while the new kmsan_orig call is not consistently applied.

I'm okay if you want to proceed thorough this patch to further cleanup, I think the changes are harmless except for a little extra kmsan_mark work, but it might also make sense either to avoid the behavior difference in this patch pending further cleanup or else to just address it all here.

[markj]

Clean up KMSAN handling as well.

Add a comment explaining why we allow M_EXEC in malloc_domainset().

[markj]

sys/kern/kern_malloc.c

759–762

I think it's easiest to address it all here. Basically:

• we should try to call kmsan_orig() as close to the consumer as possible;
• we can rely on UMA/kmem_malloc() to mark the allocation as uninitialized, so we just need to update the origin map;
• I can't see a reason for malloc() and malloc_domainset() to behave differently here.

The new version of the patch addresses these points.

[rlibby]

Looks good.