Don't use F_SEAL_FUTURE_WRITE for boot image methods.
The behavior with private mappings on fork is unexpected.
Bug: 143833776
Bug: 119800099
Bug: 143637883
Bug: 143569713
Test: boots
Change-Id: Iaccf33b787a7a5cb09a0054b87e4f34850929c72
diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc
index 63b5ffd..f69d786 100644
--- a/runtime/jit/jit.cc
+++ b/runtime/jit/jit.cc
@@ -626,6 +626,14 @@
// the file hasn't been sealed writable.
zygote_mapping_methods_ = MemMap::Invalid();
+ // Seal writes now. Zygote and children will map the memory private in order
+ // to write to it.
+ if (fcntl(fd_methods_, F_ADD_SEALS, F_SEAL_SEAL | F_SEAL_WRITE) == -1) {
+ PLOG(WARNING) << "Failed to seal boot image methods file descriptor";
+ code_cache_->GetZygoteMap()->SetCompilationState(ZygoteCompilationState::kNotifiedFailure);
+ return;
+ }
+
std::string error_str;
MemMap child_mapping_methods = MemMap::MapFile(
fd_methods_size_,
@@ -643,39 +651,31 @@
return;
}
- if (!IsSealFutureWriteSupported()) {
- // If we didn't write seal the fd before, seal it now.
-
- if (fcntl(fd_methods_, F_ADD_SEALS, F_SEAL_SEAL | F_SEAL_WRITE) == -1) {
- PLOG(WARNING) << "Failed to seal boot image methods file descriptor";
- code_cache_->GetZygoteMap()->SetCompilationState(ZygoteCompilationState::kNotifiedFailure);
- return;
- }
-
- // Ensure the contents are the same as before: there was a window between
- // the memcpy and the sealing where other processes could have changed the
- // contents.
- offset = 0;
- for (gc::space::ImageSpace* space : Runtime::Current()->GetHeap()->GetBootImageSpaces()) {
- const ImageHeader& header = space->GetImageHeader();
- const ImageSection& section = header.GetMethodsSection();
- // Because mremap works at page boundaries, we can only handle methods
- // within a page range. For methods that falls above or below the range,
- // the child processes will copy their contents to their private mapping
- // in `child_mapping_methods`. See `MapBootImageMethods`.
- uint8_t* page_start = AlignUp(header.GetImageBegin() + section.Offset(), kPageSize);
- uint8_t* page_end =
- AlignDown(header.GetImageBegin() + section.Offset() + section.Size(), kPageSize);
- if (page_end > page_start) {
- uint64_t capacity = page_end - page_start;
- if (memcmp(child_mapping_methods.Begin() + offset, page_start, capacity) != 0) {
- LOG(WARNING) << "Contents differ in boot image methods data";
- code_cache_->GetZygoteMap()->SetCompilationState(
- ZygoteCompilationState::kNotifiedFailure);
- return;
- }
- offset += capacity;
+ // Ensure the contents are the same as before: there was a window between
+ // the memcpy and the sealing where other processes could have changed the
+ // contents.
+ // Note this would not be needed if we could have used F_SEAL_FUTURE_WRITE,
+ // see b/143833776.
+ offset = 0;
+ for (gc::space::ImageSpace* space : Runtime::Current()->GetHeap()->GetBootImageSpaces()) {
+ const ImageHeader& header = space->GetImageHeader();
+ const ImageSection& section = header.GetMethodsSection();
+ // Because mremap works at page boundaries, we can only handle methods
+ // within a page range. For methods that falls above or below the range,
+ // the child processes will copy their contents to their private mapping
+ // in `child_mapping_methods`. See `MapBootImageMethods`.
+ uint8_t* page_start = AlignUp(header.GetImageBegin() + section.Offset(), kPageSize);
+ uint8_t* page_end =
+ AlignDown(header.GetImageBegin() + section.Offset() + section.Size(), kPageSize);
+ if (page_end > page_start) {
+ uint64_t capacity = page_end - page_start;
+ if (memcmp(child_mapping_methods.Begin() + offset, page_start, capacity) != 0) {
+ LOG(WARNING) << "Contents differ in boot image methods data";
+ code_cache_->GetZygoteMap()->SetCompilationState(
+ ZygoteCompilationState::kNotifiedFailure);
+ return;
}
+ offset += capacity;
}
}
@@ -1156,23 +1156,14 @@
return;
}
- if (IsSealFutureWriteSupported()) {
- // Seal now.
- if (fcntl(mem_fd,
- F_ADD_SEALS,
- F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_SEAL | F_SEAL_FUTURE_WRITE) == -1) {
- PLOG(WARNING) << "Failed to seal boot image methods file descriptor";
- zygote_mapping_methods_ = MemMap();
- return;
- }
- } else {
- // Only seal the size. We will seal the write once we are donew writing
- // to the shared mapping.
- if (fcntl(mem_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW) == -1) {
- PLOG(WARNING) << "Failed to seal boot image methods file descriptor";
- zygote_mapping_methods_ = MemMap();
- return;
- }
+ // We should use the F_SEAL_FUTURE_WRITE flag, but this has unexpected
+ // behavior on private mappings after fork (the mapping becomes shared between
+ // parent and children), see b/143833776.
+ // We will seal the write once we are done writing to the shared mapping.
+ if (fcntl(mem_fd, F_ADD_SEALS, F_SEAL_SHRINK | F_SEAL_GROW) == -1) {
+ PLOG(WARNING) << "Failed to seal boot image methods file descriptor";
+ zygote_mapping_methods_ = MemMap();
+ return;
}
fd_methods_ = unique_fd(mem_fd.release());
fd_methods_size_ = total_capacity;