This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Use MAP_NORESERVE to release pages
AbandonedPublic

Authored by vitalybuka on May 23 2021, 12:06 AM.

Details

Reviewers
cryptoad
pcc
hctim
Summary

This allows to avoid memset in workaround
of QEMU-user implementation of MADV_DONTNEED.

Diff Detail

Event Timeline

vitalybuka requested review of this revision.May 23 2021, 12:06 AM
vitalybuka created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2021, 12:06 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
cryptoad added inline comments.May 23 2021, 10:04 AM
compiler-rt/lib/scudo/standalone/linux.cpp
124

Looks like Data is still UNUSED

compiler-rt/lib/scudo/standalone/secondary.h
162

Maybe move that up a block since it's common to the if & else

compiler-rt/lib/scudo/standalone/tests/common_test.cpp
1

There is a misalignment of - compared to the bottom one. Not sure which one has the right count.

18–20

Can this be put in SCUDO_LINUX block and a UNREACHABLE or equivalent for other systems?

37

= {};
Not important for Linux but will be for Fuchsia.

vitalybuka updated this revision to Diff 347276.EditedMay 23 2021, 3:55 PM
vitalybuka marked 5 inline comments as done.

addressing @cryptoad comments

vitalybuka added inline comments.May 23 2021, 3:56 PM
compiler-rt/lib/scudo/standalone/tests/common_test.cpp
37

This file is done on D102994

hctim added inline comments.May 24 2021, 9:53 AM
compiler-rt/lib/scudo/standalone/primary64.h
267

More descriptive defaults: MAP_ALLOWNOMEM | MAP_RESIZABLE?

357

MTE mode can change at runtime after initLinkerInitialized is called, so we do need to re-check useMemoryTagging<>() every time we allocate. Maybe call the member PersistentMapFlags (i.e. it persists across the allocator and is once-init), and add a member getMapFlags() that returns PersistentMapFlags | (useMemoryTagging<>() ? MAP_MEMTAG : 0).

pcc added a comment.May 24 2021, 10:00 AM

TBH, I'm not sure it's worth going to this much effort to work around this QEMU bug. It seems better to figure out a way to use QEMU in system mode instead.

compiler-rt/lib/scudo/standalone/primary64.h
358

This will continue to create mappings with PROT_MTE after MTE is disabled.

compiler-rt/lib/scudo/standalone/secondary.h
385–386

This will break the split mapping of MTE enabled secondary allocations. The first few pages need to have MTE enabled and the rest have MTE disabled.

pcc added inline comments.May 24 2021, 10:04 AM
compiler-rt/lib/scudo/standalone/primary64.h
267

Initialize to 0 so that the combined allocator remains in bss.

vitalybuka marked 4 inline comments as done.May 24 2021, 11:05 AM

TBH, I'm not sure it's worth going to this much effort to work around this QEMU bug. It seems better to figure out a way to use QEMU in system mode instead.

This does not looks like a lot of work to maintain this, but I propose to threat this as default behavior D102980.
Let's chat about if we need QEMU-user later today.

compiler-rt/lib/scudo/standalone/primary64.h
267

I replaced with function which will return flags.

357

Actually missed this point, and tried to keep same protection.
If so I can just read Options when create ReleaseRecorder.

compiler-rt/lib/scudo/standalone/secondary.h
385–386

Thanks, I missed this one. Seems easy to fix.
Before that I'd like to NFC refactor this code to make it more obvious.

vitalybuka marked 2 inline comments as done.

addressed most comments

vitalybuka planned changes to this revision.May 24 2021, 11:11 AM

Avoid tracking Prot flags in allocators

fix x86 build

vitalybuka planned changes to this revision.May 27 2021, 2:19 PM

need reconsider as I found another related issue

ormris removed a subscriber: ormris.Jun 3 2021, 10:18 AM
vitalybuka abandoned this revision.Jun 11 2021, 3:12 PM