All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support
@ 2023-02-08 20:56 Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 1/9] selftests/bpf: Quote host tools Ilya Leoshkevich
                   ` (8 more replies)
  0 siblings, 9 replies; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

Hi,

This series adds support for building selftests with Memory Sanitizer
[1] - a compiler instrumentation for detecting usages of undefined
memory.

The primary motivation is to make sure that such usages do not occur
during testing, since the ones that have not been caught yet are likely
to affect the CI results on s390x. The secondary motivation is to be
able to use libbpf in applications instrumented with MSan (it requires
all code running in a process to be instrumented).

MSan has found one real issue (fixed by patch 6), and of course a
number of false positives. This rest of this series deals with
preparing the build infrastructure and adding MSan annotations to
libbpf and selftests.

The setup I'm using is as follows:

- Instrumented zlib-ng and patched elfutils [2].
- Patched LLVM [3, 4].
- Clang-built kernel.
- Building tests with MSan:

      make \
          -C tools/testing/selftests/bpf \
          CC="ccache clang-17" \
          LD=ld \
          HOSTCC="ccache clang-17" \
          HOSTLD=ld \
          LLVM=1 \
          LLVM_SUFFIX=-17 \
          OBJCOPY=objcopy \
          CLANG="ccache clang-17" \
          LLVM_STRIP=llvm-strip-17 \
          LLC=llc-17 \
          BPF_GCC= \
          SAN_CFLAGS="-fsanitize=memory \
                      -fsanitize-memory-track-origins \
                      -I$PWD/../zlib-ng/dist/include \
                      -I$PWD/../elfutils/dist/include" \
          SAN_LDFLAGS="-fsanitize=memory \
                       -fsanitize-memory-track-origins \
                       -L$PWD/../zlib-ng/dist/lib \
                       -L$PWD/../elfutils/dist/lib" \
          CROSS_COMPILE=s390x-linux-gnu-

  It's a lot of options, but most of them are trivial: setting up LLVM
  toolchain, taking in account s390x quirks, setting up MSan and
  disabling bpf-gcc. The CROSS_COMPILE one is a hack: instrumenting
  bpftool turned out to be too complicated from the build system
  perspective, so having CROSS_COMPILE forces compiling the host libbpf
  uninstrumented and guest libbpf instrumented.

- Running tests with MSan on s390x:

      LD_LIBRARY_PATH=<instrumented libs> ./test_progs
      ...
      Summary: 282/1624 PASSED, 23 SKIPPED, 4 FAILED
  
  The 4 failures happen without MSan too, they are already known and
  denylisted.

Best regards,
Ilya

[1] https://clang.llvm.org/docs/MemorySanitizer.html
[2] https://sourceware.org/pipermail/elfutils-devel/2023q1/005831.html
[3] https://reviews.llvm.org/D143296
[4] https://reviews.llvm.org/D143330

Ilya Leoshkevich (9):
  selftests/bpf: Quote host tools
  tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support
  selftests/bpf: Split SAN_CFLAGS and SAN_LDFLAGS
  selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and
    libbpf
  selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach
  selftests/bpf: Attach to fopen()/fclose() in attach_probe
  libbpf: Fix alen calculation in libbpf_nla_dump_errormsg()
  libbpf: Add MSan annotations
  selftests/bpf: Add MSan annotations

 tools/bpf/runqslower/Makefile                 |  6 ++
 tools/lib/bpf/bpf.c                           | 70 +++++++++++++++++--
 tools/lib/bpf/btf.c                           |  1 +
 tools/lib/bpf/libbpf.c                        |  1 +
 tools/lib/bpf/libbpf_internal.h               | 14 ++++
 tools/lib/bpf/nlattr.c                        |  2 +-
 tools/testing/selftests/bpf/Makefile          | 17 +++--
 tools/testing/selftests/bpf/cap_helpers.c     |  3 +
 .../selftests/bpf/prog_tests/attach_probe.c   | 10 +--
 .../selftests/bpf/prog_tests/bpf_obj_id.c     | 10 +++
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     |  3 +
 tools/testing/selftests/bpf/prog_tests/btf.c  | 11 +++
 .../selftests/bpf/prog_tests/send_signal.c    |  2 +
 .../bpf/prog_tests/tp_attach_query.c          |  6 ++
 .../bpf/prog_tests/uprobe_autoattach.c        | 12 ++--
 .../selftests/bpf/prog_tests/xdp_bonding.c    |  3 +
 .../selftests/bpf/progs/test_attach_probe.c   |  8 ++-
 .../bpf/progs/test_uprobe_autoattach.c        | 10 +--
 tools/testing/selftests/bpf/xdp_synproxy.c    |  2 +
 19 files changed, 161 insertions(+), 30 deletions(-)

-- 
2.39.1


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

* [PATCH bpf-next 1/9] selftests/bpf: Quote host tools
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support Ilya Leoshkevich
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

Using HOSTCC="ccache clang" breaks building the tests, since, when it's
forwarded to e.g. bpftool, the child make sees HOSTCC=ccache and
"clang" is considered a target. Fix by quoting it, and also HOSTLD and
HOSTAR for consistency.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index b2eb3201b85a..49bc1ba12d3a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -248,7 +248,7 @@ BPFTOOL ?= $(DEFAULT_BPFTOOL)
 $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
 		    $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool
 	$(Q)$(MAKE) $(submake_extras)  -C $(BPFTOOLDIR)			       \
-		    ARCH= CROSS_COMPILE= CC=$(HOSTCC) LD=$(HOSTLD) 	       \
+		    ARCH= CROSS_COMPILE= CC="$(HOSTCC)" LD="$(HOSTLD)" 	       \
 		    EXTRA_CFLAGS='-g -O0'				       \
 		    OUTPUT=$(HOST_BUILD_DIR)/bpftool/			       \
 		    LIBBPF_OUTPUT=$(HOST_BUILD_DIR)/libbpf/		       \
@@ -280,7 +280,8 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
 		| $(HOST_BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR)                             \
 		    EXTRA_CFLAGS='-g -O0' ARCH= CROSS_COMPILE=		       \
-		    OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
+		    OUTPUT=$(HOST_BUILD_DIR)/libbpf/			       \
+		    CC="$(HOSTCC)" LD="$(HOSTLD)"			       \
 		    DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers
 endif
 
@@ -301,7 +302,7 @@ $(RESOLVE_BTFIDS): $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/resolve_btfids	\
 		       $(TOOLSDIR)/lib/ctype.c			\
 		       $(TOOLSDIR)/lib/str_error_r.c
 	$(Q)$(MAKE) $(submake_extras) -C $(TOOLSDIR)/bpf/resolve_btfids	\
-		CC=$(HOSTCC) LD=$(HOSTLD) AR=$(HOSTAR) \
+		CC="$(HOSTCC)" LD="$(HOSTLD)" AR="$(HOSTAR)" \
 		LIBBPF_INCLUDE=$(HOST_INCLUDE_DIR) \
 		OUTPUT=$(HOST_BUILD_DIR)/resolve_btfids/ BPFOBJ=$(HOST_BPFOBJ)
 
-- 
2.39.1


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

* [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 1/9] selftests/bpf: Quote host tools Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:00   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 3/9] selftests/bpf: Split SAN_CFLAGS and SAN_LDFLAGS Ilya Leoshkevich
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

This makes it possible to add sanitizer flags.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/bpf/runqslower/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index 8b3d87b82b7a..4ff4eed6c01d 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -13,6 +13,12 @@ BPF_DESTDIR := $(BPFOBJ_OUTPUT)
 BPF_INCLUDE := $(BPF_DESTDIR)/include
 INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../include/uapi)
 CFLAGS := -g -Wall $(CLANG_CROSS_FLAGS)
+ifneq ($(EXTRA_CFLAGS),)
+CFLAGS += $(EXTRA_CFLAGS)
+endif
+ifneq ($(EXTRA_LDFLAGS),)
+LDFLAGS += $(EXTRA_LDFLAGS)
+endif
 
 # Try to detect best kernel BTF source
 KERNEL_REL := $(shell uname -r)
-- 
2.39.1


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

* [PATCH bpf-next 3/9] selftests/bpf: Split SAN_CFLAGS and SAN_LDFLAGS
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 1/9] selftests/bpf: Quote host tools Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf Ilya Leoshkevich
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

Memory Sanitizer requires passing different options to CFLAGS and
LDFLAGS: besides the mandatory -fsanitize=memory, one needs to pass
header and library paths, and passing -L to a compilation step
triggers -Wunused-command-line-argument. So introduce a separate
variable for linker flags. Use $(SAN_CFLAGS) as a default in order to
avoid complicating the ASan usage.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 49bc1ba12d3a..9b5786ac676e 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -22,10 +22,11 @@ endif
 
 BPF_GCC		?= $(shell command -v bpf-gcc;)
 SAN_CFLAGS	?=
+SAN_LDFLAGS	?= $(SAN_CFLAGS)
 CFLAGS += -g -O0 -rdynamic -Wall -Werror $(GENFLAGS) $(SAN_CFLAGS)	\
 	  -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)		\
 	  -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)
-LDFLAGS += $(SAN_CFLAGS)
+LDFLAGS += $(SAN_LDFLAGS)
 LDLIBS += -lelf -lz -lrt -lpthread
 
 # Silence some warnings when compiled with clang
-- 
2.39.1


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

* [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (2 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 3/9] selftests/bpf: Split SAN_CFLAGS and SAN_LDFLAGS Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:03   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach Ilya Leoshkevich
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

To get useful results from the Memory Sanitizer, all code running in a
process needs to be instrumented. When building tests with other
sanitizers, it's not strictly necessary, but is also helpful.
So make sure runqslower and libbpf are compiled with SAN_CFLAGS and
linked with SAN_LDFLAGS.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 9b5786ac676e..c4b5c44cdee2 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -215,7 +215,9 @@ $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
 		    OUTPUT=$(RUNQSLOWER_OUTPUT) VMLINUX_BTF=$(VMLINUX_BTF)     \
 		    BPFTOOL_OUTPUT=$(HOST_BUILD_DIR)/bpftool/		       \
 		    BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf			       \
-		    BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR) &&	       \
+		    BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR)		       \
+		    EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)'			       \
+		    EXTRA_LDFLAGS='$(SAN_LDFLAGS)' &&			       \
 		    cp $(RUNQSLOWER_OUTPUT)runqslower $@
 
 TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
@@ -272,7 +274,8 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
 	   $(APIDIR)/linux/bpf.h					       \
 	   | $(BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
-		    EXTRA_CFLAGS='-g -O0'				       \
+		    EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)'			       \
+		    EXTRA_LDFLAGS='$(SAN_LDFLAGS)'			       \
 		    DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
 
 ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
-- 
2.39.1


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

* [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (3 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:05   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe Ilya Leoshkevich
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

malloc() and free() may be completely replaced by sanitizers, use
fopen() and fclose() instead.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 .../selftests/bpf/prog_tests/uprobe_autoattach.c     | 12 ++++++------
 .../selftests/bpf/progs/test_uprobe_autoattach.c     | 10 +++++-----
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
index 82807def0d24..b862948f95a8 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
@@ -16,10 +16,10 @@ static noinline int autoattach_trigger_func(int arg1, int arg2, int arg3,
 
 void test_uprobe_autoattach(void)
 {
+	const char *devnull_str = "/dev/null";
 	struct test_uprobe_autoattach *skel;
 	int trigger_ret;
-	size_t malloc_sz = 1;
-	char *mem;
+	FILE *devnull;
 
 	skel = test_uprobe_autoattach__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
@@ -36,16 +36,16 @@ void test_uprobe_autoattach(void)
 	skel->bss->test_pid = getpid();
 
 	/* trigger & validate shared library u[ret]probes attached by name */
-	mem = malloc(malloc_sz);
+	devnull = fopen(devnull_str, "r");
 
 	ASSERT_EQ(skel->bss->uprobe_byname_parm1, 1, "check_uprobe_byname_parm1");
 	ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran");
 	ASSERT_EQ(skel->bss->uretprobe_byname_rc, trigger_ret, "check_uretprobe_byname_rc");
 	ASSERT_EQ(skel->bss->uretprobe_byname_ret, trigger_ret, "check_uretprobe_byname_ret");
 	ASSERT_EQ(skel->bss->uretprobe_byname_ran, 2, "check_uretprobe_byname_ran");
-	ASSERT_EQ(skel->bss->uprobe_byname2_parm1, malloc_sz, "check_uprobe_byname2_parm1");
+	ASSERT_EQ(skel->bss->uprobe_byname2_parm1, devnull_str, "check_uprobe_byname2_parm1");
 	ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran");
-	ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc");
+	ASSERT_EQ(skel->bss->uretprobe_byname2_rc, (void *)devnull, "check_uretprobe_byname2_rc");
 	ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran");
 
 	ASSERT_EQ(skel->bss->a[0], 1, "arg1");
@@ -67,7 +67,7 @@ void test_uprobe_autoattach(void)
 	ASSERT_EQ(skel->bss->a[7], 8, "arg8");
 #endif
 
-	free(mem);
+	fclose(devnull);
 cleanup:
 	test_uprobe_autoattach__destroy(skel);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
index 774ddeb45898..72f5e7a82c58 100644
--- a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
+++ b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
@@ -13,9 +13,9 @@ int uprobe_byname_ran = 0;
 int uretprobe_byname_rc = 0;
 int uretprobe_byname_ret = 0;
 int uretprobe_byname_ran = 0;
-size_t uprobe_byname2_parm1 = 0;
+void *uprobe_byname2_parm1 = NULL;
 int uprobe_byname2_ran = 0;
-char *uretprobe_byname2_rc = NULL;
+void *uretprobe_byname2_rc = NULL;
 int uretprobe_byname2_ran = 0;
 
 int test_pid;
@@ -88,7 +88,7 @@ int BPF_URETPROBE(handle_uretprobe_byname, int ret)
 }
 
 
-SEC("uprobe/libc.so.6:malloc")
+SEC("uprobe/libc.so.6:fopen")
 int handle_uprobe_byname2(struct pt_regs *ctx)
 {
 	int pid = bpf_get_current_pid_tgid() >> 32;
@@ -96,12 +96,12 @@ int handle_uprobe_byname2(struct pt_regs *ctx)
 	/* ignore irrelevant invocations */
 	if (test_pid != pid)
 		return 0;
-	uprobe_byname2_parm1 = PT_REGS_PARM1_CORE(ctx);
+	uprobe_byname2_parm1 = (void *)(long)PT_REGS_PARM1_CORE(ctx);
 	uprobe_byname2_ran = 3;
 	return 0;
 }
 
-SEC("uretprobe/libc.so.6:malloc")
+SEC("uretprobe/libc.so.6:fopen")
 int handle_uretprobe_byname2(struct pt_regs *ctx)
 {
 	int pid = bpf_get_current_pid_tgid() >> 32;
-- 
2.39.1


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

* [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (4 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:06   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg() Ilya Leoshkevich
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

malloc() and free() may be completely replaced by sanitizers, use
fopen() and fclose() instead.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/attach_probe.c | 10 +++++-----
 tools/testing/selftests/bpf/progs/test_attach_probe.c |  8 +++++---
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 9566d9d2f6ee..56374c8b5436 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -33,8 +33,8 @@ void test_attach_probe(void)
 	struct test_attach_probe* skel;
 	ssize_t uprobe_offset, ref_ctr_offset;
 	struct bpf_link *uprobe_err_link;
+	FILE *devnull;
 	bool legacy;
-	char *mem;
 
 	/* Check if new-style kprobe/uprobe API is supported.
 	 * Kernels that support new FD-based kprobe and uprobe BPF attachment
@@ -147,7 +147,7 @@ void test_attach_probe(void)
 	/* test attach by name for a library function, using the library
 	 * as the binary argument. libc.so.6 will be resolved via dlopen()/dlinfo().
 	 */
-	uprobe_opts.func_name = "malloc";
+	uprobe_opts.func_name = "fopen";
 	uprobe_opts.retprobe = false;
 	skel->links.handle_uprobe_byname2 =
 			bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname2,
@@ -157,7 +157,7 @@ void test_attach_probe(void)
 	if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byname2, "attach_uprobe_byname2"))
 		goto cleanup;
 
-	uprobe_opts.func_name = "free";
+	uprobe_opts.func_name = "fclose";
 	uprobe_opts.retprobe = true;
 	skel->links.handle_uretprobe_byname2 =
 			bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe_byname2,
@@ -195,8 +195,8 @@ void test_attach_probe(void)
 	usleep(1);
 
 	/* trigger & validate shared library u[ret]probes attached by name */
-	mem = malloc(1);
-	free(mem);
+	devnull = fopen("/dev/null", "r");
+	fclose(devnull);
 
 	/* trigger & validate uprobe & uretprobe */
 	trigger_func();
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index a1e45fec8938..269a184c265c 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -94,10 +94,12 @@ int handle_uretprobe_byname(struct pt_regs *ctx)
 SEC("uprobe")
 int handle_uprobe_byname2(struct pt_regs *ctx)
 {
-	unsigned int size = PT_REGS_PARM1(ctx);
+	void *mode_ptr = (void *)(long)PT_REGS_PARM2(ctx);
+	char mode[2] = {};
 
-	/* verify malloc size */
-	if (size == 1)
+	/* verify fopen mode */
+	bpf_probe_read_user(mode, sizeof(mode), mode_ptr);
+	if (mode[0] == 'r' && mode[1] == 0)
 		uprobe_byname2_res = 7;
 	return 0;
 }
-- 
2.39.1


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

* [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg()
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (5 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:14   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 8/9] libbpf: Add MSan annotations Ilya Leoshkevich
  2023-02-08 20:56 ` [PATCH bpf-next 9/9] selftests/bpf: " Ilya Leoshkevich
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

