Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Prevend unsoundness in environments with broken madvise(MADV_DONTNEED)
  • Loading branch information
koute committed Jun 21, 2022
commit d6e2395e658ccef69780bdd73b09f5f4b49b8838
84 changes: 74 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions client/executor/wasmtime/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ sp-runtime-interface = { version = "6.0.0", path = "../../../primitives/runtime-
sp-sandbox = { version = "0.10.0-dev", path = "../../../primitives/sandbox" }
sp-wasm-interface = { version = "6.0.0", features = ["wasmtime"], path = "../../../primitives/wasm-interface" }

[target.'cfg(target_os = "linux")'.dependencies]
rustix = { version = "0.35.6", default-features = false, features = ["mm", "fs"] }
once_cell = "1.12.0"

[dev-dependencies]
wat = "1.0"
sc-runtime-test = { version = "2.0.0", path = "../runtime-test" }
Expand Down
15 changes: 11 additions & 4 deletions client/executor/wasmtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
use crate::{
host::HostState,
instance_wrapper::{EntryPoint, InstanceWrapper},
util,
util::{self, replace_strategy_if_broken},
};

use sc_allocator::FreeingBumpHeapAllocator;
Expand Down Expand Up @@ -412,6 +412,7 @@ fn common_config(semantics: &Semantics) -> std::result::Result<wasmtime::Config,
/// See [here][stack_height] for more details of the instrumentation
///
/// [stack_height]: https://github.com/paritytech/wasm-utils/blob/d9432baf/src/stack_height/mod.rs#L1-L50
#[derive(Clone)]
pub struct DeterministicStackLimit {
/// A number of logical "values" that can be pushed on the wasm stack. A trap will be triggered
/// if exceeded.
Expand Down Expand Up @@ -469,6 +470,7 @@ enum InternalInstantiationStrategy {
Builtin,
}

#[derive(Clone)]
pub struct Semantics {
/// The instantiation strategy to use.
pub instantiation_strategy: InstantiationStrategy,
Expand Down Expand Up @@ -599,11 +601,13 @@ where
/// [`create_runtime_from_artifact`] to get more details.
unsafe fn do_create_runtime<H>(
code_supply_mode: CodeSupplyMode<'_>,
config: Config,
mut config: Config,
) -> std::result::Result<WasmtimeRuntime, WasmError>
where
H: HostFunctions,
{
replace_strategy_if_broken(&mut config.semantics.instantiation_strategy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do this call in common_config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it, but honestly that's kind of messy, since the instantiation_strategy is also used outside of common_config, so common_config would have to modify the semantics which is passed to it, and if it did that then instead of doing only one thing (creating a config for the engine) it'd be doing two things (creating a config and replacing the strategy), so I felt it'd be cleaner to just do it outside of common_config and have it done explicitly.


let mut wasmtime_config = common_config(&config.semantics)?;
if let Some(ref cache_path) = config.cache_path {
if let Err(reason) = setup_wasmtime_caching(cache_path, &mut wasmtime_config) {
Expand Down Expand Up @@ -720,9 +724,12 @@ pub fn prepare_runtime_artifact(
blob: RuntimeBlob,
semantics: &Semantics,
) -> std::result::Result<Vec<u8>, WasmError> {
let blob = prepare_blob_for_compilation(blob, semantics)?;
let mut semantics = semantics.clone();
replace_strategy_if_broken(&mut semantics.instantiation_strategy);

let blob = prepare_blob_for_compilation(blob, &semantics)?;

let engine = Engine::new(&common_config(semantics)?)
let engine = Engine::new(&common_config(&semantics)?)
.map_err(|e| WasmError::Other(format!("cannot create the engine: {}", e)))?;

engine
Expand Down
90 changes: 89 additions & 1 deletion client/executor/wasmtime/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use crate::runtime::StoreData;
use crate::{runtime::StoreData, InstantiationStrategy};
use sc_executor_common::{
error::{Error, Result},
util::checked_range,
Expand Down Expand Up @@ -98,3 +98,91 @@ pub(crate) fn write_memory_from(
memory[range].copy_from_slice(data);
Ok(())
}

/// Checks whether the `madvise(MADV_DONTNEED)` works as expected.
///
/// In certain environments (e.g. when running under the QEMU user-mode emulator)
/// this syscall is broken.
#[cfg(target_os = "linux")]
fn is_madvise_working() -> std::result::Result<bool, String> {
unsafe {
// Allocate two memory pages.
let pointer = rustix::mm::mmap_anonymous(
std::ptr::null_mut(),
2 * 4096,
rustix::mm::ProtFlags::READ | rustix::mm::ProtFlags::WRITE,
rustix::mm::MapFlags::PRIVATE,
)
.map_err(|error| format!("mmap failed: {}", error))?;

// Dirty them both.
std::ptr::write_volatile(pointer.cast::<u8>(), b'A');
std::ptr::write_volatile(pointer.cast::<u8>().add(4096), b'B');

// Clear the first page.
let result_madvise = rustix::mm::madvise(pointer, 4096, rustix::mm::Advice::LinuxDontNeed)
.map_err(|error| format!("madvise failed: {}", error));

// Fetch the values.
let value_1 = std::ptr::read_volatile(pointer.cast::<u8>());
let value_2 = std::ptr::read_volatile(pointer.cast::<u8>().add(4096));

let result_munmap = rustix::mm::munmap(pointer, 2 * 4096)
.map_err(|error| format!("munmap failed: {}", error));

result_madvise?;
result_munmap?;

// Verify that the first page was cleared, while the second one was not.
Ok(value_1 == 0 && value_2 == b'B')
}
}

#[cfg(test)]
#[cfg(target_os = "linux")]
#[test]
fn test_is_madvise_working() {
assert_eq!(is_madvise_working(), Ok(true));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want this test, especially when someone maybe runs the tests in Qemu or similar :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... well, maybe, but I'd probably just leave this here and see if anyone complains. Maybe it'll motivate someone to actually fix QEMU. :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I would complain, because I sometimes run all tests on my virtual machine that is running on Mac os :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By VM do you mean an actual VM which emulates a whole machine with a full Linux running under it? Because that should work just fine and not be affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, fine, I've changed this test to check that it never returns an Err instead of it checking that madvise always works. So now it should never fail.


/// Checks whether a given instantiation strategy can be safely used, and replaces
/// it with a slower (but sound) alternative if it isn't.
#[cfg(target_os = "linux")]
pub(crate) fn replace_strategy_if_broken(strategy: &mut InstantiationStrategy) {
let replacement_strategy = match *strategy {
// These strategies don't need working `madvise`.
InstantiationStrategy::Pooling | InstantiationStrategy::RecreateInstance => return,

// These strategies require a working `madvise` to be sound.
InstantiationStrategy::PoolingCopyOnWrite => InstantiationStrategy::Pooling,
InstantiationStrategy::RecreateInstanceCopyOnWrite |
InstantiationStrategy::LegacyInstanceReuse => InstantiationStrategy::RecreateInstance,
};

use once_cell::sync::OnceCell;
static IS_OK: OnceCell<bool> = OnceCell::new();

let is_ok = IS_OK.get_or_init(|| {
let is_ok = match is_madvise_working() {
Ok(is_ok) => is_ok,
Err(error) => {
// This should never happen.
log::warn!("Failed to check whether `madvise(MADV_DONTNEED)` works: {}", error);
false
}
};

if !is_ok {
log::warn!("You're running on a system with a broken `madvise(MADV_DONTNEED)` implementation. This will result in lower performance.");
}

is_ok
});

if !is_ok {
*strategy = replacement_strategy;
}
}

#[cfg(not(target_os = "linux"))]
pub(crate) fn replace_strategy_if_broken(_: &mut InstantiationStrategy) {}