On Fri, Jan 13, 2023 at 12:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2023-01-11 at 16:24 -0800, Andrii Nakryiko wrote: > [...] > > > > I'm wondering if we should consider allowing uninitialized > > (STACK_INVALID) reads from stack, in general. It feels like it's > > causing more issues than is actually helpful in practice. Common code > > pattern is to __builtin_memset() some struct first, and only then > > initialize it, basically doing unnecessary work of zeroing out. All > > just to avoid verifier to complain about some irrelevant padding not > > being initialized. I haven't thought about this much, but it feels > > that STACK_MISC (initialized, but unknown scalar value) is basically > > equivalent to STACK_INVALID for all intents and purposes. Thoughts? > > Do you have an example of the __builtin_memset() usage? > I tried passing partially initialized stack allocated structure to > bpf_map_update_elem() and bpf_probe_write_user() and verifier did not > complain. > > Regarding STACK_MISC vs STACK_INVALID, I think it's ok to replace > STACK_INVALID with STACK_MISC if we are talking about STX/LDX/ALU > instructions because after LDX you would get a full range register and > you can't do much with a full range value. However, if a structure > containing un-initialized fields (e.g. not just padding) is passed to > a helper or kfunc is it an error? if we are passing stack as a memory to helper/kfunc (which should be the only valid use case with STACK_MISC, right?), then I think we expect helper/kfunc to treat it as memory with unknowable contents. Not sure if I'm missing something, but MISC says it's some unknown value, and the only difference between INVALID and MISC is that MISC's value was written by program explicitly, while for INVALID that garbage value was there on the stack already (but still unknowable scalar), which effectively is the same thing. > > > Obviously, this is a completely separate change and issue from what > > you are addressing in this patch set. > > > > Awesome job on tracking this down and fixing it! For the patch set: > > Thank you for reviewing this issue with me. > > > > > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > > > [...]
