Re: [PATCH net-next v2 2/4] bpf: test_run: Use system page pool for XDP live frame mode

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Jesper Dangaard Brouer <hawk@kernel.org> writes:

> On 21/02/2024 15.48, Toke Høiland-Jørgensen wrote:
>> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>> 
>>> The cookie is a random 128-bit value, which means the probability that
>>> we will get accidental collisions (which would lead to recycling the
>>> wrong page values and reading garbage) is on the order of 2^-128. This
>>> is in the "won't happen before the heat death of the universe" range, so
>>> this marking is safe for the intended usage.
>> 
>> Alright, got a second opinion on this from someone better at security
>> than me; I'll go try out some different ideas :)
>
> It is a general security concern for me that BPF test_run gets access to
> memory used by 'system page pool', with the concern of leaking data
> (from real traffic) to an attacker than can inject a BPF test_run
> program via e.g. a CI pipeline.
>
> I'm not saying we leaking data today in BPF/XDP progs, but there is a
> potential, because to gain performance in XDP and page_pool we don't
> clear memory to avoid cache line performance issues.
> I guess today, I could BPF tail extend and read packet data from older
> frames, in this way, if I get access to 'system page pool'.

I agree that the leak concern is non-trivial (also of the secret cookie
value), so I am not planning to re-submit with that approach. I got
half-way revising the patches to use the system PP but always
re-initialise the pages before the merge window. This comes with a ~7%
overhead on small packets and probably more with big frames (due to the
memcpy() of the payload data).

Due to this, my current plan is to take a hybrid approach, depending on
the 'repetitions' parameter: for low repetition counts, just
pre-allocate a bunch of pages from the system PP at setup time,
initialise them, and don't bother with recycling. And for large
repetition counts, keep the current approach of allocating a separate PP
for each syscall invocation. The threshold for when something is a "low"
number is the kicker here, of course, but probably some static value can
be set as a threshold; I'll play around with this and see what makes
sense.

WDYT about this?

-Toke