Received-SPF: pass (google.com: domain of linux-kernel+bounces-179375-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240514231155.1004295-1-kuba@kernel.org>
In-Reply-To: <20240514231155.1004295-1-kuba@kernel.org>
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Tue, 14 May 2024 20:32:24 -0700
Message-ID: <CAHk-=wiSiGppp-J25Ww-gN6qgpc7gZRb_cP+dn3Q8_zdntzgYQ@mail.gmail.com>
Subject: Re: [GIT PULL] Networking for v6.10
To: Jakub Kicinski <kuba@kernel.org>
Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, 
	pabeni@redhat.com, bpf@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Tue, 14 May 2024 at 16:12, Jakub Kicinski <kuba@kernel.org> 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