The code assumes that everything that comes after nlmsgerr are nlattrs.
When calculating their size, it does not account for the initial
nlmsghdr. This may lead to accessing uninitialized memory.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/nlattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/nlattr.c b/tools/lib/bpf/nlattr.c
index 3900d052ed19..c5da7662bb04 100644
--- a/tools/lib/bpf/nlattr.c
+++ b/tools/lib/bpf/nlattr.c
@@ -178,7 +178,7 @@ int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh)
 		hlen += nlmsg_len(&err->msg);
 
 	attr = (struct nlattr *) ((void *) err + hlen);
-	alen = nlh->nlmsg_len - hlen;
+	alen = (char *)nlh + nlh->nlmsg_len - (char *)attr;
 
 	if (libbpf_nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen,
 			     extack_policy) != 0) {
-- 
2.39.1


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

* [PATCH bpf-next 8/9] libbpf: Add MSan annotations
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (6 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg() Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:29   ` Andrii Nakryiko
  2023-02-08 20:56 ` [PATCH bpf-next 9/9] selftests/bpf: " Ilya Leoshkevich
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

MSan runs into a few false positives in libbpf. They all come from the
fact that MSan does not know anything about the bpf syscall,
particularly, what it writes to.

Add libbpf_mark_defined() function to mark memory modified by the bpf
syscall. Use the abstract name (it could be e.g.
libbpf_msan_unpoison()), because it can be used for valgrind in the
future as well.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/bpf.c             | 70 +++++++++++++++++++++++++++++++--
 tools/lib/bpf/btf.c             |  1 +
 tools/lib/bpf/libbpf.c          |  1 +
 tools/lib/bpf/libbpf_internal.h | 14 +++++++
 4 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 9aff98f42a3d..12fdc4ce1780 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -92,6 +92,10 @@ int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts)
 		fd = sys_bpf_fd(BPF_PROG_LOAD, attr, size);
 	} while (fd < 0 && errno == EAGAIN && --attempts > 0);
 
