BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf-next] selftests/bpf: Fix the snprintf test
@ 2021-04-28 15:25 Florent Revest
  2021-04-30 17:46 ` Andrii Nakryiko
  2021-04-30 17:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 3+ messages in thread
From: Florent Revest @ 2021-04-28 15:25 UTC (permalink / raw)
  To: bpf; +Cc: ast, daniel, andrii, kpsingh, jackmanb, linux-kernel, Florent Revest

The BPF program for the snprintf selftest runs on all syscall entries.
On busy multicore systems this can cause concurrency issues.

For example it was observed that sometimes the userspace part of the
test reads "    4 0000" instead of "    4 000" (extra '0' at the end)
which seems to happen just before snprintf on another core sets
end[-1] = '\0'.

This patch adds a pid filter to the test to ensure that no
bpf_snprintf() will write over the test's output buffers while the
userspace reads the values.

Fixes: c2e39c6bdc7e ("selftests/bpf: Add a series of tests for bpf_snprintf")
Reported-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Florent Revest <revest@chromium.org>
---
 tools/testing/selftests/bpf/prog_tests/snprintf.c | 2 ++
 tools/testing/selftests/bpf/progs/test_snprintf.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
index a958c22aec75..dffbcaa1ec98 100644
--- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
@@ -43,6 +43,8 @@ void test_snprintf_positive(void)
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
 		return;
 
+	skel->bss->pid = getpid();
+
 	if (!ASSERT_OK(test_snprintf__attach(skel), "skel_attach"))
 		goto cleanup;
 
diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
index 951a0301c553..e35129bea0a0 100644
--- a/tools/testing/selftests/bpf/progs/test_snprintf.c
+++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
@@ -5,6 +5,8 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 
+__u32 pid = 0;
+
 char num_out[64] = {};
 long num_ret = 0;
 
@@ -42,6 +44,9 @@ int handler(const void *ctx)
 	static const char str1[] = "str1";
 	static const char longstr[] = "longstr";
 
+	if ((int)bpf_get_current_pid_tgid() != pid)
+		return 0;
+
 	/* Integer types */
 	num_ret  = BPF_SNPRINTF(num_out, sizeof(num_out),
 				"%d %u %x %li %llu %lX",
-- 
2.31.1.498.g6c1eba8ee3d-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix the snprintf test
  2021-04-28 15:25 [PATCH bpf-next] selftests/bpf: Fix the snprintf test Florent Revest
@ 2021-04-30 17:46 ` Andrii Nakryiko
  2021-04-30 17:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: Andrii Nakryiko @ 2021-04-30 17:46 UTC (permalink / raw)
  To: Florent Revest
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	KP Singh, Brendan Jackman, open list

On Wed, Apr 28, 2021 at 8:25 AM Florent Revest <revest@chromium.org> wrote:
>
> The BPF program for the snprintf selftest runs on all syscall entries.
> On busy multicore systems this can cause concurrency issues.
>
> For example it was observed that sometimes the userspace part of the
> test reads "    4 0000" instead of "    4 000" (extra '0' at the end)
> which seems to happen just before snprintf on another core sets
> end[-1] = '\0'.
>
> This patch adds a pid filter to the test to ensure that no
> bpf_snprintf() will write over the test's output buffers while the
> userspace reads the values.
>
> Fixes: c2e39c6bdc7e ("selftests/bpf: Add a series of tests for bpf_snprintf")
> Reported-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---

Applied to bpf tree. Thanks.

>  tools/testing/selftests/bpf/prog_tests/snprintf.c | 2 ++
>  tools/testing/selftests/bpf/progs/test_snprintf.c | 5 +++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf.c b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> index a958c22aec75..dffbcaa1ec98 100644
> --- a/tools/testing/selftests/bpf/prog_tests/snprintf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/snprintf.c
> @@ -43,6 +43,8 @@ void test_snprintf_positive(void)
>         if (!ASSERT_OK_PTR(skel, "skel_open"))
>                 return;
>
> +       skel->bss->pid = getpid();
> +
>         if (!ASSERT_OK(test_snprintf__attach(skel), "skel_attach"))
>                 goto cleanup;
>
> diff --git a/tools/testing/selftests/bpf/progs/test_snprintf.c b/tools/testing/selftests/bpf/progs/test_snprintf.c
> index 951a0301c553..e35129bea0a0 100644
> --- a/tools/testing/selftests/bpf/progs/test_snprintf.c
> +++ b/tools/testing/selftests/bpf/progs/test_snprintf.c
> @@ -5,6 +5,8 @@
>  #include <bpf/bpf_helpers.h>
>  #include <bpf/bpf_tracing.h>
>
> +__u32 pid = 0;
> +
>  char num_out[64] = {};
>  long num_ret = 0;
>
> @@ -42,6 +44,9 @@ int handler(const void *ctx)
>         static const char str1[] = "str1";
>         static const char longstr[] = "longstr";
>
> +       if ((int)bpf_get_current_pid_tgid() != pid)
> +               return 0;
> +
>         /* Integer types */
>         num_ret  = BPF_SNPRINTF(num_out, sizeof(num_out),
>                                 "%d %u %x %li %llu %lX",
> --
> 2.31.1.498.g6c1eba8ee3d-goog
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH bpf-next] selftests/bpf: Fix the snprintf test
  2021-04-28 15:25 [PATCH bpf-next] selftests/bpf: Fix the snprintf test Florent Revest
  2021-04-30 17:46 ` Andrii Nakryiko
@ 2021-04-30 17:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-04-30 17:50 UTC (permalink / raw)
  To: Florent Revest; +Cc: bpf, ast, daniel, andrii, kpsingh, jackmanb, linux-kernel

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

On Wed, 28 Apr 2021 17:25:01 +0200 you wrote:
> The BPF program for the snprintf selftest runs on all syscall entries.
> On busy multicore systems this can cause concurrency issues.
> 
> For example it was observed that sometimes the userspace part of the
> test reads "    4 0000" instead of "    4 000" (extra '0' at the end)
> which seems to happen just before snprintf on another core sets
> end[-1] = '\0'.
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: Fix the snprintf test
    https://git.kernel.org/bpf/bpf/c/f80f88f0e2f2

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



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-28 15:25 [PATCH bpf-next] selftests/bpf: Fix the snprintf test Florent Revest
2021-04-30 17:46 ` Andrii Nakryiko
2021-04-30 17:50 ` patchwork-bot+netdevbpf

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git