Fix the sandbox use case and add a test. #269

davidchisnall · 2021-01-07T16:56:27Z

Summary of changes:

Add a new PAL that doesn't allocate memory, which can be used with a
memory provider that is pre-initialised with a range of memory.
Slightly refactor the memory provider class so that it has a narrower
interface with LargeAlloc and is easier to proxy.
Allow the address space manager and the memory provider to be
initialised with a range of memory.

This may eventually also remove the need for (or, at least, simplify)
the Open Enclave PAL.

Summary of changes: - Add a new PAL that doesn't allocate memory, which can be used with a memory provider that is pre-initialised with a range of memory. - Slightly refactor the memory provider class so that it has a narrower interface with LargeAlloc and is easier to proxy. - Allow the address space manager and the memory provider to be initialised with a range of memory. This may eventually also remove the need for (or, at least, simplify) the Open Enclave PAL.

davidchisnall · 2021-01-07T16:56:47Z

This is neccessary for fixing the Verona process isolation code.

rengolin

I'm not entirely familiar with snmalloc or its implementation, so my comments could be misleading. Grain of salt and all.

src/mem/address_space.h

rengolin · 2021-01-07T17:02:01Z

src/mem/largealloc.h

+        const size_t rsize = bits::one_at_bit(SUPERSLAB_BITS) << large_class;
+        available_large_chunks_in_bytes -= rsize;
+      }
+      return 0;


Shouldn't this return p?

Yes. I honestly have no idea how this passes any tests as written.

You probably would only detect this if you are tracking address space usage, this will not recycle any chunks (i.e. 1MiB ranges) of memory, So tests that go up and down in usage a lot will leak. Did it pass CI. I would expect a Windows test to fail with OOM with this bug. But on your own machine, probably none of the tests will come close to an issue.

src/test/func/sandbox/sandbox.cc

nwf · 2021-01-07T17:40:40Z

src/test/func/sandbox/sandbox.cc

