[PATCH 0/2] Allow bpf_user_ringbuf_drain() callbacks to return 1

[PATCH 0/2] Allow bpf_user_ringbuf_drain() callbacks to return 1

[David Vernet]

The bpf_user_ringbuf_drain() helper function allows a BPF program to
specify a callback that is invoked when draining entries from a
BPF_MAP_TYPE_USER_RINGBUF ring buffer map. The API is meant to allow the
callback to return 0 if it wants to continue draining samples, and 1 if
it's done draining. Unfortunately, bpf_user_ringbuf_drain() landed shortly
after commit 1bfe26fb0827 ("bpf: Add verifier support for custom
callback return range"), which changed the default behavior of callbacks
to only support returning 0, and the corresponding necessary change to
bpf_user_ringbuf_drain() callbacks was missed.

This patch set fixes this oversight, and updates the user_ringbuf
selftests to return 1 in a callback to catch future instances of
regression.

This patch set should be merged to the bpf tree.

David Vernet (2):
  bpf: Allow bpf_user_ringbuf_drain() callbacks to return 1
  selftests/bpf: Make bpf_user_ringbuf_drain() selftest callback return
    1

 kernel/bpf/verifier.c                                    | 1 +
 tools/testing/selftests/bpf/progs/user_ringbuf_success.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.38.0

[PATCH 1/2] bpf: Allow bpf_user_ringbuf_drain() callbacks to return 1

[David Vernet]

The bpf_user_ringbuf_drain() helper function allows a BPF program to
specify a callback that is invoked when draining entries from a
BPF_MAP_TYPE_USER_RINGBUF ring buffer map. The API is meant to allow the
callback to return 0 if it wants to continue draining samples, and 1 if
it's done draining. Unfortunately, bpf_user_ringbuf_drain() landed shortly
after commit 1bfe26fb0827 ("bpf: Add verifier support for custom
callback return range"), which changed the default behavior of callbacks
to only support returning 0.

This patch corrects that oversight by allowing bpf_user_ringbuf_drain()
callbacks to return 0 or 1. A follow-on patch will update the
user_ringbuf selftests to also return 1 from a bpf_user_ringbuf_drain()
callback to prevent this from regressing in the future.

Fixes: 205715673844 ("bpf: Add bpf_user_ringbuf_drain() helper")
Signed-off-by: David Vernet <void@manifault.com>
---
 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6f6d2d511c06..9ab7188d8f68 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6946,6 +6946,7 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env,
 	__mark_reg_not_init(env, &callee->regs[BPF_REG_5]);
 
 	callee->in_callback_fn = true;
+	callee->callback_ret_range = tnum_range(0, 1);
 	return 0;
 }
 
-- 
2.38.0

[PATCH 2/2] selftests/bpf: Make bpf_user_ringbuf_drain() selftest callback return 1

[David Vernet]

In commit 1bfe26fb0827 ("bpf: Add verifier support for custom callback
return range"), the verifier was updated to require callbacks to BPF
helpers to explicitly specify the range of values that can be returned.
bpf_user_ringbuf_drain() was merged after this in commit 205715673844
("bpf: Add bpf_user_ringbuf_drain() helper"), and this change in default
behavior was missed. This patch updates the BPF_MAP_TYPE_USER_RINGBUF
selftests to also return 1 from a bpf_user_ringbuf_drain() callback so
as to properly test this going forward.

Signed-off-by: David Vernet <void@manifault.com>
---
 tools/testing/selftests/bpf/progs/user_ringbuf_success.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_success.c b/tools/testing/selftests/bpf/progs/user_ringbuf_success.c
index 099c23d9aa21..b39093dd5715 100644
--- a/tools/testing/selftests/bpf/progs/user_ringbuf_success.c
+++ b/tools/testing/selftests/bpf/progs/user_ringbuf_success.c
@@ -47,14 +47,14 @@ record_sample(struct bpf_dynptr *dynptr, void *context)
 		if (status) {
 			bpf_printk("bpf_dynptr_read() failed: %d\n", status);
 			err = 1;
-			return 0;
+			return 1;
 		}
 	} else {
 		sample = bpf_dynptr_data(dynptr, 0, sizeof(*sample));
 		if (!sample) {
 			bpf_printk("Unexpectedly failed to get sample\n");
 			err = 2;
-			return 0;
+			return 1;
 		}
 		stack_sample = *sample;
 	}
-- 
2.38.0

Re: [PATCH 0/2] Allow bpf_user_ringbuf_drain() callbacks to return 1

[Andrii Nakryiko]

On Wed, Oct 12, 2022 at 4:20 PM David Vernet <void@manifault.com> wrote:
>
> The bpf_user_ringbuf_drain() helper function allows a BPF program to
> specify a callback that is invoked when draining entries from a
> BPF_MAP_TYPE_USER_RINGBUF ring buffer map. The API is meant to allow the
> callback to return 0 if it wants to continue draining samples, and 1 if
> it's done draining. Unfortunately, bpf_user_ringbuf_drain() landed shortly
> after commit 1bfe26fb0827 ("bpf: Add verifier support for custom
> callback return range"), which changed the default behavior of callbacks
> to only support returning 0, and the corresponding necessary change to
> bpf_user_ringbuf_drain() callbacks was missed.
>
> This patch set fixes this oversight, and updates the user_ringbuf
> selftests to return 1 in a callback to catch future instances of
> regression.
>
> This patch set should be merged to the bpf tree.

Please tag patch as [PATCH bpf x/N] next time. This will make it clear
that it's targeted against the bpf tree and will let our CI know that
it should be applied and tested in bpf (it chooses bpf-next by
default).

>
> David Vernet (2):
>   bpf: Allow bpf_user_ringbuf_drain() callbacks to return 1
>   selftests/bpf: Make bpf_user_ringbuf_drain() selftest callback return
>     1
>
>  kernel/bpf/verifier.c                                    | 1 +
>  tools/testing/selftests/bpf/progs/user_ringbuf_success.c | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> --
> 2.38.0
>

Re: [PATCH 0/2] Allow bpf_user_ringbuf_drain() callbacks to return 1

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 12 Oct 2022 18:20:13 -0500 you wrote:
> The bpf_user_ringbuf_drain() helper function allows a BPF program to
> specify a callback that is invoked when draining entries from a
> BPF_MAP_TYPE_USER_RINGBUF ring buffer map. The API is meant to allow the
> callback to return 0 if it wants to continue draining samples, and 1 if
> it's done draining. Unfortunately, bpf_user_ringbuf_drain() landed shortly
> after commit 1bfe26fb0827 ("bpf: Add verifier support for custom
> callback return range"), which changed the default behavior of callbacks
> to only support returning 0, and the corresponding necessary change to
> bpf_user_ringbuf_drain() callbacks was missed.
> 
> [...]

Here is the summary with links:
  - [1/2] bpf: Allow bpf_user_ringbuf_drain() callbacks to return 1
    https://git.kernel.org/bpf/bpf/c/c92a7a522438
  - [2/2] selftests/bpf: Make bpf_user_ringbuf_drain() selftest callback return 1
    https://git.kernel.org/bpf/bpf/c/6e44b9f375a3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html