+	if (attr->log_buf)
+		libbpf_mark_defined((void *)(unsigned long)attr->log_buf,
+				    attr->log_size);
+
 	return fd;
 }
 
@@ -395,6 +399,33 @@ int bpf_map_update_elem(int fd, const void *key, const void *value,
 	return libbpf_err_errno(ret);
 }
 
+static void mark_map_value_defined(int fd, void *value)
+{
+#ifdef HAVE_LIBBPF_MARK_DEFINED
+	struct bpf_map_info info;
+	size_t size = 0;
+	__u32 info_len;
+	int num_cpus;
+	int err;
+
+	info_len = sizeof(info);
+	err = bpf_obj_get_info_by_fd(fd, &info, &info_len);
+	if (!err) {
+		size = info.value_size;
+		if (info.type == BPF_MAP_TYPE_PERCPU_ARRAY ||
+		    info.type == BPF_MAP_TYPE_PERCPU_HASH ||
+		    info.type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
+		    info.type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
+			num_cpus = libbpf_num_possible_cpus();
+			if (num_cpus > 0)
+				size *= num_cpus;
+		}
+	}
+	if (size)
+		libbpf_mark_defined(value, size);
+#endif
+}
+
 int bpf_map_lookup_elem(int fd, const void *key, void *value)
 {
 	const size_t attr_sz = offsetofend(union bpf_attr, flags);
@@ -407,6 +438,8 @@ int bpf_map_lookup_elem(int fd, const void *key, void *value)
 	attr.value = ptr_to_u64(value);
 
 	ret = sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, attr_sz);
+	if (!ret)
+		mark_map_value_defined(fd, value);
 	return libbpf_err_errno(ret);
 }
 
@@ -423,6 +456,8 @@ int bpf_map_lookup_elem_flags(int fd, const void *key, void *value, __u64 flags)
 	attr.flags = flags;
 
 	ret = sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, attr_sz);
+	if (!ret)
+		mark_map_value_defined(fd, value);
 	return libbpf_err_errno(ret);
 }
 
@@ -438,6 +473,8 @@ int bpf_map_lookup_and_delete_elem(int fd, const void *key, void *value)
 	attr.value = ptr_to_u64(value);
 
 	ret = sys_bpf(BPF_MAP_LOOKUP_AND_DELETE_ELEM, &attr, attr_sz);
+	if (!ret)
+		mark_map_value_defined(fd, value);
 	return libbpf_err_errno(ret);
 }
 
@@ -454,6 +491,8 @@ int bpf_map_lookup_and_delete_elem_flags(int fd, const void *key, void *value, _
 	attr.flags = flags;
 
 	ret = sys_bpf(BPF_MAP_LOOKUP_AND_DELETE_ELEM, &attr, attr_sz);
+	if (!ret)
+		mark_map_value_defined(fd, value);
 	return libbpf_err_errno(ret);
 }
 
@@ -823,10 +862,12 @@ int bpf_prog_query_opts(int target_fd,
 {
 	const size_t attr_sz = offsetofend(union bpf_attr, query);
 	union bpf_attr attr;
+	__u32 *prog_ids;
 	int ret;
 
 	if (!OPTS_VALID(opts, bpf_prog_query_opts))
 		return libbpf_err(-EINVAL);
+	prog_ids = OPTS_GET(opts, prog_ids, NULL);
 
 	memset(&attr, 0, attr_sz);
 
@@ -834,11 +875,15 @@ int bpf_prog_query_opts(int target_fd,
 	attr.query.attach_type	= type;
 	attr.query.query_flags	= OPTS_GET(opts, query_flags, 0);
 	attr.query.prog_cnt	= OPTS_GET(opts, prog_cnt, 0);
-	attr.query.prog_ids	= ptr_to_u64(OPTS_GET(opts, prog_ids, NULL));
+	attr.query.prog_ids	= ptr_to_u64(prog_ids);
 	attr.query.prog_attach_flags = ptr_to_u64(OPTS_GET(opts, prog_attach_flags, NULL));
 
 	ret = sys_bpf(BPF_PROG_QUERY, &attr, attr_sz);
 
+	if (!ret && prog_ids)
+		libbpf_mark_defined(prog_ids,
+				    attr.query.prog_cnt * sizeof(*prog_ids));
+
 	OPTS_SET(opts, attach_flags, attr.query.attach_flags);
 	OPTS_SET(opts, prog_cnt, attr.query.prog_cnt);
 
@@ -868,10 +913,14 @@ int bpf_prog_test_run_opts(int prog_fd, struct bpf_test_run_opts *opts)
 {
 	const size_t attr_sz = offsetofend(union bpf_attr, test);
 	union bpf_attr attr;
+	void *data_out;
+	void *ctx_out;
 	int ret;
 
 	if (!OPTS_VALID(opts, bpf_test_run_opts))
 		return libbpf_err(-EINVAL);
+	data_out = OPTS_GET(opts, data_out, NULL);
+	ctx_out = OPTS_GET(opts, ctx_out, NULL);
 
 	memset(&attr, 0, attr_sz);
 	attr.test.prog_fd = prog_fd;
@@ -885,12 +934,19 @@ int bpf_prog_test_run_opts(int prog_fd, struct bpf_test_run_opts *opts)
 	attr.test.data_size_in = OPTS_GET(opts, data_size_in, 0);
 	attr.test.data_size_out = OPTS_GET(opts, data_size_out, 0);
 	attr.test.ctx_in = ptr_to_u64(OPTS_GET(opts, ctx_in, NULL));
-	attr.test.ctx_out = ptr_to_u64(OPTS_GET(opts, ctx_out, NULL));
+	attr.test.ctx_out = ptr_to_u64(ctx_out);
 	attr.test.data_in = ptr_to_u64(OPTS_GET(opts, data_in, NULL));
-	attr.test.data_out = ptr_to_u64(OPTS_GET(opts, data_out, NULL));
+	attr.test.data_out = ptr_to_u64(data_out);
 
 	ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, attr_sz);
 
+	if (!ret) {
+		if (data_out)
+			libbpf_mark_defined(data_out, attr.test.data_size_out);
+		if (ctx_out)
+			libbpf_mark_defined(ctx_out, attr.test.ctx_size_out);
+	}
+
 	OPTS_SET(opts, data_size_out, attr.test.data_size_out);
 	OPTS_SET(opts, ctx_size_out, attr.test.ctx_size_out);
 	OPTS_SET(opts, duration, attr.test.duration);
@@ -1039,8 +1095,10 @@ int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
 	attr.info.info = ptr_to_u64(info);
 
 	err = sys_bpf(BPF_OBJ_GET_INFO_BY_FD, &attr, attr_sz);
-	if (!err)
+	if (!err) {
 		*info_len = attr.info.info_len;
+		libbpf_mark_defined(info, attr.info.info_len);
+	}
 	return libbpf_err_errno(err);
 }
 
@@ -1103,6 +1161,8 @@ int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_loa
 		attr.btf_log_level = 1;
 		fd = sys_bpf_fd(BPF_BTF_LOAD, &attr, attr_sz);
 	}
+	if (log_buf)
+		libbpf_mark_defined(log_buf, attr.btf_log_size);
 	return libbpf_err_errno(fd);
 }
 
@@ -1122,6 +1182,8 @@ int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
 	attr.task_fd_query.buf_len = *buf_len;
 
 	err = sys_bpf(BPF_TASK_FD_QUERY, &attr, attr_sz);
+	if (!err && buf)
+		libbpf_mark_defined(buf, attr.task_fd_query.buf_len + 1);
 
 	*buf_len = attr.task_fd_query.buf_len;
 	*prog_id = attr.task_fd_query.prog_id;
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 64841117fbb2..24a957604a97 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1388,6 +1388,7 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
 		goto exit_free;
 	}
 