+    {
+      if constexpr (pal_supports<AlignedAllocation, DefaultPal>)
+      {
+        return DefaultPal::reserve_aligned<true>(sb_size);


Needs to test that sb_size >= DefaultPal::minimum_alloc_size?

In theory, yes, but for this test it will be - I don't think we're going to have a PAL that can't allocate 128MiB.

Not all PALs provide minimum_alloc_size. We should probably add that to the concept...

AFAIK minimum_alloc_size is required by AlignedAllocation PALs. Sorry to have left it out of the concept; I can catch that in my next round of miscellany if you don't want to do it here.

Yep, it uses it here:

snmalloc/src/mem/address_space.h

Lines 181 to 185 in 4837c82

if constexpr (pal_supports<AlignedAllocation, PAL>)

{

if (size >= PAL::minimum_alloc_size)

return PAL::template reserve_aligned<committed>(size);

}

nwf · 2021-01-07T17:53:58Z

src/pal/pal_noalloc.h

+     */
+    static constexpr uint64_t pal_features = 0;
+
+    static constexpr size_t page_size = ErrorHandler::page_size;


Maybe BasePal since it's not just for error handling?

I didn't want to call it a Pal because we don't forward any of the memory ops to it.

I've modified this to use the Aal's one, so now this just needs to implement the two error-related things.

nwf · 2021-01-07T17:54:46Z

src/pal/pal_noalloc.h

+     */
+    static std::pair<void*, size_t> reserve_at_least(size_t) noexcept
+    {
+      error("Out of memory");


Maybe expand the comment a bit? "This should never be called. The simplest way to achieve that is to use this PAL with a replacement AddressSpaceManager whose reserve() method does not attempt to use its Pal to reserve memory. See test/func/sandbox/sandbox.cc for an example." ?

Is this here because PalConcept requires it to be? If that's the only reason, maybe we should add a PalFeature NoAllocation flag and relax the Concept in that case?

I like that idea of a feature flag for this.

The ASM still has this code path, it just shouldn't be reached. Currently, snmalloc doesn't handle allocation failures at all, this is how the other PALs handle allocation failure.

I've added a NoAllocation flag and guarded access to the reserve-family functions on it. This function is now gone.

The ASM still has this code path, it just shouldn't be reached. Currently, snmalloc doesn't handle allocation failures at all, this is how the other PALs handle allocation failure.

Returning nullptr should work for allocation failures. We have tests that check for OOM as that is requried behaviour on OE. If there are code paths that cannot handle a null return from the Pal, then that is a problem.

src/mem/address_space.h

CMakeLists.txt

mjp41 · 2021-01-07T18:29:20Z

src/pal/pal_noalloc.h

+    template<bool page_aligned = false>
+    static void zero(void* p, size_t size) noexcept
+    {
+      bzero(p, size);


This would need to be overridable/mixinable for OE to be able to reuse this. It would be nice to not increase the code size unnecessarily.

Or the OE version could just implement snmalloc::bzero as a wrapper around oe_memset_s.

src/test/func/sandbox/sandbox.cc

Hopefully appease Windows CI.

Fix the consexpr if block in sandbox.cc.

rengolin · 2021-01-08T12:19:07Z

In GCC 8 test:

../src/mem/allocstats.h:199:22: error: array subscript 18446744073709551601 is above array bounds of 'size_t [28]' {aka 'long unsigned int [28]'} [-Werror=array-bounds]
       large_pop_count[sc]++;

Uninitialised?

davidchisnall · 2021-01-08T16:09:56Z

CI is happy! Thus ends today's game of whack-a-mole.

mjp41

LGTM, thanks for all the changes.

davidchisnall requested review from nwf, mjp41 and a user January 7, 2021 16:56

rengolin reviewed Jan 7, 2021

View reviewed changes

Fix typo.

34f2a20

nwf reviewed Jan 7, 2021

View reviewed changes

mjp41 reviewed Jan 7, 2021

View reviewed changes

mjp41 reviewed Jan 8, 2021

View reviewed changes

src/test/func/sandbox/sandbox.cc Outdated Show resolved Hide resolved

davidchisnall added 5 commits January 8, 2021 10:22

Address reviewer comments.

78d1bf4

Remove the test dependency on malloc.cc

295501c

Hopefully appease Windows CI.

Adopt Matt's suggestion to make Linux happy.

35207fa

Remove the unaligned alloc function from the BSD aligned PAL.

bc31191

Fix the consexpr if block in sandbox.cc.

Appease MSVC.

adda928

davidchisnall added 7 commits January 8, 2021 12:53

Hopefully fix the build on Windows.

565f925

Make GCC happy.

72f575d

Use memset instead of less-portable bzero.

1625119

Add missing commit.

68e5d32

Remove words from a comment that made Matt unhappy.

83a9f84

Make clang happy again.

1c5bb81

clang-format

900dd50

nwf approved these changes Jan 8, 2021

View reviewed changes

[NFC] Remove extra character in comment.

904928c

mjp41 approved these changes Jan 8, 2021

View reviewed changes

davidchisnall merged commit c33f355 into master Jan 11, 2021

davidchisnall deleted the fix_sandbox branch January 11, 2021 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the sandbox use case and add a test. #269

Fix the sandbox use case and add a test. #269

davidchisnall commented Jan 7, 2021

davidchisnall commented Jan 7, 2021

rengolin left a comment

rengolin Jan 7, 2021

davidchisnall Jan 7, 2021

mjp41 Jan 8, 2021

nwf Jan 7, 2021

davidchisnall Jan 7, 2021

davidchisnall Jan 8, 2021

nwf Jan 8, 2021

mjp41 Jan 8, 2021

nwf Jan 7, 2021

davidchisnall Jan 7, 2021

davidchisnall Jan 8, 2021

nwf Jan 7, 2021

mjp41 Jan 7, 2021 •

edited

Loading

davidchisnall Jan 7, 2021

davidchisnall Jan 8, 2021

mjp41 Jan 8, 2021

mjp41 Jan 7, 2021

davidchisnall Jan 8, 2021

rengolin commented Jan 8, 2021

davidchisnall commented Jan 8, 2021

mjp41 left a comment

	if constexpr (pal_supports<AlignedAllocation, PAL>)
	{
	if (size >= PAL::minimum_alloc_size)
	return PAL::template reserve_aligned<committed>(size);
	}

Fix the sandbox use case and add a test. #269

Fix the sandbox use case and add a test. #269

Conversation

davidchisnall commented Jan 7, 2021

davidchisnall commented Jan 7, 2021

rengolin left a 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

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

mjp41 Jan 7, 2021 • edited Loading

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

rengolin commented Jan 8, 2021

davidchisnall commented Jan 8, 2021

mjp41 left a comment

Choose a reason for hiding this comment

mjp41 Jan 7, 2021 •

edited

Loading