paritytech / polkavm

A fast and secure RISC-V based virtual machine
Apache License 2.0
251 stars 58 forks source link

Stack size as a property of an instance #194

Open s0me0ne-unkn0wn opened 4 weeks ago

s0me0ne-unkn0wn commented 4 weeks ago

Currently, stack size is defined as a property of program blob/module. There are two problems with that: 1) The default value is too small without any obvious way to bump it (there's logic that allows setting it from the value stored in .polkavm_min_stack_size ELF section, but IIUC no means are provided for end developers to actually set this value); 2) To ensure deterministic execution in a parametrized execution environment, the stack size should be a property of an instance, not a module.

I'd propose to replace (or supplement) the Module::instantiate() with something like Module::instance_builder().with_stack_size(...).build().

If we agree on the necessity of the implementation described and on the interface, I'm ready to work on it.

koute commented 4 weeks ago
  1. The default value is too small without any obvious way to bump it (there's logic that allows setting it from the value stored in .polkavm_min_stack_size ELF section, but IIUC no means are provided for end developers to actually set this value);

I haven't added a macro to set it, but it should be relatively simple to add one in the polkavm_derive crate.

To ensure deterministic execution in a parametrized execution environment, the stack size should be a property of an instance, not a module.

Hm? I don't understand. How is making it a property of the instance non-deterministic? (:

If anything I imagine that making it a property of the module is the way to make it deterministic, because the module itself should know how much stack it needs, and not the execution environment (so the execution environment never needs to know how much stack space the module needs).

s0me0ne-unkn0wn commented 4 weeks ago

Well, when you put it like that, I'm starting to argue my own point :thinking:

I'm thinking about the PVF use case, of course, and we always wanted the stack size to be deterministically controllable by the environment. First, because we wanted to have a hard limit for that, and second, because we wanted to be able to bump that limit for the whole environment if needed.

But if we make it module-defined... Well, okay, we can rule out modules going over the hard limit on pre-checking. We also would need a parachain to perform a runtime upgrade if it finds it needs more stack -- but that, indeed, increases granularity. Do we really need it to be environment-controlled? Let me think a bit more about that.

In the meantime, I'll try to add a macro to define the stack size. One page is obviously way too small even for a simplest PVFs (one more little bug report is that the stack overflow looks like a nameless "Trap" without debug enabled, and even with full tracing you need quite some time to figure out that the out-of-bound memory region write is indeed a stack overflow).

koute commented 4 weeks ago

one more little bug report is that the stack overflow looks like a nameless "Trap" without debug enabled, and even with full tracing you need quite some time to figure out that the out-of-bound memory region write is indeed a stack overflow

Yes, because you do get a trap on any out of bounds memory access (and unlike WASM the stack here is just normal memory), and the VM currently doesn't print out which exact memory address was accessed. So this is not a bug, just a missing feature. It wouldn't be that hard for the VM to also grab the address which the guest tried to access, check whether it's near the stack, and print out that it's most likely a stack overflow.