On Tue, 14 May 2024 at 16:12, Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > Full disclosure I hit a KASAN OOB read warning in BPF when testing > on Meta's production servers (which load a lot of BPF). > BPF folks aren't super alarmed by it, and also they are partying at > LSFMM so I don't think it's worth waiting for the fix. > But you may feel differently... https://pastebin.com/0fzqy3cW Hmm. As long as people are aware of it, I don't think a known issue needs to hold up any pull request. Even if that whole 'struct bpf_map can be embedded in many different structures", combined with "users just magically know which structure it is and use container_of()" looks like a horrid pattern. Why does it do that disgusting struct bpf_array *array = container_of(map, struct bpf_array, map); ... *insn++ = BPF_ALU32_IMM(BPF_AND, BPF_REG_0, array->index_mask); thing? As far as I can tell, a bpf map can be embedded in many different structures, not just that 'bpf_array' thing. That spectre-v1 code generation is disgusting. But worse, it's stupid. The way to turn the index into a data dependency isn't to just 'and' it with some fixed mask (that is wrong anyway and requires that whole "round up to the next power-of-two), it's to just teach the JIT to generate the proper Spectre-v1 sequence. So that code should be able to rely purely on map->max_entries, and not do that disgusting "look up struct 'bpf_array'" Anyway, I've pulled it - the bpf code looks broken, but it looks fairly straightforward to do it right if I understood that code correctly. Linus
