* [PATCH v2 bpf-next 1/4] libbpf: add explicit padding to bpf_xdp_set_link_opts
2021-03-13 21:09 [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode Andrii Nakryiko
@ 2021-03-13 21:09 ` Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 2/4] bpftool: fix maybe-uninitialized warnings Andrii Nakryiko
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-13 21:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team
Adding such anonymous padding fixes the issue with uninitialized portions of
bpf_xdp_set_link_opts when using LIBBPF_DECLARE_OPTS macro with inline field
initialization:
DECLARE_LIBBPF_OPTS(bpf_xdp_set_link_opts, opts, .old_fd = -1);
When such code is compiled in debug mode, compiler is generating code that
leaves padding bytes uninitialized, which triggers error inside libbpf APIs
that do strict zero initialization checks for OPTS structs.
Adding anonymous padding field fixes the issue.
Fixes: bd5ca3ef93cd ("libbpf: Add function to set link XDP fd while specifying old program")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/libbpf.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 3c35eb401931..3d690d4e785c 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -507,6 +507,7 @@ struct xdp_link_info {
struct bpf_xdp_set_link_opts {
size_t sz;
int old_fd;
+ size_t :0;
};
#define bpf_xdp_set_link_opts__last_field old_fd
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf-next 2/4] bpftool: fix maybe-uninitialized warnings
2021-03-13 21:09 [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 1/4] libbpf: add explicit padding to bpf_xdp_set_link_opts Andrii Nakryiko
@ 2021-03-13 21:09 ` Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 3/4] selftests/bpf: fix maybe-uninitialized warning in xdpxceiver test Andrii Nakryiko
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-13 21:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team
Somehow when bpftool is compiled in -Og mode, compiler produces new warnings
about possibly uninitialized variables. Fix all the reported problems.
Fixes: 2119f2189df1 ("bpftool: add C output format option to btf dump subcommand")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/bpf/bpftool/btf.c | 3 +++
tools/bpf/bpftool/main.c | 3 +--
tools/bpf/bpftool/map.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 985610c3f193..62953bbf68b4 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -546,6 +546,7 @@ static int do_dump(int argc, char **argv)
NEXT_ARG();
if (argc < 1) {
p_err("expecting value for 'format' option\n");
+ err = -EINVAL;
goto done;
}
if (strcmp(*argv, "c") == 0) {
@@ -555,11 +556,13 @@ static int do_dump(int argc, char **argv)
} else {
p_err("unrecognized format specifier: '%s', possible values: raw, c",
*argv);
+ err = -EINVAL;
goto done;
}
NEXT_ARG();
} else {
p_err("unrecognized option: '%s'", *argv);
+ err = -EINVAL;
goto done;
}
}
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index b86f450e6fce..d9afb730136a 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -276,7 +276,7 @@ static int do_batch(int argc, char **argv)
int n_argc;
FILE *fp;
char *cp;
- int err;
+ int err = 0;
int i;
if (argc < 2) {
@@ -370,7 +370,6 @@ static int do_batch(int argc, char **argv)
} else {
if (!json_output)
printf("processed %d commands\n", lines);
- err = 0;
}
err_close:
if (fp != stdin)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index b400364ee054..09ae0381205b 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -100,7 +100,7 @@ static int do_dump_btf(const struct btf_dumper *d,
void *value)
{
__u32 value_id;
- int ret;
+ int ret = 0;
/* start of key-value pair */
jsonw_start_object(d->jw);
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf-next 3/4] selftests/bpf: fix maybe-uninitialized warning in xdpxceiver test
2021-03-13 21:09 [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 1/4] libbpf: add explicit padding to bpf_xdp_set_link_opts Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 2/4] bpftool: fix maybe-uninitialized warnings Andrii Nakryiko
@ 2021-03-13 21:09 ` Andrii Nakryiko
2021-03-13 21:09 ` [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode Andrii Nakryiko
2021-03-16 21:01 ` [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool " Alexei Starovoitov
4 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-13 21:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team
xsk_ring_prod__reserve() doesn't necessarily set idx in some conditions, so
from static analysis point of view compiler is right about the problems like:
In file included from xdpxceiver.c:92:
xdpxceiver.c: In function ‘xsk_populate_fill_ring’:
/data/users/andriin/linux/tools/testing/selftests/bpf/tools/include/bpf/xsk.h:119:20: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
return &addrs[idx & fill->mask];
~~~~^~~~~~~~~~~~
xdpxceiver.c:300:6: note: ‘idx’ was declared here
u32 idx;
^~~
xdpxceiver.c: In function ‘tx_only’:
xdpxceiver.c:596:30: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx + i);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fix two warnings reported by compiler by pre-initializing variable.
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/testing/selftests/bpf/xdpxceiver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
index 8b0f7fdd9003..1e21a3172687 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.c
+++ b/tools/testing/selftests/bpf/xdpxceiver.c
@@ -297,7 +297,7 @@ static void xsk_configure_umem(struct ifobject *data, void *buffer, u64 size)
static void xsk_populate_fill_ring(struct xsk_umem_info *umem)
{
int ret, i;
- u32 idx;
+ u32 idx = 0;
ret = xsk_ring_prod__reserve(&umem->fq, XSK_RING_PROD__DEFAULT_NUM_DESCS, &idx);
if (ret != XSK_RING_PROD__DEFAULT_NUM_DESCS)
@@ -584,7 +584,7 @@ static void rx_pkt(struct xsk_socket_info *xsk, struct pollfd *fds)
static void tx_only(struct xsk_socket_info *xsk, u32 *frameptr, int batch_size)
{
- u32 idx;
+ u32 idx = 0;
unsigned int i;
bool tx_invalid_test = stat_test_type == STAT_TEST_TX_INVALID;
u32 len = tx_invalid_test ? XSK_UMEM__DEFAULT_FRAME_SIZE + 1 : PKT_SIZE;
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode
2021-03-13 21:09 [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode Andrii Nakryiko
` (2 preceding siblings ...)
2021-03-13 21:09 ` [PATCH v2 bpf-next 3/4] selftests/bpf: fix maybe-uninitialized warning in xdpxceiver test Andrii Nakryiko
@ 2021-03-13 21:09 ` Andrii Nakryiko
2021-03-16 5:23 ` Andrii Nakryiko
2021-03-16 21:01 ` [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool " Alexei Starovoitov
4 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-13 21:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel; +Cc: andrii, kernel-team
Build selftests, bpftool, and libbpf in debug mode with DWARF data to
facilitate easier debugging.
In terms of impact on building and running selftests. Build is actually faster
now:
BEFORE: make -j60 380.21s user 37.87s system 1466% cpu 28.503 total
AFTER: make -j60 345.47s user 37.37s system 1599% cpu 23.939 total
test_progs runtime seems to be the same:
BEFORE:
real 1m5.139s
user 0m1.600s
sys 0m43.977s
AFTER:
real 1m3.799s
user 0m1.721s
sys 0m42.420s
Huge difference is being able to debug issues throughout test_progs, bpftool,
and libbpf without constantly updating 3 Makefiles by hand (including GDB
seeing the source code without any extra incantations).
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
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 c3999587bc23..d0db2b673c6f 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -21,7 +21,7 @@ endif
BPF_GCC ?= $(shell command -v bpf-gcc;)
SAN_CFLAGS ?=
-CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS) \
+CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS) \
-I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) \
-I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT) \
-Dbpf_prog_load=bpf_prog_test_load \
@@ -201,6 +201,7 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
$(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool
$(Q)$(MAKE) $(submake_extras) -C $(BPFTOOLDIR) \
CC=$(HOSTCC) LD=$(HOSTLD) \
+ EXTRA_CFLAGS='-g -Og' \
OUTPUT=$(HOST_BUILD_DIR)/bpftool/ \
prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
@@ -218,6 +219,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
../../../include/uapi/linux/bpf.h \
| $(INCLUDE_DIR) $(BUILD_DIR)/libbpf
$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
+ EXTRA_CFLAGS='-g -Og' \
DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
@@ -225,7 +227,8 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
../../../include/uapi/linux/bpf.h \
| $(INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) \
- OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
+ EXTRA_CFLAGS='-g -Og' \
+ OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers
endif
--
2.24.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode
2021-03-13 21:09 ` [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode Andrii Nakryiko
@ 2021-03-16 5:23 ` Andrii Nakryiko
0 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-16 5:23 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Sat, Mar 13, 2021 at 1:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Build selftests, bpftool, and libbpf in debug mode with DWARF data to
> facilitate easier debugging.
>
> In terms of impact on building and running selftests. Build is actually faster
> now:
>
> BEFORE: make -j60 380.21s user 37.87s system 1466% cpu 28.503 total
> AFTER: make -j60 345.47s user 37.37s system 1599% cpu 23.939 total
>
> test_progs runtime seems to be the same:
>
> BEFORE:
> real 1m5.139s
> user 0m1.600s
> sys 0m43.977s
>
> AFTER:
> real 1m3.799s
> user 0m1.721s
> sys 0m42.420s
>
> Huge difference is being able to debug issues throughout test_progs, bpftool,
> and libbpf without constantly updating 3 Makefiles by hand (including GDB
> seeing the source code without any extra incantations).
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> 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 c3999587bc23..d0db2b673c6f 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -21,7 +21,7 @@ endif
>
> BPF_GCC ?= $(shell command -v bpf-gcc;)
> SAN_CFLAGS ?=
> -CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS) \
> +CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS) \
> -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR) \
> -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT) \
> -Dbpf_prog_load=bpf_prog_test_load \
> @@ -201,6 +201,7 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
> $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool
> $(Q)$(MAKE) $(submake_extras) -C $(BPFTOOLDIR) \
> CC=$(HOSTCC) LD=$(HOSTLD) \
> + EXTRA_CFLAGS='-g -Og' \
I was asked about '-Og' flag and the minimum GCC version that supports
it. It seems it was added in GCC 4.8 ([0]), so given the kernel's
minimum version is GCC 4.9, we shouldn't need take any extra
precautions to handle older compilers.
[0] https://gcc.gnu.org/gcc-4.8/changes.html
> OUTPUT=$(HOST_BUILD_DIR)/bpftool/ \
> prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
>
> @@ -218,6 +219,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
> ../../../include/uapi/linux/bpf.h \
> | $(INCLUDE_DIR) $(BUILD_DIR)/libbpf
> $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
> + EXTRA_CFLAGS='-g -Og' \
> DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
>
> ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
> @@ -225,7 +227,8 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile) \
> ../../../include/uapi/linux/bpf.h \
> | $(INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
> $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) \
> - OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
> + EXTRA_CFLAGS='-g -Og' \
> + OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
> DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers
> endif
>
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode
2021-03-13 21:09 [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode Andrii Nakryiko
` (3 preceding siblings ...)
2021-03-13 21:09 ` [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode Andrii Nakryiko
@ 2021-03-16 21:01 ` Alexei Starovoitov
2021-03-16 21:36 ` Andrii Nakryiko
4 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2021-03-16 21:01 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, netdev, daniel; +Cc: kernel-team
On 3/13/21 1:09 PM, Andrii Nakryiko wrote:
> Build BPF selftests and libbpf and bpftool, that are used as part of
> selftests, in debug mode (specifically, -Og). This makes it much simpler and
> nicer to do development and/or bug fixing. See patch #4 for some unscientific
> measurements.
>
> This patch set fixes new maybe-unitialized warnings produced in -Og build
> mode. Patch #1 fixes the blocker which was causing some XDP selftests failures
> due to non-zero padding in bpf_xdp_set_link_opts, which only happened in debug
> mode.
It was applied. gitbot doesn't seem to auto-reply anymore. hmm.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode
2021-03-16 21:01 ` [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool " Alexei Starovoitov
@ 2021-03-16 21:36 ` Andrii Nakryiko
0 siblings, 0 replies; 8+ messages in thread
From: Andrii Nakryiko @ 2021-03-16 21:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Kernel Team
On Tue, Mar 16, 2021 at 2:02 PM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 3/13/21 1:09 PM, Andrii Nakryiko wrote:
> > Build BPF selftests and libbpf and bpftool, that are used as part of
> > selftests, in debug mode (specifically, -Og). This makes it much simpler and
> > nicer to do development and/or bug fixing. See patch #4 for some unscientific
> > measurements.
> >
> > This patch set fixes new maybe-unitialized warnings produced in -Og build
> > mode. Patch #1 fixes the blocker which was causing some XDP selftests failures
> > due to non-zero padding in bpf_xdp_set_link_opts, which only happened in debug
> > mode.
>
> It was applied. gitbot doesn't seem to auto-reply anymore. hmm.
Thanks! Yeah, it seems to be unreliable lately. I think it sent some
notifications yesterday, but not all.
^ permalink raw reply [flat|nested] 8+ messages in thread