+	libbpf_mark_defined(ptr, btf_info.btf_size);
 	btf = btf_new(ptr, btf_info.btf_size, base_btf);
 
 exit_free:
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4191a78b2815..3e32ae5f0cce 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5443,6 +5443,7 @@ static int load_module_btfs(struct bpf_object *obj)
 			pr_warn("failed to get BTF object #%d info: %d\n", id, err);
 			goto err_out;
 		}
+		libbpf_mark_defined(name, info.name_len + 1);
 
 		/* ignore non-module BTFs */
 		if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) {
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index fbaf68335394..4e4622f66fdf 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -577,4 +577,18 @@ static inline bool is_pow_of_2(size_t x)
 #define PROG_LOAD_ATTEMPTS 5
 int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts);
 
+#if defined(__has_feature)
+#if __has_feature(memory_sanitizer)
+#define LIBBPF_MSAN
+#endif
+#endif
+
+#ifdef LIBBPF_MSAN
+#define HAVE_LIBBPF_MARK_DEFINED
+#include <sanitizer/msan_interface.h>
+#define libbpf_mark_defined __msan_unpoison
+#else
+static inline void libbpf_mark_defined(void *s, size_t n) {}
+#endif
+
 #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
-- 
2.39.1


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

* [PATCH bpf-next 9/9] selftests/bpf: Add MSan annotations
  2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
                   ` (7 preceding siblings ...)
  2023-02-08 20:56 ` [PATCH bpf-next 8/9] libbpf: Add MSan annotations Ilya Leoshkevich
@ 2023-02-08 20:56 ` Ilya Leoshkevich
  2023-02-09  1:34   ` Andrii Nakryiko
  8 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-08 20:56 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Ilya Leoshkevich

eBPF selftests produce a few false positives with MSan. These can be
divided in two classes:

- Sending uninitalized data via a socket.
- bpf_obj_get_info_by_fd() calls.

The first class is trivial; the second should ideally be handled by
libbpf, but it doesn't look possible at the moment, since we don't
know the type of the eBPF object referred to by fd, and therefore the
structure of the output data.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/cap_helpers.c             |  3 +++
 tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c   | 10 ++++++++++
 tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c   |  3 +++
 tools/testing/selftests/bpf/prog_tests/btf.c          | 11 +++++++++++
 tools/testing/selftests/bpf/prog_tests/send_signal.c  |  2 ++
 .../selftests/bpf/prog_tests/tp_attach_query.c        |  6 ++++++
 tools/testing/selftests/bpf/prog_tests/xdp_bonding.c  |  3 +++
 tools/testing/selftests/bpf/xdp_synproxy.c            |  2 ++
 8 files changed, 40 insertions(+)

diff --git a/tools/testing/selftests/bpf/cap_helpers.c b/tools/testing/selftests/bpf/cap_helpers.c
index d5ac507401d7..f5775b342b30 100644
--- a/tools/testing/selftests/bpf/cap_helpers.c
+++ b/tools/testing/selftests/bpf/cap_helpers.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
+#include <bpf/libbpf_internal.h>
 #include "cap_helpers.h"
 
 /* Avoid including <sys/capability.h> from the libcap-devel package,
@@ -20,6 +21,7 @@ int cap_enable_effective(__u64 caps, __u64 *old_caps)
 	err = capget(&hdr, data);
 	if (err)
 		return err;
+	libbpf_mark_defined(data, sizeof(data));
 
 	if (old_caps)
 		*old_caps = (__u64)(data[1].effective) << 32 | data[0].effective;
@@ -50,6 +52,7 @@ int cap_disable_effective(__u64 caps, __u64 *old_caps)
 	err = capget(&hdr, data);
 	if (err)
 		return err;
+	libbpf_mark_defined(data, sizeof(data));
 
 	if (old_caps)
 		*old_caps = (__u64)(data[1].effective) << 32 | data[0].effective;
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index e1c1e521cca2..7253d5dc4bb2 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <bpf/libbpf_internal.h>
 
 #define nr_iters 2
 
@@ -31,6 +32,7 @@ void serial_test_bpf_obj_id(void)
 	__u64 array_value;
 	uid_t my_uid = getuid();
 	time_t now, load_time;
+	int tp_name_len;
 
 	err = bpf_prog_get_fd_by_id(0);
 	CHECK(err >= 0 || errno != ENOENT,
@@ -122,6 +124,10 @@ void serial_test_bpf_obj_id(void)
 					     &info_len);
 		load_time = (real_time_ts.tv_sec - boot_time_ts.tv_sec)
 			+ (prog_infos[i].load_time / nsec_per_sec);
+		if (!err)
+			libbpf_mark_defined(&map_ids[i],
+					    prog_infos[i].nr_map_ids *
+						sizeof(map_ids[0]));
 		if (CHECK(err ||
 			  prog_infos[i].type != BPF_PROG_TYPE_RAW_TRACEPOINT ||
 			  info_len != sizeof(struct bpf_prog_info) ||
@@ -163,6 +169,10 @@ void serial_test_bpf_obj_id(void)
 		link_infos[i].raw_tracepoint.tp_name_len = sizeof(tp_name);
 		err = bpf_obj_get_info_by_fd(bpf_link__fd(links[i]),
 					     &link_infos[i], &info_len);
+		if (!err) {
+			tp_name_len = link_infos[i].raw_tracepoint.tp_name_len;
+			libbpf_mark_defined(tp_name, tp_name_len + 1);
+		}
 		if (CHECK(err ||
 			  link_infos[i].type != BPF_LINK_TYPE_RAW_TRACEPOINT ||
 			  link_infos[i].prog_id != prog_infos[i].id ||
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index e980188d4124..11f02f68e152 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
@@ -4,6 +4,7 @@
 #include <linux/err.h>
 #include <netinet/tcp.h>
 #include <test_progs.h>
+#include <bpf/libbpf_internal.h>
 #include "network_helpers.h"
 #include "bpf_dctcp.skel.h"
 #include "bpf_cubic.skel.h"
@@ -39,6 +40,8 @@ static void *server(void *arg)
 	ssize_t nr_sent = 0, bytes = 0;
 	char batch[1500];
 
+	libbpf_mark_defined(batch, sizeof(batch));
+
 	fd = accept(lfd, NULL, NULL);
 	while (fd == -1) {
 		if (errno == EINTR)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf.c b/tools/testing/selftests/bpf/prog_tests/btf.c
index de1b5b9eb93a..ff6950404c02 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf.c
@@ -20,6 +20,7 @@
 #include <assert.h>
 #include <bpf/libbpf.h>
 #include <bpf/btf.h>
+#include <bpf/libbpf_internal.h>
 
 #include "bpf_util.h"
 #include "../test_btf.h"
@@ -4500,6 +4501,8 @@ static int test_btf_id(unsigned int test_num)
 	/* Test BPF_OBJ_GET_INFO_BY_ID on btf_id */
 	info_len = sizeof(info[0]);
 	err = bpf_obj_get_info_by_fd(btf_fd[0], &info[0], &info_len);
