[PATCH 0/4] tools/bpf: allow building with musl

[PATCH 0/4] tools/bpf: allow building with musl

[Dominique Martinet]

Hi,

I'd like to build bpftool on alpine linux, which is musl based.

There are a few incompatibilities with it, I've commented on each patch
when I could think of alternative solutions.

I've tested the patch on an x86_64 debian testing with no problem, so
didn't obviously break glibc builds, and the binaries built for alpine
seem to work on aarch64 as well.


Dominique Martinet (4):
  tools/runqslower: musl compat: explicitly link with libargp if found
  tools/bpf: musl compat: do not use DEFFILEMODE
  tools/bpf: musl compat: replace nftw with FTW_ACTIONRETVAL
  tools/bpf: replace sys/fcntl.h by fcntl.h

 tools/bpf/bpf_jit_disasm.c         |   2 +-
 tools/bpf/bpftool/perf.c           | 115 +++++++++++++++--------------
 tools/bpf/bpftool/tracelog.c       |   2 +-
 tools/bpf/runqslower/Makefile      |  30 +++++++-
 tools/build/feature/Makefile       |   4 +
 tools/build/feature/test-all.c     |   4 +
 tools/build/feature/test-libargp.c |  14 ++++
 7 files changed, 111 insertions(+), 60 deletions(-)
 create mode 100644 tools/build/feature/test-libargp.c

-- 
2.35.1

[PATCH 1/4] tools/bpf/runqslower: musl compat: explicitly link with libargp if found

[Dominique Martinet]

musl doesn't implement argp.h and requires an explicit lib for it, so
we must test for -largp presence and use it if appropriate

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

After having done this work I noticed runqslower is not actually
installed, so ideally instead of all of this it'd make more sense to
just not build it: would it make sense to take it out of the defaults
build targets?
I could just directly build the appropriate targets from tools/bpf
directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but
ideally I'd like to keep alpine's build script way of calling make from
the tools parent directory, and 'make bpf' there is all or nothing.


OTOH, we might as well keep this to allow people on alpine/void linux to
build runqslower if they want to. I didn't add libargp to default features
check so it shouldn't change much except for runqslower itself.
As an example it might be better to keep it independant from kbuild but
it already wasn't so I don't see much harm here.

 tools/bpf/runqslower/Makefile      | 30 +++++++++++++++++++++++++++++-
 tools/build/feature/Makefile       |  4 ++++
 tools/build/feature/test-all.c     |  4 ++++
 tools/build/feature/test-libargp.c | 14 ++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 tools/build/feature/test-libargp.c

diff --git a/tools/bpf/runqslower/Makefile b/tools/bpf/runqslower/Makefile
index da6de16a3dfb..20a1d9a2a908 100644
--- a/tools/bpf/runqslower/Makefile
+++ b/tools/bpf/runqslower/Makefile
@@ -23,6 +23,34 @@ VMLINUX_BTF_PATHS := $(if $(O),$(O)/vmlinux)		\
 VMLINUX_BTF_PATH := $(or $(VMLINUX_BTF),$(firstword			       \
 					  $(wildcard $(VMLINUX_BTF_PATHS))))
 
+# musl requires linking with an external libargp
+FEATURE_USER = .runqslower
+FEATURE_TEST = libargp
+FEATURE_DISPLAY =
+
+check_feat := 1
+NON_CHECK_FEAT_TARGETS := clean
+ifdef MAKECMDGOALS
+ifeq ($(filter-out $(NON_CHECK_FEAT_TARGETS),$(MAKECMDGOALS)),)
+  check_feat := 0
+endif
+endif
+
+ifeq ($(check_feat),1)
+ifeq ($(FEATURES_DUMP),)
+srctree := $(abspath ../../..)
+include $(srctree)/tools/build/Makefile.feature
+else
+include $(FEATURES_DUMP)
+endif
+endif
+
+LIBS = -lelf -lz
+$(call feature_check,libargp)
+ifeq ($(feature-libargp), 1)
+LIBS += -largp
+endif
+
 ifeq ($(V),1)
 Q =
 else
@@ -49,7 +77,7 @@ clean:
 libbpf_hdrs: $(BPFOBJ)
 
 $(OUTPUT)/runqslower: $(OUTPUT)/runqslower.o $(BPFOBJ)
