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 <Alexander.Sideropoulos@netapp.com>
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 74934 Build 71817: arc lint + arc unit
Event Timeline
| 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–763 | 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. | |
| 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–763 | 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. | |
| sys/kern/kern_malloc.c | ||
|---|---|---|
| 759–763 | 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. | |