Re: [PATCH bpf-next v2] bpf: roll back stream capacity when allocation fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fixes a capacity leak in `bpf_stream_push_str()` when stream element allocation fails, but misses an identical leak in `bpf_stream_stage_printk()`.
- [High] `bpf_stream_read` drops successfully read data if a subsequent `copy_to_user` fails, violating API contracts.
- [Medium] Log messages are entirely dropped and return an error instead of pushing the truncated message when the formatted output exceeds the internal string buffer size.
--

--- Patch [1]: [PATCH bpf-next v2] bpf: roll back stream capacity when allocation fails ---
commit 3fa8598d2b8afa51091cf463bee21d3c9f140975
Author: Jianlin Shi <shijianlin11@foxmail.com>

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.

Roll back the capacity charge when creating the stream element fails.

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index be9ce98e94692..4b8a74b9162e1 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c
> @@ -79,7 +79,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)
> +		atomic_sub(len, &stream->capacity);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does bpf_stream_stage_printk() suffer from
the identical capacity leak that this patch fixes here?

Looking at bpf_stream_stage_printk():

kernel/bpf/stream.c:bpf_stream_stage_printk() {
    ...
	ss->len += ret;
	/* Exclude NULL byte during push. */
	ret = __bpf_stream_push_str(&ss->log, buf->buf, ret);
    ...
}

If __bpf_stream_push_str() fails to allocate memory, ss->len remains incremented.
Later, bpf_stream_stage_commit() unconditionally consumes this erroneously
inflated ss->len from the stream capacity:

kernel/bpf/stream.c:bpf_stream_stage_commit() {
    ...
	ret = bpf_stream_consume_capacity(stream, ss->len);
	if (ret)
		return ret;
    ...
}

Since the failed element isn't in the log, its length will never be released,
which could permanently leak stream capacity over time.

[Severity: Medium]
This is a pre-existing issue, but what happens if the formatted output exceeds
the internal string buffer size?

Both bstr_printf() and vsnprintf() return the number of characters that would
have been written, not the actual number written.

For example, in bpf_stream_vprintk():

kernel/bpf/stream.c: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);
    ...
}

If the string is truncated, the un-capped return value is passed as len to
bpf_stream_push_str(). Since bpf_stream_elem_alloc() has a strict bounds check,
it will reject the allocation and return -ENOMEM.

Does this mean the entire log message is dropped instead of safely pushing the
truncated message prefix?

[Severity: High]
This is a pre-existing issue, but does bpf_stream_read() permanently drop data
if a subsequent copy_to_user() fails?

In bpf_stream_read():

kernel/bpf/stream.c:bpf_stream_read() {
    ...
		/* Restore in case of error. */
		if (ret) {
			ret = -EFAULT;
			elem->consumed_len = cons_len;
			break;
		}
    ...
	return ret ? ret : len - rem_len;
}

If copy_to_user() fails on a subsequent element in the loop, it breaks and
unconditionally returns -EFAULT.

Since elements that were successfully read in previous iterations have already
been popped and freed, returning an error instead of the number of successfully
read bytes hides the fact that partial data was consumed. 

Will retrying the read operation cause the previously read data to be
permanently lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_C919BB32458A4DAD645A68F441345B971E05@qq.com?part=1