-	$(QUIET_LINK)$(CC) $(CFLAGS) $^ -lelf -lz -o $@
+	$(QUIET_LINK)$(CC) $(CFLAGS) $^ $(LIBS) -o $@
 
 $(OUTPUT)/runqslower.o: runqslower.h $(OUTPUT)/runqslower.skel.h	      \
 			$(OUTPUT)/runqslower.bpf.o | libbpf_hdrs
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index de66e1cc0734..ceb4224a0ede 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -37,6 +37,7 @@ FILES=                                          \
          test-libtraceevent.bin                 \
          test-libtracefs.bin                    \
          test-libcrypto.bin                     \
+         test-libargp.bin                       \
          test-libunwind.bin                     \
          test-libunwind-debug-frame.bin         \
          test-libunwind-x86.bin                 \
@@ -205,6 +206,9 @@ $(OUTPUT)test-libtracefs.bin:
 $(OUTPUT)test-libcrypto.bin:
 	$(BUILD) -lcrypto
 
+$(OUTPUT)test-libargp.bin:
+	$(BUILD) -largp
+
 $(OUTPUT)test-gtk2.bin:
 	$(BUILD) $(shell $(PKG_CONFIG) --libs --cflags gtk+-2.0 2>/dev/null) -Wno-deprecated-declarations
 
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 5ffafb967b6e..149d3ef4a439 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -146,6 +146,10 @@
 # include "test-libcrypto.c"
 #undef main
 
+#define main main_test_libargp
+# include "test-libargp.c"
+#undef main
+
 #define main main_test_sdt
 # include "test-sdt.c"
 #undef main
diff --git a/tools/build/feature/test-libargp.c b/tools/build/feature/test-libargp.c
new file mode 100644
index 000000000000..63b65d1f11fe
--- /dev/null
+++ b/tools/build/feature/test-libargp.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <argp.h>
+
+const char *argp_program_version = "test-libargp";
+static const struct argp_option opts[] = { {} };
+
+int main(int argc, char **argv)
+{
+	static const struct argp argp = {
+		.options = opts,
+	};
+	argp_parse(&argp, argc, argv, 0, NULL, NULL);
+	return 0;
+}
-- 
2.35.1

Re: [PATCH 1/4] tools/bpf/runqslower: musl compat: explicitly link with libargp if found

[Dominique Martinet]

Dominique Martinet wrote on Sun, Apr 24, 2022 at 02:10:19PM +0900:
> After having done this work I noticed runqslower is not actually
> installed, so ideally instead of all of this it'd make more sense to
> just not build it: would it make sense to take it out of the defaults
> build targets?
> I could just directly build the appropriate targets from tools/bpf
> directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but
> ideally I'd like to keep alpine's build script way of calling make from
> the tools parent directory, and 'make bpf' there is all or nothing.

Well, it turns out runqslower doesn't build if the current kernel or
vmlinux in tree don't have BTF enabled, so the current alpine builder
can't build it.