+	if (!err)
+		libbpf_mark_defined(user_btf[0], info[0].btf_size);
 	if (CHECK(err, "errno:%d", errno)) {
 		err = -1;
 		goto done;
@@ -4513,6 +4516,8 @@ static int test_btf_id(unsigned int test_num)
 
 	ret = 0;
 	err = bpf_obj_get_info_by_fd(btf_fd[1], &info[1], &info_len);
+	if (!err)
+		libbpf_mark_defined(user_btf[1], info[1].btf_size);
 	if (CHECK(err || info[0].id != info[1].id ||
 		  info[0].btf_size != info[1].btf_size ||
 		  (ret = memcmp(user_btf[0], user_btf[1], info[0].btf_size)),
@@ -4639,6 +4644,8 @@ static void do_test_get_info(unsigned int test_num)
 
 	ret = 0;
 	err = bpf_obj_get_info_by_fd(btf_fd, &info, &info_len);
+	if (!err)
+		libbpf_mark_defined(user_btf, info.btf_size);
 	if (CHECK(err || !info.id || info_len != sizeof(info) ||
 		  info.btf_size != raw_btf_size ||
 		  (ret = memcmp(raw_btf, user_btf, expected_nbytes)),
@@ -4788,6 +4795,8 @@ static void do_test_file(unsigned int test_num)
 	info.func_info = ptr_to_u64(func_info);
 
 	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+	if (!err)
+		libbpf_mark_defined(func_info, info.nr_func_info * rec_size);
 
 	if (CHECK(err < 0, "invalid get info (2nd) errno:%d", errno)) {
 		fprintf(stderr, "%s\n", btf_log_buf);
@@ -6436,6 +6445,8 @@ static int test_get_finfo(const struct prog_info_raw_test *test,
 	info.func_info_rec_size = rec_size;
 	info.func_info = ptr_to_u64(func_info);
 	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+	if (!err)
+		libbpf_mark_defined(func_info, info.nr_func_info * rec_size);
 	if (CHECK(err < 0, "invalid get info (2nd) errno:%d", errno)) {
 		fprintf(stderr, "%s\n", btf_log_buf);
 		err = -1;
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index d63a20fbed33..94f42b48e45d 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <bpf/libbpf_internal.h>
 #include <sys/time.h>
 #include <sys/resource.h>
 #include "test_send_signal_kern.skel.h"
@@ -58,6 +59,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 		ASSERT_OK(setpriority(PRIO_PROCESS, 0, -20), "setpriority");
 
 		/* notify parent signal handler is installed */
+		libbpf_mark_defined(buf, 1);
 		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
 
 		/* make sure parent enabled bpf program to send_signal */
diff --git a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
index a479080533db..259bd8102907 100644
--- a/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
+++ b/tools/testing/selftests/bpf/prog_tests/tp_attach_query.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include <bpf/libbpf_internal.h>
 
 void serial_test_tp_attach_query(void)
 {
@@ -65,6 +66,7 @@ void serial_test_tp_attach_query(void)
 		if (i == 0) {
 			/* check NULL prog array query */
 			query->ids_len = num_progs;
+			query->prog_cnt = 0;
 			err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, query);
 			if (CHECK(err || query->prog_cnt != 0,
 				  "perf_event_ioc_query_bpf",
@@ -109,6 +111,10 @@ void serial_test_tp_attach_query(void)
 
 		query->ids_len = num_progs;
 		err = ioctl(pmu_fd[i], PERF_EVENT_IOC_QUERY_BPF, query);
+		if (!err)
+			libbpf_mark_defined(query->ids,
+					    query->prog_cnt *
+						sizeof(query->ids[0]));
 		if (CHECK(err || query->prog_cnt != (i + 1),
 			  "perf_event_ioc_query_bpf",
 			  "err %d errno %d query->prog_cnt %u\n",
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
index 5e3a26b15ec6..2620c66533b9 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_bonding.c
@@ -14,6 +14,7 @@
 #include <net/if.h>
 #include <linux/if_link.h>
 #include "test_progs.h"
+#include "bpf/libbpf_internal.h"
 #include "network_helpers.h"
 #include <linux/if_bonding.h>
 #include <linux/limits.h>
@@ -224,6 +225,8 @@ static int send_udp_packets(int vary_dst_ip)
 	int i, s = -1;
 	int ifindex;
 
+	libbpf_mark_defined(buf, sizeof(buf));
+
 	s = socket(AF_PACKET, SOCK_RAW, IPPROTO_RAW);
 	if (!ASSERT_GE(s, 0, "socket"))
 		goto err;
diff --git a/tools/testing/selftests/bpf/xdp_synproxy.c b/tools/testing/selftests/bpf/xdp_synproxy.c
index 6dbe0b745198..7667393bc7b5 100644
--- a/tools/testing/selftests/bpf/xdp_synproxy.c
+++ b/tools/testing/selftests/bpf/xdp_synproxy.c
@@ -12,6 +12,7 @@
 #include <sys/types.h>
 #include <bpf/bpf.h>
 #include <bpf/libbpf.h>
+#include <bpf/libbpf_internal.h>
 #include <net/if.h>
 #include <linux/if_link.h>
 #include <linux/limits.h>
@@ -297,6 +298,7 @@ static int syncookie_open_bpf_maps(__u32 prog_id, int *values_map_fd, int *ports
 		fprintf(stderr, "Error: bpf_obj_get_info_by_fd: %s\n", strerror(-err));
 		goto out;
 	}
+	libbpf_mark_defined(map_ids, prog_info.nr_map_ids * sizeof(map_ids[0]));
 
 	if (prog_info.nr_map_ids < 2) {
 		fprintf(stderr, "Error: Found %u BPF maps, expected at least 2\n",
-- 
2.39.1


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

* Re: [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support
  2023-02-08 20:56 ` [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support Ilya Leoshkevich
@ 2023-02-09  1:00   ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:00 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> This makes it possible to add sanitizer flags.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/bpf/runqslower/Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
> index 8b3d87b82b7a..4ff4eed6c01d 100644
> --- a/tools/bpf/runqslower/Makefile
> +++ b/tools/bpf/runqslower/Makefile
> @@ -13,6 +13,12 @@ BPF_DESTDIR := $(BPFOBJ_OUTPUT)
>  BPF_INCLUDE := $(BPF_DESTDIR)/include
>  INCLUDES := -I$(OUTPUT) -I$(BPF_INCLUDE) -I$(abspath ../../include/uapi)
>  CFLAGS := -g -Wall $(CLANG_CROSS_FLAGS)
> +ifneq ($(EXTRA_CFLAGS),)
> +CFLAGS += $(EXTRA_CFLAGS)
> +endif
> +ifneq ($(EXTRA_LDFLAGS),)
> +LDFLAGS += $(EXTRA_LDFLAGS)
> +endif
>

can't we do this unconditionally? If those variables are not defined,
Makefile will substitute empty string, no?

>  # Try to detect best kernel BTF source
>  KERNEL_REL := $(shell uname -r)
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf
  2023-02-08 20:56 ` [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf Ilya Leoshkevich
@ 2023-02-09  1:03   ` Andrii Nakryiko
  2023-02-09  9:55     ` Ilya Leoshkevich
  0 siblings, 1 reply; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:03 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> To get useful results from the Memory Sanitizer, all code running in a
> process needs to be instrumented. When building tests with other
> sanitizers, it's not strictly necessary, but is also helpful.
> So make sure runqslower and libbpf are compiled with SAN_CFLAGS and
> linked with SAN_LDFLAGS.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 9b5786ac676e..c4b5c44cdee2 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -215,7 +215,9 @@ $(OUTPUT)/runqslower: $(BPFOBJ) | $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
>                     OUTPUT=$(RUNQSLOWER_OUTPUT) VMLINUX_BTF=$(VMLINUX_BTF)     \
>                     BPFTOOL_OUTPUT=$(HOST_BUILD_DIR)/bpftool/                  \
>                     BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf                          \
> -                   BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR) &&             \
> +                   BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR)                \
> +                   EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)'                        \
> +                   EXTRA_LDFLAGS='$(SAN_LDFLAGS)' &&                          \
>                     cp $(RUNQSLOWER_OUTPUT)runqslower $@
>

I wouldn't do it for runqslower, we just make sure that it compiles,
we don't really run it at all. No need to complicate its build, IMO.

>  TEST_GEN_PROGS_EXTENDED += $(DEFAULT_BPFTOOL)
> @@ -272,7 +274,8 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                 \
>            $(APIDIR)/linux/bpf.h                                               \
>            | $(BUILD_DIR)/libbpf
>         $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
> -                   EXTRA_CFLAGS='-g -O0'                                      \
> +                   EXTRA_CFLAGS='-g -O0 $(SAN_CFLAGS)'                        \
> +                   EXTRA_LDFLAGS='$(SAN_LDFLAGS)'                             \
>                     DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
>
>  ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach
  2023-02-08 20:56 ` [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach Ilya Leoshkevich
@ 2023-02-09  1:05   ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:05 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> malloc() and free() may be completely replaced by sanitizers, use
> fopen() and fclose() instead.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  .../selftests/bpf/prog_tests/uprobe_autoattach.c     | 12 ++++++------
>  .../selftests/bpf/progs/test_uprobe_autoattach.c     | 10 +++++-----
>  2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> index 82807def0d24..b862948f95a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> @@ -16,10 +16,10 @@ static noinline int autoattach_trigger_func(int arg1, int arg2, int arg3,
>
>  void test_uprobe_autoattach(void)
>  {
> +       const char *devnull_str = "/dev/null";
>         struct test_uprobe_autoattach *skel;
>         int trigger_ret;
> -       size_t malloc_sz = 1;
> -       char *mem;
> +       FILE *devnull;
>
>         skel = test_uprobe_autoattach__open_and_load();
>         if (!ASSERT_OK_PTR(skel, "skel_open"))
> @@ -36,16 +36,16 @@ void test_uprobe_autoattach(void)
>         skel->bss->test_pid = getpid();
>
>         /* trigger & validate shared library u[ret]probes attached by name */
> -       mem = malloc(malloc_sz);
> +       devnull = fopen(devnull_str, "r");
>
>         ASSERT_EQ(skel->bss->uprobe_byname_parm1, 1, "check_uprobe_byname_parm1");
>         ASSERT_EQ(skel->bss->uprobe_byname_ran, 1, "check_uprobe_byname_ran");
>         ASSERT_EQ(skel->bss->uretprobe_byname_rc, trigger_ret, "check_uretprobe_byname_rc");
>         ASSERT_EQ(skel->bss->uretprobe_byname_ret, trigger_ret, "check_uretprobe_byname_ret");
>         ASSERT_EQ(skel->bss->uretprobe_byname_ran, 2, "check_uretprobe_byname_ran");
> -       ASSERT_EQ(skel->bss->uprobe_byname2_parm1, malloc_sz, "check_uprobe_byname2_parm1");
> +       ASSERT_EQ(skel->bss->uprobe_byname2_parm1, devnull_str, "check_uprobe_byname2_parm1");
>         ASSERT_EQ(skel->bss->uprobe_byname2_ran, 3, "check_uprobe_byname2_ran");
> -       ASSERT_EQ(skel->bss->uretprobe_byname2_rc, mem, "check_uretprobe_byname2_rc");
> +       ASSERT_EQ(skel->bss->uretprobe_byname2_rc, (void *)devnull, "check_uretprobe_byname2_rc");
>         ASSERT_EQ(skel->bss->uretprobe_byname2_ran, 4, "check_uretprobe_byname2_ran");
>
>         ASSERT_EQ(skel->bss->a[0], 1, "arg1");
> @@ -67,7 +67,7 @@ void test_uprobe_autoattach(void)
>         ASSERT_EQ(skel->bss->a[7], 8, "arg8");
>  #endif
>
> -       free(mem);
> +       fclose(devnull);
>  cleanup:
>         test_uprobe_autoattach__destroy(skel);
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
> index 774ddeb45898..72f5e7a82c58 100644
> --- a/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
> +++ b/tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
> @@ -13,9 +13,9 @@ int uprobe_byname_ran = 0;
>  int uretprobe_byname_rc = 0;
>  int uretprobe_byname_ret = 0;
>  int uretprobe_byname_ran = 0;
> -size_t uprobe_byname2_parm1 = 0;
> +void *uprobe_byname2_parm1 = NULL;
>  int uprobe_byname2_ran = 0;
> -char *uretprobe_byname2_rc = NULL;
> +void *uretprobe_byname2_rc = NULL;


ugh... shall we use u64 and avoid problems with 32-bit host vs 64-bit
BPF mismatch? Maybe it will never bite us, but why risking?

>  int uretprobe_byname2_ran = 0;
>
>  int test_pid;
> @@ -88,7 +88,7 @@ int BPF_URETPROBE(handle_uretprobe_byname, int ret)
>  }
>
>
> -SEC("uprobe/libc.so.6:malloc")
> +SEC("uprobe/libc.so.6:fopen")
>  int handle_uprobe_byname2(struct pt_regs *ctx)
>  {
>         int pid = bpf_get_current_pid_tgid() >> 32;
> @@ -96,12 +96,12 @@ int handle_uprobe_byname2(struct pt_regs *ctx)
>         /* ignore irrelevant invocations */
>         if (test_pid != pid)
>                 return 0;
> -       uprobe_byname2_parm1 = PT_REGS_PARM1_CORE(ctx);
> +       uprobe_byname2_parm1 = (void *)(long)PT_REGS_PARM1_CORE(ctx);
>         uprobe_byname2_ran = 3;
>         return 0;
>  }
>
> -SEC("uretprobe/libc.so.6:malloc")
> +SEC("uretprobe/libc.so.6:fopen")
>  int handle_uretprobe_byname2(struct pt_regs *ctx)
>  {
>         int pid = bpf_get_current_pid_tgid() >> 32;
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe
  2023-02-08 20:56 ` [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe Ilya Leoshkevich
@ 2023-02-09  1:06   ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:06 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> malloc() and free() may be completely replaced by sanitizers, use
> fopen() and fclose() instead.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/attach_probe.c | 10 +++++-----
>  tools/testing/selftests/bpf/progs/test_attach_probe.c |  8 +++++---
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index 9566d9d2f6ee..56374c8b5436 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -33,8 +33,8 @@ void test_attach_probe(void)
>         struct test_attach_probe* skel;
>         ssize_t uprobe_offset, ref_ctr_offset;
>         struct bpf_link *uprobe_err_link;
> +       FILE *devnull;
>         bool legacy;
> -       char *mem;
>
>         /* Check if new-style kprobe/uprobe API is supported.
>          * Kernels that support new FD-based kprobe and uprobe BPF attachment
> @@ -147,7 +147,7 @@ void test_attach_probe(void)
>         /* test attach by name for a library function, using the library
>          * as the binary argument. libc.so.6 will be resolved via dlopen()/dlinfo().
>          */
> -       uprobe_opts.func_name = "malloc";
> +       uprobe_opts.func_name = "fopen";
>         uprobe_opts.retprobe = false;
>         skel->links.handle_uprobe_byname2 =
>                         bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname2,
> @@ -157,7 +157,7 @@ void test_attach_probe(void)
>         if (!ASSERT_OK_PTR(skel->links.handle_uprobe_byname2, "attach_uprobe_byname2"))
>                 goto cleanup;
>
> -       uprobe_opts.func_name = "free";
> +       uprobe_opts.func_name = "fclose";
>         uprobe_opts.retprobe = true;
>         skel->links.handle_uretprobe_byname2 =
>                         bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe_byname2,
> @@ -195,8 +195,8 @@ void test_attach_probe(void)
>         usleep(1);
>
>         /* trigger & validate shared library u[ret]probes attached by name */
> -       mem = malloc(1);
> -       free(mem);
> +       devnull = fopen("/dev/null", "r");
> +       fclose(devnull);
>
>         /* trigger & validate uprobe & uretprobe */
>         trigger_func();
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index a1e45fec8938..269a184c265c 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -94,10 +94,12 @@ int handle_uretprobe_byname(struct pt_regs *ctx)
>  SEC("uprobe")
>  int handle_uprobe_byname2(struct pt_regs *ctx)
>  {
> -       unsigned int size = PT_REGS_PARM1(ctx);
> +       void *mode_ptr = (void *)(long)PT_REGS_PARM2(ctx);

let's use BPF_UPROBE() macro instead of PT_REGS_xxx() calls?

> +       char mode[2] = {};
>
> -       /* verify malloc size */
> -       if (size == 1)
> +       /* verify fopen mode */
> +       bpf_probe_read_user(mode, sizeof(mode), mode_ptr);
> +       if (mode[0] == 'r' && mode[1] == 0)
>                 uprobe_byname2_res = 7;
>         return 0;
>  }
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg()
  2023-02-08 20:56 ` [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg() Ilya Leoshkevich
@ 2023-02-09  1:14   ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:14 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> The code assumes that everything that comes after nlmsgerr are nlattrs.
> When calculating their size, it does not account for the initial
> nlmsghdr. This may lead to accessing uninitialized memory.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/nlattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/nlattr.c b/tools/lib/bpf/nlattr.c
> index 3900d052ed19..c5da7662bb04 100644
> --- a/tools/lib/bpf/nlattr.c
> +++ b/tools/lib/bpf/nlattr.c
> @@ -178,7 +178,7 @@ int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh)
>                 hlen += nlmsg_len(&err->msg);
>
>         attr = (struct nlattr *) ((void *) err + hlen);
> -       alen = nlh->nlmsg_len - hlen;
> +       alen = (char *)nlh + nlh->nlmsg_len - (char *)attr;

we use (void *) for pointer manipulations, let's be consistent?
Otherwise looks good (I think, this whole nlattr stuff is very cryptic
to me).

>
>         if (libbpf_nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen,
>                              extack_policy) != 0) {
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 8/9] libbpf: Add MSan annotations
  2023-02-08 20:56 ` [PATCH bpf-next 8/9] libbpf: Add MSan annotations Ilya Leoshkevich
@ 2023-02-09  1:29   ` Andrii Nakryiko
  2023-02-09 10:01     ` Ilya Leoshkevich
  0 siblings, 1 reply; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:29 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> MSan runs into a few false positives in libbpf. They all come from the
> fact that MSan does not know anything about the bpf syscall,
> particularly, what it writes to.
>
> Add libbpf_mark_defined() function to mark memory modified by the bpf
> syscall. Use the abstract name (it could be e.g.
> libbpf_msan_unpoison()), because it can be used for valgrind in the
> future as well.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---

This is a lot to satisfy MSan, especially mark_map_value_defined which
has to do bpf_obj_get_info_by_fd()... Is there any other way? What
happens if we don't do it?

As for libbpf_mark_defined, wouldn't it be cleaner to teach it to
handle NULL pointer and/or zero size transparently? Also, we can have
libbpf_mark_defined_if(void *ptr, size_t sz, bool cond), so in code we
don't have to have those explicit if conditions. Instead of:

> +       if (!ret && prog_ids)
> +               libbpf_mark_defined(prog_ids,
> +                                   attr.query.prog_cnt * sizeof(*prog_ids));

we can write just

libbpf_mark_defined_if(prog_ids, attr.query.prog_cnt *
sizeof(*prog_ids), ret == 0);

?

Should we also call a helper something like 'libbpf_mark_mem_written',
because that's what we are saying here, right?

>  tools/lib/bpf/bpf.c             | 70 +++++++++++++++++++++++++++++++--
>  tools/lib/bpf/btf.c             |  1 +
>  tools/lib/bpf/libbpf.c          |  1 +
>  tools/lib/bpf/libbpf_internal.h | 14 +++++++
>  4 files changed, 82 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index fbaf68335394..4e4622f66fdf 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -577,4 +577,18 @@ static inline bool is_pow_of_2(size_t x)
>  #define PROG_LOAD_ATTEMPTS 5
>  int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts);
>
> +#if defined(__has_feature)
> +#if __has_feature(memory_sanitizer)
> +#define LIBBPF_MSAN
> +#endif
> +#endif
> +
> +#ifdef LIBBPF_MSAN

would below work:

#if defined(__has_feature) && __has_feature(memory_sanitizer)

?

> +#define HAVE_LIBBPF_MARK_DEFINED
> +#include <sanitizer/msan_interface.h>
> +#define libbpf_mark_defined __msan_unpoison
> +#else
> +static inline void libbpf_mark_defined(void *s, size_t n) {}
> +#endif
> +
>  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> --
> 2.39.1
>

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

* Re: [PATCH bpf-next 9/9] selftests/bpf: Add MSan annotations
  2023-02-08 20:56 ` [PATCH bpf-next 9/9] selftests/bpf: " Ilya Leoshkevich
@ 2023-02-09  1:34   ` Andrii Nakryiko
  2023-02-09 10:30     ` Ilya Leoshkevich
  0 siblings, 1 reply; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09  1:34 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> eBPF selftests produce a few false positives with MSan. These can be
> divided in two classes:
>
> - Sending uninitalized data via a socket.
> - bpf_obj_get_info_by_fd() calls.
>
> The first class is trivial; the second should ideally be handled by
> libbpf, but it doesn't look possible at the moment, since we don't
> know the type of the eBPF object referred to by fd, and therefore the
> structure of the output data.

yeah, bpf_obj_get_info_by_fd() is quite bad from usability standpoint.
I think we should add bpf_get_{map,prog,link,btf}_info_by_fd()
wrappers and try to use them. That will allow to specify correct
expected struct types, we'll be able to mark initialized memory
properly. We already have bpf_{map,prog,btf,link}_get_fd_by_id()
family, so having similar for getting info seems fitting (even if
underlying bpf() command is generic).

Thoughts?

>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/testing/selftests/bpf/cap_helpers.c             |  3 +++
>  tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c   | 10 ++++++++++
>  tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c   |  3 +++
>  tools/testing/selftests/bpf/prog_tests/btf.c          | 11 +++++++++++
>  tools/testing/selftests/bpf/prog_tests/send_signal.c  |  2 ++
>  .../selftests/bpf/prog_tests/tp_attach_query.c        |  6 ++++++
>  tools/testing/selftests/bpf/prog_tests/xdp_bonding.c  |  3 +++
>  tools/testing/selftests/bpf/xdp_synproxy.c            |  2 ++
>  8 files changed, 40 insertions(+)
>

[...]

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

* Re: [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf
  2023-02-09  1:03   ` Andrii Nakryiko
@ 2023-02-09  9:55     ` Ilya Leoshkevich
  2023-02-09 17:17       ` Andrii Nakryiko
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-09  9:55 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, 2023-02-08 at 17:03 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > To get useful results from the Memory Sanitizer, all code running
> > in a
> > process needs to be instrumented. When building tests with other
> > sanitizers, it's not strictly necessary, but is also helpful.
> > So make sure runqslower and libbpf are compiled with SAN_CFLAGS and
> > linked with SAN_LDFLAGS.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/testing/selftests/bpf/Makefile | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/Makefile
> > b/tools/testing/selftests/bpf/Makefile
> > index 9b5786ac676e..c4b5c44cdee2 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -215,7 +215,9 @@ $(OUTPUT)/runqslower: $(BPFOBJ) |
> > $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
> >                     OUTPUT=$(RUNQSLOWER_OUTPUT)
> > VMLINUX_BTF=$(VMLINUX_BTF)     \
> >                    
> > BPFTOOL_OUTPUT=$(HOST_BUILD_DIR)/bpftool/                  \
> >                    
> > BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf                          \
> > -                   BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR)
> > &&             \
> > +                   BPFOBJ=$(BPFOBJ)
> > BPF_INCLUDE=$(INCLUDE_DIR)                \
> > +                   EXTRA_CFLAGS='-g -O0
> > $(SAN_CFLAGS)'                        \
> > +                   EXTRA_LDFLAGS='$(SAN_LDFLAGS)'
> > &&                          \
> >                     cp $(RUNQSLOWER_OUTPUT)runqslower $@
> > 
> 
> I wouldn't do it for runqslower, we just make sure that it compiles,
> we don't really run it at all. No need to complicate its build, IMO.

runqslower is linked with target libbpf, which is instrumented.
This produces undefined symbol errors, since MSan runtime is expected
to be a part of an executable.

[...]

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

* Re: [PATCH bpf-next 8/9] libbpf: Add MSan annotations
  2023-02-09  1:29   ` Andrii Nakryiko
@ 2023-02-09 10:01     ` Ilya Leoshkevich
  2023-02-09 19:37       ` Andrii Nakryiko
  0 siblings, 1 reply; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-09 10:01 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, 2023-02-08 at 17:29 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > MSan runs into a few false positives in libbpf. They all come from
> > the
> > fact that MSan does not know anything about the bpf syscall,
> > particularly, what it writes to.
> > 
> > Add libbpf_mark_defined() function to mark memory modified by the
> > bpf
> > syscall. Use the abstract name (it could be e.g.
> > libbpf_msan_unpoison()), because it can be used for valgrind in the
> > future as well.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> 
> This is a lot to satisfy MSan, especially mark_map_value_defined
> which
> has to do bpf_obj_get_info_by_fd()... Is there any other way? What
> happens if we don't do it?

It would generate false positives when accessing the resulting map
values, which is not nice. The main complexity in this case comes not
from mark_map_value_defined() per se, but from the fact that we don't
know the value size, especially for percpu maps.

I toyed with the idea to extend the BPF_MAP_LOOKUP_ELEM interface to
return the number of bytes written, but decided against it - the only
user of this would be MSan, and at least for the beginning it's better
to have extra complexity in userspace, rather than in kernel.

> As for libbpf_mark_defined, wouldn't it be cleaner to teach it to
> handle NULL pointer and/or zero size transparently? Also, we can have
> libbpf_mark_defined_if(void *ptr, size_t sz, bool cond), so in code
> we
> don't have to have those explicit if conditions. Instead of:
> 
> > +       if (!ret && prog_ids)
> > +               libbpf_mark_defined(prog_ids,
> > +                                   attr.query.prog_cnt *
> > sizeof(*prog_ids));
> 
> we can write just
> 
> libbpf_mark_defined_if(prog_ids, attr.query.prog_cnt *
> sizeof(*prog_ids), ret == 0);
> 
> ?
> 
> Should we also call a helper something like
> 'libbpf_mark_mem_written',
> because that's what we are saying here, right?

Sure, all this sounds reasonable. Will do.

> 
> >  tools/lib/bpf/bpf.c             | 70
> > +++++++++++++++++++++++++++++++--
> >  tools/lib/bpf/btf.c             |  1 +
> >  tools/lib/bpf/libbpf.c          |  1 +
> >  tools/lib/bpf/libbpf_internal.h | 14 +++++++
> >  4 files changed, 82 insertions(+), 4 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/tools/lib/bpf/libbpf_internal.h
> > b/tools/lib/bpf/libbpf_internal.h
> > index fbaf68335394..4e4622f66fdf 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -577,4 +577,18 @@ static inline bool is_pow_of_2(size_t x)
> >  #define PROG_LOAD_ATTEMPTS 5
> >  int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int
> > attempts);
> > 
> > +#if defined(__has_feature)
> > +#if __has_feature(memory_sanitizer)
> > +#define LIBBPF_MSAN
> > +#endif
> > +#endif
> > +
> > +#ifdef LIBBPF_MSAN
> 
> would below work:
> 
> #if defined(__has_feature) && __has_feature(memory_sanitizer)
> 
> ?
> 
> > +#define HAVE_LIBBPF_MARK_DEFINED
> > +#include <sanitizer/msan_interface.h>
> > +#define libbpf_mark_defined __msan_unpoison
> > +#else
> > +static inline void libbpf_mark_defined(void *s, size_t n) {}
> > +#endif
> > +
> >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > --
> > 2.39.1
> > 


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

* Re: [PATCH bpf-next 9/9] selftests/bpf: Add MSan annotations
  2023-02-09  1:34   ` Andrii Nakryiko
@ 2023-02-09 10:30     ` Ilya Leoshkevich
  0 siblings, 0 replies; 22+ messages in thread
From: Ilya Leoshkevich @ 2023-02-09 10:30 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Wed, 2023-02-08 at 17:34 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > eBPF selftests produce a few false positives with MSan. These can
> > be
> > divided in two classes:
> > 
> > - Sending uninitalized data via a socket.
> > - bpf_obj_get_info_by_fd() calls.
> > 
> > The first class is trivial; the second should ideally be handled by
> > libbpf, but it doesn't look possible at the moment, since we don't
> > know the type of the eBPF object referred to by fd, and therefore
> > the
> > structure of the output data.
> 
> yeah, bpf_obj_get_info_by_fd() is quite bad from usability
> standpoint.
> I think we should add bpf_get_{map,prog,link,btf}_info_by_fd()
> wrappers and try to use them. That will allow to specify correct
> expected struct types, we'll be able to mark initialized memory
> properly. We already have bpf_{map,prog,btf,link}_get_fd_by_id()
> family, so having similar for getting info seems fitting (even if
> underlying bpf() command is generic).
> 
> Thoughts?

Sounds good to me, I will give it a try.

> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/testing/selftests/bpf/cap_helpers.c             |  3 +++
> >  tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c   | 10
> > ++++++++++
> >  tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c   |  3 +++
> >  tools/testing/selftests/bpf/prog_tests/btf.c          | 11
> > +++++++++++
> >  tools/testing/selftests/bpf/prog_tests/send_signal.c  |  2 ++
> >  .../selftests/bpf/prog_tests/tp_attach_query.c        |  6 ++++++
> >  tools/testing/selftests/bpf/prog_tests/xdp_bonding.c  |  3 +++
> >  tools/testing/selftests/bpf/xdp_synproxy.c            |  2 ++
> >  8 files changed, 40 insertions(+)
> > 
> 
> [...]


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

* Re: [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf
  2023-02-09  9:55     ` Ilya Leoshkevich
@ 2023-02-09 17:17       ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09 17:17 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Thu, Feb 9, 2023 at 1:55 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-02-08 at 17:03 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > To get useful results from the Memory Sanitizer, all code running
> > > in a
> > > process needs to be instrumented. When building tests with other
> > > sanitizers, it's not strictly necessary, but is also helpful.
> > > So make sure runqslower and libbpf are compiled with SAN_CFLAGS and
> > > linked with SAN_LDFLAGS.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  tools/testing/selftests/bpf/Makefile | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile
> > > b/tools/testing/selftests/bpf/Makefile
> > > index 9b5786ac676e..c4b5c44cdee2 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -215,7 +215,9 @@ $(OUTPUT)/runqslower: $(BPFOBJ) |
> > > $(DEFAULT_BPFTOOL) $(RUNQSLOWER_OUTPUT)
> > >                     OUTPUT=$(RUNQSLOWER_OUTPUT)
> > > VMLINUX_BTF=$(VMLINUX_BTF)     \
> > >
> > > BPFTOOL_OUTPUT=$(HOST_BUILD_DIR)/bpftool/                  \
> > >
> > > BPFOBJ_OUTPUT=$(BUILD_DIR)/libbpf                          \
> > > -                   BPFOBJ=$(BPFOBJ) BPF_INCLUDE=$(INCLUDE_DIR)
> > > &&             \
> > > +                   BPFOBJ=$(BPFOBJ)
> > > BPF_INCLUDE=$(INCLUDE_DIR)                \
> > > +                   EXTRA_CFLAGS='-g -O0
> > > $(SAN_CFLAGS)'                        \
> > > +                   EXTRA_LDFLAGS='$(SAN_LDFLAGS)'
> > > &&                          \
> > >                     cp $(RUNQSLOWER_OUTPUT)runqslower $@
> > >
> >
> > I wouldn't do it for runqslower, we just make sure that it compiles,
> > we don't really run it at all. No need to complicate its build, IMO.
>
> runqslower is linked with target libbpf, which is instrumented.
> This produces undefined symbol errors, since MSan runtime is expected
> to be a part of an executable.

ah, ok then, never mind

>
> [...]

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

* Re: [PATCH bpf-next 8/9] libbpf: Add MSan annotations
  2023-02-09 10:01     ` Ilya Leoshkevich
@ 2023-02-09 19:37       ` Andrii Nakryiko
  0 siblings, 0 replies; 22+ messages in thread
From: Andrii Nakryiko @ 2023-02-09 19:37 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev

On Thu, Feb 9, 2023 at 2:02 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Wed, 2023-02-08 at 17:29 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > MSan runs into a few false positives in libbpf. They all come from
> > > the
> > > fact that MSan does not know anything about the bpf syscall,
> > > particularly, what it writes to.
> > >
> > > Add libbpf_mark_defined() function to mark memory modified by the
> > > bpf
> > > syscall. Use the abstract name (it could be e.g.
> > > libbpf_msan_unpoison()), because it can be used for valgrind in the
> > > future as well.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> >
> > This is a lot to satisfy MSan, especially mark_map_value_defined
> > which
> > has to do bpf_obj_get_info_by_fd()... Is there any other way? What
> > happens if we don't do it?
>
> It would generate false positives when accessing the resulting map
> values, which is not nice. The main complexity in this case comes not
> from mark_map_value_defined() per se, but from the fact that we don't
> know the value size, especially for percpu maps.
>
> I toyed with the idea to extend the BPF_MAP_LOOKUP_ELEM interface to
> return the number of bytes written, but decided against it - the only
> user of this would be MSan, and at least for the beginning it's better
> to have extra complexity in userspace, rather than in kernel.

yep, I know, it's a source of very confusing problems in some cases.
Which is why bpf_map__lookup_elem() and such expect user to provide
total key/value size (and checks it against struct bpf_map's knowledge
of key/value size, taking into account if map is per-CPU)

Ok, I don't see any other way around this as well. Just please extract
this check of whether the map is per-cpu or not into a separate
helper, so we can maintain this list in only one place. Currently we
have this check in validate_map_op() and I don't want to duplicate
this.

>
> > As for libbpf_mark_defined, wouldn't it be cleaner to teach it to
> > handle NULL pointer and/or zero size transparently? Also, we can have
> > libbpf_mark_defined_if(void *ptr, size_t sz, bool cond), so in code
> > we
> > don't have to have those explicit if conditions. Instead of:
> >
> > > +       if (!ret && prog_ids)
> > > +               libbpf_mark_defined(prog_ids,
> > > +                                   attr.query.prog_cnt *
> > > sizeof(*prog_ids));
> >
> > we can write just
> >
> > libbpf_mark_defined_if(prog_ids, attr.query.prog_cnt *
> > sizeof(*prog_ids), ret == 0);
> >
> > ?
> >
> > Should we also call a helper something like
> > 'libbpf_mark_mem_written',
> > because that's what we are saying here, right?
>
> Sure, all this sounds reasonable. Will do.
>
> >
> > >  tools/lib/bpf/bpf.c             | 70
> > > +++++++++++++++++++++++++++++++--
> > >  tools/lib/bpf/btf.c             |  1 +
> > >  tools/lib/bpf/libbpf.c          |  1 +
> > >  tools/lib/bpf/libbpf_internal.h | 14 +++++++
> > >  4 files changed, 82 insertions(+), 4 deletions(-)
> > >
> >
> > [...]
> >
> > > diff --git a/tools/lib/bpf/libbpf_internal.h
> > > b/tools/lib/bpf/libbpf_internal.h
> > > index fbaf68335394..4e4622f66fdf 100644
> > > --- a/tools/lib/bpf/libbpf_internal.h
> > > +++ b/tools/lib/bpf/libbpf_internal.h
> > > @@ -577,4 +577,18 @@ static inline bool is_pow_of_2(size_t x)
> > >  #define PROG_LOAD_ATTEMPTS 5
> > >  int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int
> > > attempts);
> > >
> > > +#if defined(__has_feature)
> > > +#if __has_feature(memory_sanitizer)
> > > +#define LIBBPF_MSAN
> > > +#endif
> > > +#endif
> > > +
> > > +#ifdef LIBBPF_MSAN
> >
> > would below work:
> >
> > #if defined(__has_feature) && __has_feature(memory_sanitizer)
> >
> > ?
> >
> > > +#define HAVE_LIBBPF_MARK_DEFINED
> > > +#include <sanitizer/msan_interface.h>
> > > +#define libbpf_mark_defined __msan_unpoison
> > > +#else
> > > +static inline void libbpf_mark_defined(void *s, size_t n) {}
> > > +#endif
> > > +
> > >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > > --
> > > 2.39.1
> > >
>

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

end of thread, other threads:[~2023-02-09 19:37 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-08 20:56 [PATCH bpf-next 0/9] selftests/bpf: Add Memory Sanitizer support Ilya Leoshkevich
2023-02-08 20:56 ` [PATCH bpf-next 1/9] selftests/bpf: Quote host tools Ilya Leoshkevich
2023-02-08 20:56 ` [PATCH bpf-next 2/9] tools: runqslower: Add EXTRA_CFLAGS and EXTRA_LDFLAGS support Ilya Leoshkevich
2023-02-09  1:00   ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 3/9] selftests/bpf: Split SAN_CFLAGS and SAN_LDFLAGS Ilya Leoshkevich
2023-02-08 20:56 ` [PATCH bpf-next 4/9] selftests/bpf: Forward SAN_CFLAGS and SAN_LDFLAGS to runqslower and libbpf Ilya Leoshkevich
2023-02-09  1:03   ` Andrii Nakryiko
2023-02-09  9:55     ` Ilya Leoshkevich
2023-02-09 17:17       ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 5/9] selftests/bpf: Attach to fopen()/fclose() in uprobe_autoattach Ilya Leoshkevich
2023-02-09  1:05   ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 6/9] selftests/bpf: Attach to fopen()/fclose() in attach_probe Ilya Leoshkevich
2023-02-09  1:06   ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 7/9] libbpf: Fix alen calculation in libbpf_nla_dump_errormsg() Ilya Leoshkevich
2023-02-09  1:14   ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 8/9] libbpf: Add MSan annotations Ilya Leoshkevich
2023-02-09  1:29   ` Andrii Nakryiko
2023-02-09 10:01     ` Ilya Leoshkevich
2023-02-09 19:37       ` Andrii Nakryiko
2023-02-08 20:56 ` [PATCH bpf-next 9/9] selftests/bpf: " Ilya Leoshkevich
2023-02-09  1:34   ` Andrii Nakryiko
2023-02-09 10:30     ` Ilya Leoshkevich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.