Re: [PATCH bpf-next v3 1/5] bpf: roll back stream capacity when allocation fails

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch addresses a stream capacity leak in `bpf_stream_push_str()` when element allocation fails, but leaves an identical leak in the stream staging API (`bpf_stream_stage_printk`).
- [Medium] Unbounded return values from `vsnprintf()` and `bstr_printf()` are passed directly to stream push operations, leading to inadvertently dropped messages and aborted stack traces.
--

--- Patch [1]: [PATCH bpf-next v3 1/5] bpf: roll back stream capacity when allocation fails ---
commit 057b94c20f902b9cfbc6a855c11361e82913b675
Author: Jianlin Shi <[email protected]>

bpf: roll back stream capacity when allocation fails

bpf_stream_push_str() accounts the string length before allocating a
stream element. If the allocation fails, the length remains charged even
though no element is queued and therefore cannot be released by a reader.
Repeated failures can exhaust the stream capacity permanently until the
BPF program is freed.

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index be9ce98e94692..0b157ec4e38eb 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c

[ ... ]

> @@ -79,7 +77,14 @@ static int bpf_stream_push_str(struct bpf_stream *stream, const char *str, int l
>  {
>  	int ret = bpf_stream_consume_capacity(stream, len);
>  
> -	return ret ?: __bpf_stream_push_str(&stream->log, str, len);
> +	if (ret)
> +		return ret;
> +
> +	ret = __bpf_stream_push_str(&stream->log, str, len);
> +	if (ret)
> +		bpf_stream_release_capacity(stream, len);
> +
> +	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
capacity leak here, does the stream staging API have an identical leak?

Looking at bpf_stream_stage_printk() in kernel/bpf/stream.c:

    ss->len += ret;
    /* Exclude NULL byte during push. */
    ret = __bpf_stream_push_str(&ss->log, buf->buf, ret);

Here ss->len is unconditionally incremented even if __bpf_stream_push_str()
fails. Later, bpf_stream_stage_commit() consumes this inflated ss->len
from the capacity:

    ret = bpf_stream_consume_capacity(stream, ss->len);

Because the failed element is never added to the stream queue, it will
never be popped by a reader, which permanently leaks the capacity. Should
this be rolled back similarly?

[Severity: Medium]
This is also a pre-existing issue, but while reviewing the length handling,
could the unbounded return values from vsnprintf() and bstr_printf() cause
dropped messages?

In bpf_stream_vprintk():

    ret = bstr_printf(data.buf, MAX_BPRINTF_BUF, fmt__str, data.bin_args);
    /* Exclude NULL byte during push. */
    ret = bpf_stream_push_str(stream, data.buf, ret);

And similarly in bpf_stream_stage_printk():

    ret = vsnprintf(buf->buf, ARRAY_SIZE(buf->buf), fmt, args);
    ...
    ret = __bpf_stream_push_str(&ss->log, buf->buf, ret);

Both vsnprintf() and bstr_printf() return the unbounded number of
characters that would have been written, even if truncated. If this length
exceeds MAX_BPRINTF_BUF (1024 bytes), passing it as the length to
bpf_stream_push_str() or __bpf_stream_push_str() will cause
bpf_stream_elem_alloc() to return NULL.

Instead of pushing the successfully truncated string, the entire message
is dropped. Furthermore, in bpf_stream_stage_dump_stack(), this -ENOMEM
return causes the arch_bpf_stack_walk to prematurely abort. Should the
length be capped before being passed to the push functions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1