I've dropped this patch from my alpine MR[1] and built things directly
with make bpftool etc as suggested above, so my suggestion to make it
more easily buildable that way is probably the way to go?
[1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554


Thanks,
-- 
Dominique

Re: [PATCH 1/4] tools/bpf/runqslower: musl compat: explicitly link with libargp if found

[Daniel Borkmann]

On 4/24/22 8:58 AM, Dominique Martinet wrote:
> Dominique Martinet wrote on Sun, Apr 24, 2022 at 02:10:19PM +0900:
>> After having done this work I noticed runqslower is not actually
>> installed, so ideally instead of all of this it'd make more sense to
>> just not build it: would it make sense to take it out of the defaults
>> build targets?
>> I could just directly build the appropriate targets from tools/bpf
>> directory with 'make bpftool bpf_dbg bpf_asm bpf_jit_disasm', but
>> ideally I'd like to keep alpine's build script way of calling make from
>> the tools parent directory, and 'make bpf' there is all or nothing.
> 
> Well, it turns out runqslower doesn't build if the current kernel or
> vmlinux in tree don't have BTF enabled, so the current alpine builder
> can't build it.
> 
> I've dropped this patch from my alpine MR[1] and built things directly
> with make bpftool etc as suggested above, so my suggestion to make it
> more easily buildable that way is probably the way to go?
> [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554

Thanks for looking into this, Dominique! I slightly massaged patch 3 & 4
and applied it to bpf-next tree.

I don't really mind about patch 1 & 2, though out of tools/bpf/ the only
one you /really/ might want to package is bpftool. The other tools are on
the legacy side of things and JIT disasm you can also get via bpftool anyway.

Given this is not covered by BPF CI, are you planning to regularly check
for musl compatibility before a new kernel is cut?

Thanks,
Daniel

Re: [PATCH 1/4] tools/bpf/runqslower: musl compat: explicitly link with libargp if found

[Dominique Martinet]

Daniel Borkmann wrote on Mon, Apr 25, 2022 at 11:35:41PM +0200:
> > I've dropped this patch from my alpine MR[1] and built things directly
> > with make bpftool etc as suggested above, so my suggestion to make it
> > more easily buildable that way is probably the way to go?
> > [1] https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/33554
> 
> Thanks for looking into this, Dominique! I slightly massaged patch 3 & 4
> and applied it to bpf-next tree.

Thanks!

> I don't really mind about patch 1 & 2, though out of tools/bpf/ the only
> one you /really/ might want to package is bpftool. The other tools are on
> the legacy side of things and JIT disasm you can also get via bpftool anyway.

I was thinking the other tools still had their uses, but I'll readily
admit I've never had a need for them so wasn't sure if I should package
them together or not.

I can see the use of bpf_dbg, but it's occasional enough that people who
need it can just build it when they need... Let's drop both patches and
I'll remove the other legacy tools from package as well.

My last concern would then just be to build it more easily. I just
noticed I can actually 'make bpf/bpftool' directly from the tools/
parent directory, but there's no equivalent for _install rules.

Would something like this make sense? (I can resend as proper patch if
so)
----
diff --git a/tools/Makefile b/tools/Makefile
index db2f7b8ebed5..743d242aebb3 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -112,6 +112,9 @@ cpupower_install:
 cgroup_install counter_install firewire_install gpio_install hv_install iio_install perf_install bootconfig_install spi_install usb_install virtio_install vm_install bpf_install objtool_install wmi_install pci_install debugging_install tracing_install:
        $(call descend,$(@:_install=),install)
 
+bpf/%_install: FORCE
+       $(call descend,$(@:_install=),install)
+
 selftests_install:
        $(call descend,testing/$(@:_install=),install)
 
----


> Given this is not covered by BPF CI, are you planning to regularly check
> for musl compatibility before a new kernel is cut?

alpine doesn't update the 'tools' subpackage with every kernel release,
I'm not sure what the exact schedule is but from the looks of it it
tracks LTS releases with updates every few months within the stable
release or to the next one.


I don't really have any resource to run a regular CI, but I guess I can
check from time to time.. If I ever get around to adding a linux-next
test to work's CI I can check bpftool builds at the same time, but who
knows when that'll ever be.

OTOH I had a first look last year (back when I tried to push
ACTIONRETVAL to musl) and there haven't been any new incompatibility, so
I think it's fine to just deal with minor hiccups when alpine upgrades
once in a while.

-- 
Dominique

[PATCH 2/4] tools/bpf: musl compat: do not use DEFFILEMODE

[Dominique Martinet]

DEFFILEMODE is not defined on musl libc.

Linus has expressed preference towards using explicit octal value in
the past over combinaisons of S_Ix{USR,GRP,OTH}, so just replace it
with 0666 directly

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

I wanted to link to the Linus mail that said this, but it turns out
there weren't any list in Cc... I could be making this up but here's the
relevant part of his mail, which I hope is acceptable to forward as
there's nothing personal in it:
----
Date: Sat, 27 Feb 2021 11:29:31 -0800
From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [RFC][PATCHSET] inode type bits fixes
Message-ID: <CAHk-=whRkLinjW3gRJQ=fWHZcFP5iy37+4VVr38TzSXEwZrZGg@mail.gmail.com>

[...]

Finally, I absolutely _abhor_ the crazy "S_%&^%&^$" macros. They are
completely illegible garbage, imnsho. I'm looking at that

+                       mode = stat->st_mode & S_IALLUGO;
+                       mode |= inode->i_mode & ~S_IALLUGO;

and I'm like "WTF is that random character sequence again".

In this case, it's everything but the format.  I think it would be
more legible written the other way around, ie

+                       mode = stat->st_mode & ~S_IFMT;
+                       mode |= inode->i_mode & S_IFMT;

because at least that one has _less_ of the stupid random-generated letters.

Every single one of the "UGO" things are pure and utter crap. The
octal representation of the actual permissions masks are _way_ more
legible than the insane "standard" names for them.
----


 tools/bpf/bpf_jit_disasm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpf_jit_disasm.c b/tools/bpf/bpf_jit_disasm.c
index c8ae95804728..f748863e294c 100644
--- a/tools/bpf/bpf_jit_disasm.c
+++ b/tools/bpf/bpf_jit_disasm.c
@@ -303,7 +303,7 @@ int main(int argc, char **argv)
 		goto done;
 	}
 
-	ofd = open(ofile, O_WRONLY | O_CREAT | O_TRUNC, DEFFILEMODE);
+	ofd = open(ofile, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 	if (ofd < 0) {
 		fprintf(stderr, "Could not open file %s for writing: ", ofile);
 		perror(NULL);
-- 
2.35.1

[PATCH 3/4] tools/bpf: musl compat: replace nftw with FTW_ACTIONRETVAL

[Dominique Martinet]

musl nftw implementation does not support FTW_ACTIONRETVAL.

There have been multiple attempts at pushing the feature in musl
upstream but it has been refused or ignored all the times:
https://www.openwall.com/lists/musl/2021/03/26/1
https://www.openwall.com/lists/musl/2022/01/22/1

In this case we only care about /proc/<pid>/fd/<fd>, so it's not
too difficult to reimplement directly instead, and the new
implementation makes 'bpftool perf' slightly faster because it doesn't
needlessly stat/readdir unneeded directories (54ms -> 13ms on my machine)

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

Alternatively alpine has one package where they reimplemented nftw with
FTW_ACTIONRETVAL support locally, so if reaaallly needed we could do the
same here.. But honestly doing two readdirs is probably just as simple
for this particular case.

 tools/bpf/bpftool/perf.c | 116 ++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 57 deletions(-)

diff --git a/tools/bpf/bpftool/perf.c b/tools/bpf/bpftool/perf.c
index 50de087b0db7..de793872544e 100644
--- a/tools/bpf/bpftool/perf.c
+++ b/tools/bpf/bpftool/perf.c
@@ -11,7 +11,7 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
-#include <ftw.h>
+#include <dirent.h>
 
 #include <bpf/bpf.h>
 
@@ -147,81 +147,83 @@ static void print_perf_plain(int pid, int fd, __u32 prog_id, __u32 fd_type,
 	}
 }
 
-static int show_proc(const char *fpath, const struct stat *sb,
-		     int tflag, struct FTW *ftwbuf)
+static int show_proc(void)
 {
 	__u64 probe_offset, probe_addr;
 	__u32 len, prog_id, fd_type;
-	int err, pid = 0, fd = 0;
+	int err, pid, fd;
+	DIR *proc, *pid_fd;
+	struct dirent *proc_de, *pid_fd_de;
 	const char *pch;
 	char buf[4096];
 
-	/* prefix always /proc */
-	pch = fpath + 5;
-	if (*pch == '\0')
-		return 0;
+	proc = opendir("/proc");
+	if (!proc)
+		return -1;
+	while ((proc_de = readdir(proc))) {
+		pid = 0;
+		pch = proc_de->d_name;
 
-	/* pid should be all numbers */
-	pch++;
-	while (isdigit(*pch)) {
-		pid = pid * 10 + *pch - '0';
-		pch++;
+		/* pid should be all numbers */
+		while (isdigit(*pch)) {
+			pid = pid * 10 + *pch - '0';
+			pch++;
+		}
+		if (*pch != '\0')
+			continue;
+
+		err = snprintf(buf, sizeof(buf), "/proc/%s/fd", proc_de->d_name);
+		if (err < 0 || err >= (int)sizeof(buf))
+			continue;
+
+		pid_fd = opendir(buf);
+		if (!pid_fd)
+			continue;
+
+		while ((pid_fd_de = readdir(pid_fd))) {
+			fd = 0;
+			pch = pid_fd_de->d_name;
+
+			/* fd should be all numbers */
+			while (isdigit(*pch)) {
+				fd = fd * 10 + *pch - '0';
+				pch++;
+			}
+			if (*pch != '\0')
+				continue;
+
+			/* query (pid, fd) for potential perf events */
+			len = sizeof(buf);
+			err = bpf_task_fd_query(pid, fd, 0, buf, &len, &prog_id, &fd_type,
+						&probe_offset, &probe_addr);
+			if (err < 0)
+				continue;
+
+			if (json_output)
+				print_perf_json(pid, fd, prog_id, fd_type, buf, probe_offset,
+						probe_addr);
+			else
+				print_perf_plain(pid, fd, prog_id, fd_type, buf, probe_offset,
+						 probe_addr);
+		}
+		closedir(pid_fd);
 	}
-	if (*pch == '\0')
-		return 0;
-	if (*pch != '/')
-		return FTW_SKIP_SUBTREE;
-
-	/* check /proc/<pid>/fd directory */
-	pch++;
-	if (strncmp(pch, "fd", 2))
-		return FTW_SKIP_SUBTREE;
-	pch += 2;
-	if (*pch == '\0')
-		return 0;
-	if (*pch != '/')
-		return FTW_SKIP_SUBTREE;
-
-	/* check /proc/<pid>/fd/<fd_num> */
-	pch++;
-	while (isdigit(*pch)) {
-		fd = fd * 10 + *pch - '0';
-		pch++;
-	}
-	if (*pch != '\0')
-		return FTW_SKIP_SUBTREE;
-
-	/* query (pid, fd) for potential perf events */
-	len = sizeof(buf);
-	err = bpf_task_fd_query(pid, fd, 0, buf, &len, &prog_id, &fd_type,
-				&probe_offset, &probe_addr);
-	if (err < 0)
-		return 0;
-
-	if (json_output)
-		print_perf_json(pid, fd, prog_id, fd_type, buf, probe_offset,
-				probe_addr);
-	else
-		print_perf_plain(pid, fd, prog_id, fd_type, buf, probe_offset,
-				 probe_addr);
-
+	closedir(proc);
 	return 0;
 }
 
 static int do_show(int argc, char **argv)
 {
-	int flags = FTW_ACTIONRETVAL | FTW_PHYS;
-	int err = 0, nopenfd = 16;
+	int err;
 
 	if (!has_perf_query_support())
 		return -1;
 
 	if (json_output)
 		jsonw_start_array(json_wtr);
-	if (nftw("/proc", show_proc, nopenfd, flags) == -1) {
-		p_err("%s", strerror(errno));
-		err = -1;
-	}
+
+	err = show_proc();
+
 	if (json_output)
 		jsonw_end_array(json_wtr);
 
-- 
2.35.1

Re: [PATCH 3/4] tools/bpf: musl compat: replace nftw with FTW_ACTIONRETVAL

[Quentin Monnet]

On Sun, 24 Apr 2022 at 06:11, Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> musl nftw implementation does not support FTW_ACTIONRETVAL.
>
> There have been multiple attempts at pushing the feature in musl
> upstream but it has been refused or ignored all the times:
> https://www.openwall.com/lists/musl/2021/03/26/1
> https://www.openwall.com/lists/musl/2022/01/22/1
>
> In this case we only care about /proc/<pid>/fd/<fd>, so it's not
> too difficult to reimplement directly instead, and the new
> implementation makes 'bpftool perf' slightly faster because it doesn't
> needlessly stat/readdir unneeded directories (54ms -> 13ms on my machine)
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Acked-by: Quentin Monnet <quentin@isovalent.com>

[PATCH 4/4] tools/bpf: replace sys/fcntl.h by fcntl.h

[Dominique Martinet]

musl does not like including sys/fcntl.h directly:
    1 | #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h>

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
 tools/bpf/bpftool/tracelog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/tracelog.c b/tools/bpf/bpftool/tracelog.c
index e80a5c79b38f..bf1f02212797 100644
--- a/tools/bpf/bpftool/tracelog.c
+++ b/tools/bpf/bpftool/tracelog.c
@@ -9,7 +9,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <linux/magic.h>
-#include <sys/fcntl.h>
+#include <fcntl.h>
 #include <sys/vfs.h>
 
 #include "main.h"
-- 
2.35.1

Re: [PATCH 4/4] tools/bpf: replace sys/fcntl.h by fcntl.h

[Quentin Monnet]

On Sun, 24 Apr 2022 at 06:11, Dominique Martinet <asmadeus@codewreck.org> wrote:
>
> musl does not like including sys/fcntl.h directly:
>     1 | #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h>
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>

Acked-by: Quentin Monnet <quentin@isovalent.com>

Re: [PATCH 0/4] tools/bpf: allow building with musl

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Sun, 24 Apr 2022 14:10:18 +0900 you wrote:
> Hi,
> 
> I'd like to build bpftool on alpine linux, which is musl based.
> 
> There are a few incompatibilities with it, I've commented on each patch
> when I could think of alternative solutions.
> 
> [...]

Here is the summary with links:
  - [1/4] tools/bpf/runqslower: musl compat: explicitly link with libargp if found
    (no matching commit)
  - [2/4] tools/bpf: musl compat: do not use DEFFILEMODE
    (no matching commit)
  - [3/4] tools/bpf: musl compat: replace nftw with FTW_ACTIONRETVAL
    https://git.kernel.org/bpf/bpf-next/c/93bc2e9e943d
  - [4/4] tools/bpf: replace sys/fcntl.h by fcntl.h
    https://git.kernel.org/bpf/bpf-next/c/246bdfa52f33

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