mrs: add paced free #2377

brooksdavis · 2025-03-26T00:58:08Z

Add support for deferring free to the malloc path where we check if we should revoke. I've called the feature paced rather than deferred because I think we'll likely want to batch frees and only do them on some calls to malloc.

OFFLOAD_QUARANTINE was making everything hard to read so I removed it as we're unlikely to want to use it again.

This branch contains the changes in #2376, but does not depend on the functionality included there (it would probably conflict if you tried to rebase without those changes)

If defined at compile time, QUARANTINE_HIGHWATER limited the size of the quarantine to QUARANTINE_HIGHWATER bytes. Given the vast range of working set sizes this isn't particularly useful except perhaps to set it to 0. The environment variable _RUNTIME_REVOCATION_EVERY_FREE_ENABLE and associated sysctl now serves this purpose.

Use of this macro was removed with store-side revocation support. Fixes: 62e9115 mrs: remove store-side support")

This allow more granularity including allowing quarantine to exceed 1/2 the size of the heap. Replace QUARANTINE_RATIO with QUARANTINE_NUMERATOR and QUARANTINE_DENOMINATOR. The quarantine is flushed when: QUARANTINE_NUMERATOR <quarantine_size> >= <heap size> * ---------------------- QUARANTINE_DENOMINATOR The default values are QUARANTINE_NUMERATOR=1 and QUARANTINE_DENOMINATOR=4 respectively giving the same behavior as the previous default QUARANTINE_RATIO=4.

jrtc27 · 2025-03-26T06:17:34Z

lib/libc/stdlib/malloc/mrs/mrs.c

