Skip to content

bpf: roll back stream capacity when allocation fails - #13317

Open
kernel-patches-daemon-bpf[bot] wants to merge 5 commits into
bpf-next_basefrom
series/1146425=>bpf-next
Open

bpf: roll back stream capacity when allocation fails#13317
kernel-patches-daemon-bpf[bot] wants to merge 5 commits into
bpf-next_basefrom
series/1146425=>bpf-next

Conversation

@kernel-patches-daemon-bpf

Copy link
Copy Markdown

Pull request for series with
subject: bpf: roll back stream capacity when allocation fails
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d82ebfc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-review-bot

Copy link
Copy Markdown
commit 581e82235b270d964d05f39923ec1681c268ccbd
Author: Jianlin Shi <shijianlin11@foxmail.com>

bpf: fix stream capacity leak in staging path

This commit fixes a capacity accounting leak in bpf_stream_stage_printk()
where ss->len was incremented before the element allocation, causing
permanent capacity charges for failed allocations.

> diff --git a/kernel/bpf/stream.c b/kernel/bpf/stream.c
> index 0b157ec4e38e..ef5a7b06ffcb 100644
> --- a/kernel/bpf/stream.c
> +++ b/kernel/bpf/stream.c
> @@ -316,17 +316,19 @@ int bpf_stream_stage_printk(struct bpf_stream_stage *ss, const char *fmt, ...)
>  {
>  	struct bpf_bprintf_buffers *buf;
>  	va_list args;
> -	int ret;
> +	int len, ret;
>
>  	if (bpf_try_get_buffers(&buf))
>  		return -EBUSY;
>
>  	va_start(args, fmt);
> -	ret = vsnprintf(buf->buf, ARRAY_SIZE(buf->buf), fmt, args);
> +	/* Cap to the written length, excluding the trailing NUL. */
> +	len = vscnprintf(buf->buf, ARRAY_SIZE(buf->buf), fmt, args);
>  	va_end(args);
> -	ss->len += ret;
>  	/* Exclude NULL byte during push. */
> -	ret = __bpf_stream_push_str(&ss->log, buf->buf, ret);
> +	ret = __bpf_stream_push_str(&ss->log, buf->buf, len);
> +	if (!ret)
> +		ss->len += len;

This isn't a bug, but with the existing '/* Exclude NULL byte during
push. */' a couple of lines down, does the new comment above vscnprintf()
add anything, or could one of the two be dropped?

>  	bpf_put_buffers();
>  	return ret;
>  }



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: fix stream capacity leak in staging path
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31874860107

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: ce7c9f6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: c93cbdb
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 8eb1892
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: d99bda7
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 34e0eb7
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 5fe7007
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 77877bf
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: adb7719
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 6b0835a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 6ab6a94
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

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.

Refactor bpf_stream_release_capacity() to take a length so the consume
and release sides are symmetric, and use it to roll back the charge when
creating the stream element fails.

Fixes: 5ab154f ("bpf: Introduce BPF standard streams")
Signed-off-by: Jianlin Shi <shijianlin11@foxmail.com>
bpf_stream_stage_printk() increments ss->len before pushing the
formatted string to the staging log. If element allocation fails,
ss->len remains inflated and bpf_stream_stage_commit() permanently
charges the stream capacity for data that was never queued.

Only account the string length after a successful push, and use
vscnprintf() so the staged length is the truncated payload without
the trailing NUL.

Fixes: 5ab154f ("bpf: Introduce BPF standard streams")
Signed-off-by: Jianlin Shi <shijianlin11@foxmail.com>
bpf_stream_read() pops and frees stream elements after a successful
copy_to_user(). If a later copy_to_user() fails, it currently restores
only the current element's consumed_len and returns -EFAULT, hiding
bytes already delivered to userspace and making the consumed data
unrecoverable on retry.

On a short copy, keep the successfully copied prefix of the current
element and return the number of bytes copied. Return -EFAULT only when
no bytes were copied for the call.

Fixes: 5ab154f ("bpf: Introduce BPF standard streams")
Signed-off-by: Jianlin Shi <shijianlin11@foxmail.com>
@kernel-patches-daemon-bpf

Copy link
Copy Markdown
Author

Upstream branch: 3d9393f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1146425
version: 3

bstr_printf() returns the would-be length excluding the trailing NUL.
When that value is >= MAX_BPRINTF_BUF the message was truncated, but
bpf_stream_push_str() still tried to allocate with the inflated length
and failed with -ENOMEM. The boundary case of exactly MAX_BPRINTF_BUF
could also copy the trailing NUL into the stream element.

Reject such lengths with -E2BIG before charging stream capacity, and
tighten bpf_stream_elem_alloc() to accept only payloads strictly shorter
than the bprintf buffer.

Fixes: 5ab154f ("bpf: Introduce BPF standard streams")
Signed-off-by: Jianlin Shi <shijianlin11@foxmail.com>
Add coverage for the stream fixes requested on the capacity rollback
series:

- oversized bpf_stream_printk() returns -E2BIG and does not leak
  capacity for a subsequent successful write;
- bpf_prog_stream_read() returns the successfully copied prefix when
  the userspace buffer straddles an unmapped page.

Signed-off-by: Jianlin Shi <shijianlin11@foxmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant