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? > 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> > > [...]