+		if ((envstr = getenv(MALLOC_QUARANTINE_DENOMINATOR_ENV)) !=
+		    NULL) {
+			quarantine_denominator = strtoul(envstr, &end, 0);
+			if (*end != '\0') {


Also need to check errno != ERANGE if quarantine_denominator == ULONG_MAX

lib/libc/stdlib/malloc/mrs/mrs.c

jrtc27 · 2025-03-26T06:24:28Z

lib/libc/stdlib/malloc/mrs/mrs.c

+	for (struct mrs_descriptor_slab *iter = quarantine->list; iter != NULL;
+	     iter = iter->next) {
+		assert(iter->next_descriptor == 0);
+		while(iter->next_descriptor < iter->num_descriptors) {


jrtc27 · 2025-03-26T06:27:33Z

lib/libc/stdlib/malloc/mrs/mrs.c

-		while(iter->next_descriptor < iter->num_descriptors) {
-			slab_free_next_descriptor(iter);
+		while((ptr = slab_pop_descriptor(iter)) != NULL) {
+			descriptor_free(ptr);
 		}
 		assert(iter->next_descriptor == iter->num_descriptors);


Shouldn't this get hit because we increment next_descriptor on the iteration where slab_pop_descriptor returns NULL

The environmental variables _RUNTIME_QUARANTINE_NUMERATOR and _RUNTIME_QUARANTINE_DENOMINATOR can now override compiled defaults for QUARANTINE_NUMERATOR and QUARANTINE_DENOMINATOR.

OFFLOAD_QUARANTINE depends on pthreads support and has been superceeded by in-kernel async revocation support. MRS_PINNED_CPUSET was technically independent, but primairly supported benchmarking OFFLOAD_QUARANTINE.

The rest of the block is also a bit sketchy, but is less obviously wrong.

Yuecheng Wang reports that with jemalloc, the tccache gets flushed if too many frees happen at once leading to poor malloc performance. This feature defers frees to the malloc path and frees one revoked pointer each time. This primative implemenation does not take size into account so likely has adverse cache effects if malloc's and free's aren't well matched. This functionality defaults to off and can be enabled with _RUNTIME_PACED_FREE_ENABLE. A _RUNTIME_PACED_FREE_DISABLE also exists should the default change. Freeing an object adds a lock to malloc path so it may be advisiable to add batching support to amortize this cost.

Libarchive 3.7.7 Security fixes: #2158 rpm: calculate huge header sizes correctly #2160 util: fix out of boundary access in mktemp functions #2168 uu: stop processing if lines are too long #2174 lzop: prevent integer overflow #2172 rar4: protect copy_from_lzss_window_to_unp() (CVE-2024-20696) #2175 unzip: unify EOF handling #2179 rar4: fix out of boundary access with large files #2203 rar4: fix OOB access with unicode filenames #2210 rar4: add boundary checks to rgb filter #2248 rar4: fix OOB in delta filter #2249 rar4: fix OOB in audio filter #2256 fix multiple vulnerabilities identified by SAST #2258 cpio: ignore out-of-range gid/uid/size/ino and harden AFIO parsing #2265 rar5: clear 'data ready' cache on window buffer reallocs #2269 rar4: fix CVE-2024-26256 (CVE-2024-26256) #2330 iso: be more cautious about parsing ISO-9660 timestamps #2343 tar: clean up linkpath between entries #2364 tar: don't crash on truncated tar archives #2366 gzip: prevent a hang when processing a malformed gzip inside a gzip #2377 tar: fix two leaks in tar header parsing Important bugfixes: #2096 rar5: report encrypted entries #2150 xar: fix another infinite loop and expat error handling #2173 shar: check strdup return value #2161 lha: fix integer truncation on 32-bit systems #2338 tar: fix memory leaks when processing symlinks or parsing pax headers #2245 7zip: fix issue when skipping first file in 7zip archive that is a multiple of 65536 bytes #2252 7-zip: read/write symlink paths as UTF-8 #2259 rar5: don't try to read rediculously long names #2290 ar: fix archive entries having no type #2360 tar: fix truncation of entry pathnames in specific archives CVE: CVE-2024-20696, CVE-2024-26256 PR: 282047 (exp-run) MFC after: 1 week

markjdb · 2025-04-02T19:53:47Z

lib/libc/stdlib/malloc/mrs/mrs.c

@@ -378,15 +383,18 @@ static size_t max_allocated_size;
 * Quarantine arenas for application threads.  At any given time, one is in
 * active use, and the others are being cleaned.


This comment should probably be extended to explain why we need at least 3 arenas.

If it's helpful, feel free to steal some prose from the relevant CHERIoT-RTOS allocator source, where it does the same thing (I am pretty sure): https://github.com/CHERIoT-Platform/cheriot-rtos/blob/20b588dca8fcff4f6ccac229dfe342cd09d50f29/sdk/core/allocator/alloc.h#L872-L903

markjdb · 2025-04-02T19:58:07Z

lib/libc/stdlib/malloc/mrs/mrs.c

+
+	/*
+	 * If we're defering actual free calls until later, just add the
+	 * quaranting to the pending queue and let them be freed one at


Typo, "quaranting"

markjdb · 2025-04-02T20:00:51Z

lib/libc/stdlib/malloc/mrs/mrs_utrace.h

@@ -45,6 +45,7 @@
 #define	UTRACE_MRS_QUARANTINE_FLUSH_DONE	10
 #define	UTRACE_MRS_QUARANTINE_REVOKE	11
 #define	UTRACE_MRS_QUARANTINE_REVOKE_DONE	12
+#define	UTRACE_MRS_REAL_FREE		13


Does this need a corresponding update to lib/libsysdecode/utrace.c?

markjdb · 2025-04-02T20:04:18Z

lib/libc/stdlib/malloc/mrs/mrs.c

-static inline void
-slab_free_next_descriptor(struct mrs_descriptor_slab *s)
+static inline void *
+slab_pop_descriptor(struct mrs_descriptor_slab *s)


Why "pop"? These entries are FIFO, aren't they?

markjdb · 2025-04-02T20:12:05Z

lib/libc/stdlib/malloc/mrs/mrs.c

 	/*
 	 * Doesn't matter if the underlying size isn't a 16-byte multiple
 	 * because all allocations will be 16-byte aligned.
 	 */
-	size_t len = __builtin_align_up(cheri_getlen(s->slab[i].ptr),
+	size_t len = __builtin_align_up(cheri_getlen(ptr),


This isn't really related to the diff itself, but the descriptor arenas spend 16 bytes per entry to store the size of the memory region, and that seems redundant is it's available from the capability. Do we do that because cheri_getlen() is relatively expensive? If so, I'd expect we want to use the saved value here rather than extracting it from the capability again.

brooksdavis added 3 commits March 24, 2025 17:26

mrs: remove CONCURRENT_REVOCATION_PASSES docs

6e4853f

Use of this macro was removed with store-side revocation support. Fixes: 62e9115 mrs: remove store-side support")

brooksdavis requested review from nwf, markjdb and Yuecheng-CAM March 26, 2025 00:58

jrtc27 reviewed Mar 26, 2025

View reviewed changes

lib/libc/stdlib/malloc/mrs/mrs.c Show resolved Hide resolved

jrtc27 reviewed Mar 26, 2025

View reviewed changes

brooksdavis added 5 commits March 26, 2025 17:21

mrs: make quarantine size tuneable

28227a2

The environmental variables _RUNTIME_QUARANTINE_NUMERATOR and _RUNTIME_QUARANTINE_DENOMINATOR can now override compiled defaults for QUARANTINE_NUMERATOR and QUARANTINE_DENOMINATOR.

mrs: GC OFFLOAD_QUARANTINE and MRS_PINNED_CPUSET

07cf8a7

OFFLOAD_QUARANTINE depends on pthreads support and has been superceeded by in-kernel async revocation support. MRS_PINNED_CPUSET was technically independent, but primairly supported benchmarking OFFLOAD_QUARANTINE.

mrs: factor out freeing revoked pointers

3ad6a5e

mrs: GC an obsolete comment

4fca268

The rest of the block is also a bit sketchy, but is less obviously wrong.

brooksdavis force-pushed the mrs-paced-free branch from a95bb9e to 6d31ff9 Compare March 26, 2025 17:33

markjdb reviewed Apr 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrs: add paced free #2377

mrs: add paced free #2377

brooksdavis commented Mar 26, 2025

jrtc27 Mar 26, 2025

jrtc27 Mar 26, 2025

jrtc27 Mar 26, 2025

markjdb Apr 2, 2025

nwf Apr 4, 2025

markjdb Apr 2, 2025

markjdb Apr 2, 2025

markjdb Apr 2, 2025

markjdb Apr 2, 2025

		@@ -378,15 +383,18 @@ static size_t max_allocated_size;
		* Quarantine arenas for application threads. At any given time, one is in
		* active use, and the others are being cleaned.

mrs: add paced free #2377

Are you sure you want to change the base?

mrs: add paced free #2377

Conversation

brooksdavis commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment