On Mon, Jun 23, 2025 at 8:13 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> +
> +static struct bpf_stream_page *bpf_stream_page_replace(void)
> +{
> + struct bpf_stream_page *stream_page, *old_stream_page;
> + struct page *page;
> +
> + page = __bpf_alloc_page(NUMA_NO_NODE);
> + if (!page)
> + return NULL;
__bpf_alloc_page() is using GFP_ACCOUNT in both nolock and normal cases,
but active_memcg is random at this point, so the page accounting
is incorrect.
I think we need to remember objcg in prog_aux similar to what
we do with maps, and then store that in bpf_stream and stream_stage
objects in corresponding init() functions.
Then do set_active_memcg() here the way we do in bpf_mem_alloc
and in map_*alloc()s.
Or use alloc_page_nolock() directly here without GFP_ACCOUNT.
Also I think it's strange to limit kernel messages to 4M,
since the kernel messages are essential debug info.
While it makes sense to limit kfunc's spam.
I suspect the idea here is to avoid OOM if the kernel is spammy
due to malicious bpf prog that forces the kernel to warn so much?
But dmesg doesn't do it.
And 4M * 2 * number_of_progs can be many gigabytes.
Maybe let's drop stream->capacity and rely on memcg to limit
the spam for both kernel and kfunc?
Accounting a page at a time seems sufficient.
> + ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt__str, data.bin_args);
> + /* If the string was truncated, we only wrote until the size of buffer. */
> + ret = min_t(u32, ret + 1, MAX_BPRINTF_BUF);
> + ret = bpf_stream_push_str(stream, data.buf, ret);
We discussed it offline, so mentioning here for the list.
Let's not emit \0 into the stream. Looks unnecessary.
