Re: [PATCH bpf-next v4 3/8] libbpf: Fix output .symtab byte-order during linking
On Fri, Aug 30, 2024 at 03:15:10PM -0700, Eduard Zingerman wrote:
> On Fri, 2024-08-30 at 00:29 -0700, Tony Ambardar wrote:
> > Object linking output data uses the default ELF_T_BYTE type for '.symtab'
> > section data, which disables any libelf-based translation. Explicitly set
> > the ELF_T_SYM type for output to restore libelf's byte-order conversion,
> > noting that input '.symtab' data is already correctly translated.
> >
> > Fixes: faf6ed321cf6 ("libbpf: Add BPF static linker APIs")
> > Signed-off-by: Tony Ambardar
> > ---
> > tools/lib/bpf/linker.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 9cd3d4109788..7489306cd6f7 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -396,6 +396,8 @@ static int init_output_elf(struct bpf_linker *linker,
> > const char *file)
> > pr_warn_elf("failed to create SYMTAB data");
> > return -EINVAL;
> > }
> > + /* Ensure libelf translates byte-order of symbol records */
> > + sec->data->d_type = ELF_T_SYM;
>
> I tried grepping through libelf to find out how this affects things,
> and identified that it is primarily used by
> elfutils/libelf/gelf_xlatetof.c:gelf_xlatetof(),
> which is an interface function and we don't seem to use it.
> It is also used by dwfl_* functions while applying relocations,
> but we don't use that either.
>
Right, gelf_xlatetof() is exposed for _explicit_ user conversions, but
libelf still does translations implicitly for known section record types,
based on the ELF file's byte-order metadata. The idea is that ELF data
loaded in memory will be native-endianness for accessibility, but output
in the original endianness at rest/in a file, all transparently.
We try to follow the same idea in libbpf when opening and writing .BTF
and .BTF.ext data (e.g. see the *_raw_data() funcs).
> Could you please elaborate a bit on effects of this change?
>
When linking objects of either endianness, libelf can translate the input
files based on ELF headers (endianness and type ELF_T_SYM) and allows us to
process .symtab data. When writing out the linked file however, we create a
new .symtab section in init_output_elf() but leave it as default ELT_T_BYTE
type, which undergoes no translation and leaves .symtab always in native
byte-order regardless of target endianness.
See also 61e8aeda9398 ("bpf: Fix libelf endian handling in resolv_btfids")
and related links for a similar example and explanations. Hope that helps.
Cheers,
Tony
> >
> > str_off = strset__add_str(linker->strtab_strs, sec->sec_name);
> > if (str_off < 0)
>
[PATCH net-next] selftests: add selftest for UDP SO_PEEK_OFF support
From: Jason Xing
Add the SO_PEEK_OFF selftest for UDP. In this patch, I mainly do
three things:
1. rename tcp_so_peek_off.c
2. adjust for UDP protocol
3. add selftests into it
Suggested-by: Jon Maloy
Signed-off-by: Jason Xing
---
Link:
https://lore.kernel.org/all/9f4dd14d-fbe3-4c61-b04c-f0e6b8096...@redhat.com/
---
tools/testing/selftests/net/Makefile | 2 +-
.../{tcp_so_peek_off.c => sk_so_peek_off.c} | 91 +++
2 files changed, 56 insertions(+), 37 deletions(-)
rename tools/testing/selftests/net/{tcp_so_peek_off.c => sk_so_peek_off.c}
(58%)
diff --git a/tools/testing/selftests/net/Makefile
b/tools/testing/selftests/net/Makefile
index 1179e3261bef..d5029f978aa9 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -80,7 +80,7 @@ TEST_PROGS += io_uring_zerocopy_tx.sh
TEST_GEN_FILES += bind_bhash
TEST_GEN_PROGS += sk_bind_sendto_listen
TEST_GEN_PROGS += sk_connect_zero_addr
-TEST_GEN_PROGS += tcp_so_peek_off
+TEST_GEN_PROGS += sk_so_peek_off
TEST_PROGS += test_ingress_egress_chaining.sh
TEST_GEN_PROGS += so_incoming_cpu
TEST_PROGS += sctp_vrf.sh
diff --git a/tools/testing/selftests/net/tcp_so_peek_off.c
b/tools/testing/selftests/net/sk_so_peek_off.c
similarity index 58%
rename from tools/testing/selftests/net/tcp_so_peek_off.c
rename to tools/testing/selftests/net/sk_so_peek_off.c
index df8a39d9d3c3..870a890138c4 100644
--- a/tools/testing/selftests/net/tcp_so_peek_off.c
+++ b/tools/testing/selftests/net/sk_so_peek_off.c
@@ -10,37 +10,41 @@
#include
#include "../kselftest.h"
-static char *afstr(int af)
+static char *afstr(int af, int proto)
{
- return af == AF_INET ? "TCP/IPv4" : "TCP/IPv6";
+ if (proto == IPPROTO_TCP)
+ return af == AF_INET ? "TCP/IPv4" : "TCP/IPv6";
+ else
+ return af == AF_INET ? "UDP/IPv4" : "UDP/IPv6";
}
-int tcp_peek_offset_probe(sa_family_t af)
+int sk_peek_offset_probe(sa_family_t af, int proto)
{
+ int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
int optv = 0;
int ret = 0;
int s;
- s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
+ s = socket(af, type, proto);
if (s < 0) {
ksft_perror("Temporary TCP socket creation failed");
} else {
if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
ret = 1;
else
- printf("%s does not support SO_PEEK_OFF\n", afstr(af));
+ printf("%s does not support SO_PEEK_OFF\n", afstr(af,
proto));
close(s);
}
return ret;
}
-static void tcp_peek_offset_set(int s, int offset)
+static void sk_peek_offset_set(int s, int offset)
{
if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset)))
ksft_perror("Failed to set SO_PEEK_OFF value\n");
}
-static int tcp_peek_offset_get(int s)
+static int sk_peek_offset_get(int s)
{
int offset;
socklen_t len = sizeof(offset);
@@ -50,8 +54,9 @@ static int tcp_peek_offset_get(int s)
return offset;
}
-static int tcp_peek_offset_test(sa_family_t af)
+static int sk_peek_offset_test(sa_family_t af, int proto)
{
+ int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
union {
struct sockaddr sa;
struct sockaddr_in a4;
@@ -62,13 +67,13 @@ static int tcp_peek_offset_test(sa_family_t af)
int recv_sock = 0;
int offset = 0;
ssize_t len;
- char buf;
+ char buf[2];
memset(&a, 0, sizeof(a));
a.sa.sa_family = af;
- s[0] = socket(af, SOCK_STREAM, IPPROTO_TCP);
- s[1] = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
+ s[0] = recv_sock = socket(af, type, proto);
+ s[1] = socket(af, type, proto);
if (s[0] < 0 || s[1] < 0) {
ksft_perror("Temporary socket creation failed\n");
@@ -82,76 +87,78 @@ static int tcp_peek_offset_test(sa_family_t af)
ksft_perror("Temporary socket getsockname() failed\n");
goto out;
}
- if (listen(s[0], 0) < 0) {
+ if (proto == IPPROTO_TCP && listen(s[0], 0) < 0) {
ksft_perror("Temporary socket listen() failed\n");
goto out;
}
- if (connect(s[1], &a.sa, sizeof(a)) >= 0 || errno != EINPROGRESS) {
+ if (connect(s[1], &a.sa, sizeof(a))) {
ksft_perror("Temporary socket connect() failed\n");
goto out;
}
- recv_sock = accept(s[0], NULL, NULL);
- if (recv_sock <= 0) {
- ksft_perror("Temporary socket accept() failed\n");
- goto out;
+ if (proto == IPPROTO_TCP) {
+ recv_sock = accept(s[0], NULL, NULL);
+ if (recv_sock <= 0) {
+ ksft_perror("Temporary socket accept() failed\n");
+
Re: [PATCH bpf-next v4 7/8] libbpf: Support creating light skeleton of either endianness
On Fri, Aug 30, 2024 at 06:24:47PM -0700, Alexei Starovoitov wrote: > On Fri, Aug 30, 2024 at 2:31 PM Andrii Nakryiko > wrote: > > > > > > > > for the rest, Alexei, can you please review and give your ack? > > It looks fine. > All of the additional pr_debug()s look a bit excessive. > Will take another look at respin. Thanks for taking a look. My experience is the added pr_debug() were essential to troubleshooting problems with the loader blobs, and only provide indexes to embedded data. I'd hate for someone else to debug without these. I think several of them could be consolidated however, so let me have a try. There's another area where I'd appreciate your help. The loader code includes some inline debug statements, but I wasn't ever able to make these work or see them in trace logs. Could you share an example of how to enable these (e.g. for test_progs tests)? Thanks, Tony
[PATCH] selftests: vDSO: Do not rely on $ARCH for vdso_test_getrandom && vdso_test_chacha
$ARCH is not always enough to know whether getrandom vDSO is supported
or not. For instance on x86 we want it for x86_64 but not i386.
On the other hand, we already have detailed architecture selection in
vdso_config.h, the only difference is that it cannot be used for
Makefile. But most selftests are built regardless of whether a
functionality is supported or not. The return value KSFT_SKIP is there
for than: it tells the test is skipped because it is not supported.
Make the implementation more flexible by setting a VDSO_GETRANDOM
macro in vdso_config.h. That macro contains the path to the file that
defines __arch_chacha20_blocks_nostack(). It avoids the symbolic
link to vdso directory and will allow architectures to have several
implementations of __arch_chacha20_blocks_nostack() if needed.
Then restore the original behaviour which was dedicated to
vdso_standalone_test_x86 and build getrandom and chacha tests all
the time just like other vDSO selftests and return SKIP when the
functionality to be tested is not implemented.
This has the advantage of doing architecture specific selection at
only one place.
Also change vdso_test_getrandom to return SKIP instead of FAIL when
vDSO function is not found, just like vdso_test_getcpu or
vdso_test_gettimeofday.
Signed-off-by: Christophe Leroy
---
Based on latest random tree (0dfed8092247)
tools/arch/x86/vdso | 1 -
tools/testing/selftests/vDSO/Makefile | 10 --
tools/testing/selftests/vDSO/vdso_config.h | 3 +++
tools/testing/selftests/vDSO/vdso_test_chacha-asm.S | 7 +++
tools/testing/selftests/vDSO/vdso_test_chacha.c | 11 +++
tools/testing/selftests/vDSO/vdso_test_getrandom.c | 2 +-
6 files changed, 26 insertions(+), 8 deletions(-)
delete mode 12 tools/arch/x86/vdso
create mode 100644 tools/testing/selftests/vDSO/vdso_test_chacha-asm.S
diff --git a/tools/arch/x86/vdso b/tools/arch/x86/vdso
deleted file mode 12
index 7eb962fd3454..
--- a/tools/arch/x86/vdso
+++ /dev/null
@@ -1 +0,0 @@
-../../../arch/x86/entry/vdso/
\ No newline at end of file
diff --git a/tools/testing/selftests/vDSO/Makefile
b/tools/testing/selftests/vDSO/Makefile
index 5ead6b1f0478..cfb7c281b22c 100644
--- a/tools/testing/selftests/vDSO/Makefile
+++ b/tools/testing/selftests/vDSO/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
-ARCH ?= $(shell uname -m | sed -e s/i.86/x86/)
-SRCARCH := $(subst x86_64,x86,$(ARCH))
+uname_M := $(shell uname -m 2>/dev/null || echo not)
+ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)
TEST_GEN_PROGS := vdso_test_gettimeofday
TEST_GEN_PROGS += vdso_test_getcpu
@@ -10,10 +10,8 @@ ifeq ($(ARCH),$(filter $(ARCH),x86 x86_64))
TEST_GEN_PROGS += vdso_standalone_test_x86
endif
TEST_GEN_PROGS += vdso_test_correctness
-ifeq ($(ARCH),$(filter $(ARCH),x86_64))
TEST_GEN_PROGS += vdso_test_getrandom
TEST_GEN_PROGS += vdso_test_chacha
-endif
CFLAGS := -std=gnu99
@@ -38,8 +36,8 @@ $(OUTPUT)/vdso_test_getrandom: CFLAGS += -isystem
$(top_srcdir)/tools/include \
$(KHDR_INCLUDES) \
-isystem $(top_srcdir)/include/uapi
-$(OUTPUT)/vdso_test_chacha:
$(top_srcdir)/tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S
+$(OUTPUT)/vdso_test_chacha: vdso_test_chacha-asm.S
$(OUTPUT)/vdso_test_chacha: CFLAGS += -idirafter $(top_srcdir)/tools/include \
- -idirafter
$(top_srcdir)/arch/$(SRCARCH)/include \
+ -idirafter
$(top_srcdir)/arch/$(ARCH)/include \
-idirafter $(top_srcdir)/include \
-D__ASSEMBLY__ -Wa,--noexecstack
diff --git a/tools/testing/selftests/vDSO/vdso_config.h
b/tools/testing/selftests/vDSO/vdso_config.h
index 740ce8c98d2e..693920471160 100644
--- a/tools/testing/selftests/vDSO/vdso_config.h
+++ b/tools/testing/selftests/vDSO/vdso_config.h
@@ -47,6 +47,7 @@
#elif defined(__x86_64__)
#define VDSO_VERSION 0
#define VDSO_NAMES 1
+#define VDSO_GETRANDOM
"../../../../arch/x86/entry/vdso/vgetrandom-chacha.S"
#elif defined(__riscv__) || defined(__riscv)
#define VDSO_VERSION 5
#define VDSO_NAMES 1
@@ -58,6 +59,7 @@
#define VDSO_NAMES 1
#endif
+#ifndef __ASSEMBLY__
static const char *versions[7] = {
"LINUX_2.6",
"LINUX_2.6.15",
@@ -88,5 +90,6 @@ static const char *names[2][7] = {
"__vdso_getrandom",
},
};
+#endif
#endif /* __VDSO_CONFIG_H__ */
diff --git a/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S
b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S
new file mode 100644
index ..8e704165f6f2
--- /dev/null
+++ b/tools/testing/selftests/vDSO/vdso_test_chacha-asm.S
@@ -0,0 +1,7 @@
+#include "vdso_config.h"
+
+#ifdef VDSO_GETRANDOM
+
+#include VDSO_GETRAND
Re: [PATCH bpf-next v4 5/8] libbpf: Support opening bpf objects of either endianness
On Fri, Aug 30, 2024 at 02:25:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar
> wrote:
> >
> > Allow bpf_object__open() to access files of either endianness, and convert
> > included BPF programs to native byte-order in-memory for introspection.
> > Loading BPF objects of non-native byte-order is still disallowed however.
> >
> > Signed-off-by: Tony Ambardar
> > ---
> > tools/lib/bpf/libbpf.c | 49 +++--
> > tools/lib/bpf/libbpf_internal.h | 11
> > 2 files changed, 52 insertions(+), 8 deletions(-)
> >
>
> [...]
>
> >
> > + /* Validate ELF object endianness... */
> > + if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB &&
> > + ehdr->e_ident[EI_DATA] != ELFDATA2MSB) {
> > + err = -LIBBPF_ERRNO__ENDIAN;
> > + pr_warn("elf: '%s' has unknown byte order\n", obj->path);
> > + goto errout;
> > + }
> > + /* and preserve outside lifetime of bpf_object_open() */
>
> what does it mean "preserve outside lifetime" ?
bpf_object_open() freed ELF data on exit but didn't zero obj->efile.ehdr,
leading to unpredictable use-after-free problems in is_native_endianness().
This is part of the fix but should be clearer e.g. "save after ELF data
freed...". Will update.
>
> > + obj->byteorder = ehdr->e_ident[EI_DATA];
> > +
> > +
> > +
>
> why so many empty lines?..
I'm blind? Fixed, thanks.
>
> > if (elf_getshdrstrndx(elf, &obj->efile.shstrndx)) {
> > pr_warn("elf: failed to get section names section index for
> > %s: %s\n",
> > obj->path, elf_errmsg(-1));
>
> [...]
>
> > err = bpf_object__elf_init(obj);
> > - err = err ? : bpf_object__check_endianness(obj);
> > err = err ? : bpf_object__elf_collect(obj);
> > err = err ? : bpf_object__collect_externs(obj);
> > err = err ? : bpf_object_fixup_btf(obj);
> > @@ -8500,6 +8529,10 @@ static int bpf_object_load(struct bpf_object *obj,
> > int extra_log_level, const ch
> >
> > if (obj->gen_loader)
> > bpf_gen__init(obj->gen_loader, extra_log_level,
> > obj->nr_programs, obj->nr_maps);
>
> nit: add {} around if, both sides should either have or not have {}
>
OK, done.
> > + else if (!is_native_endianness(obj)) {
> > + pr_warn("object '%s' is not native endianness\n",
> > obj->name);
>
> "object '%s': load is not supported in non-native endianness\n"
Clearer, will update.
>
>
> > + return libbpf_err(-LIBBPF_ERRNO__ENDIAN);
> > + }
> >
> > err = bpf_object_prepare_token(obj);
> > err = err ? : bpf_object__probe_loading(obj);
>
> [...]
Re: [PATCH 2/2] selftests: add selftest for tcp SO_PEEK_OFF support
Hello Jon,
On Tue, Aug 27, 2024 at 3:58 AM Jon Maloy wrote:
>
>
>
> On 2024-08-23 19:44, Jason Xing wrote:
> > Hello Jon,
> >
> > On Sat, Aug 24, 2024 at 5:19 AM wrote:
> >> From: Jon Maloy
> >>
> >> We add a selftest to check that the new feature added in
> >> commit 05ea491641d3 ("tcp: add support for SO_PEEK_OFF socket option")
> >> works correctly.
> >>
> >> Reviewed-by: Stefano Brivio
> >> Tested-by: Stefano Brivio
> >> Signed-off-by: Jon Maloy
> > Thanks for working on this. Sorry that I just noticed I missed your
> > previous reply :(
> There is still the ditto UDP selftest to be done ;-)
The reason why I didn't respond at that time is because I was unsure
if I had enough time to finish it. Now it's time.
After digging into this, there will be a lot of duplicated code if I
write a new one named like "udp_so_peek_off". I think adjusting your
tcp_so_peek_off.c to complete the UDP part is just fine. Of course,
tcp_so_peek_off.c will be renamed :)
I will post one later to see if it's reasonable...
Thanks,
Jason
RE: [PATCH 07/16] powerpc: mm: Support MAP_BELOW_HINT
From: Christophe Leroy
> Sent: 28 August 2024 07:35
> Hi Charlie,
>
> Le 28/08/2024 à 07:49, Charlie Jenkins a écrit :
> > Add support for MAP_BELOW_HINT to arch_get_mmap_base() and
> > arch_get_mmap_end().
> >
> > Signed-off-by: Charlie Jenkins
> > ---
> > arch/powerpc/include/asm/task_size_64.h | 36
> > +++--
> > 1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/task_size_64.h
> > b/arch/powerpc/include/asm/task_size_64.h
> > index 239b363841aa..a37a5a81365d 100644
> > --- a/arch/powerpc/include/asm/task_size_64.h
> > +++ b/arch/powerpc/include/asm/task_size_64.h
> > @@ -72,12 +72,36 @@
> > #define STACK_TOP_MAX TASK_SIZE_USER64
> > #define STACK_TOP (is_32bit_task() ? STACK_TOP_USER32 : STACK_TOP_USER64)
> >
> > -#define arch_get_mmap_base(addr, len, base, flags) \
> > - (((addr) > DEFAULT_MAP_WINDOW) ? (base) + TASK_SIZE -
> > DEFAULT_MAP_WINDOW : (base))
> > +#define arch_get_mmap_base(addr, len, base, flags)
> > \
>
> This macro looks quite big for a macro, can it be a static inline
> function instead ? Same for the other macro below.
Or even a real function?
Given the actual cost of mapping memory an extra function call
isn't going to be masurable.
David
>
> > +({
> > \
> > + unsigned long mmap_base;
> > \
> > + typeof(flags) _flags = (flags);
> > \
> > + typeof(addr) _addr = (addr);
> > \
> > + typeof(base) _base = (base);
> > \
> > + typeof(len) _len = (len);
> > \
> > + unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);
> > \
> > + if (_flags & MAP_BELOW_HINT && _addr != 0 && ((_addr + _len) >
> > BIT(VA_BITS - 1)))\
> > + mmap_base = (_addr + _len) - rnd_gap;
> > \
> > + else
> > \
> > + mmap_end = ((_addr > DEFAULT_MAP_WINDOW) ?
> > \
> > + _base + TASK_SIZE - DEFAULT_MAP_WINDOW :
> > \
> > + _base);
> > \
> > + mmap_end;
> > \
>
> mmap_end doesn't exist, did you mean mmap_base ?
>
> > +})
> >
> > -#define arch_get_mmap_end(addr, len, flags) \
> > - (((addr) > DEFAULT_MAP_WINDOW) || \
> > -(((flags) & MAP_FIXED) && ((addr) + (len) > DEFAULT_MAP_WINDOW)) ?
> > TASK_SIZE : \
> > -
> > DEFAULT_MAP_WINDOW)
> > +#define arch_get_mmap_end(addr, len, flags)
> > \
> > +({
> > \
> > + unsigned long mmap_end;
> > \
> > + typeof(flags) _flags = (flags);
> > \
> > + typeof(addr) _addr = (addr);
> > \
> > + typeof(len) _len = (len);
> > \
> > + if (_flags & MAP_BELOW_HINT && _addr != 0 && ((_addr + _len) >
> > BIT(VA_BITS - 1))) \
> > + mmap_end = (_addr + _len);
> > \
> > + else
> > \
> > + mmap_end = (((_addr) > DEFAULT_MAP_WINDOW) ||
> > \
> > + (((_flags) & MAP_FIXED) && ((_addr) + (_len) >
> > DEFAULT_MAP_WINDOW))\
> > + ? TASK_SIZE : DEFAULT_MAP_WINDOW)
> > \
> > + mmap_end;
> > \
> > +})
> >
> > #endif /* _ASM_POWERPC_TASK_SIZE_64_H */
> >
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH bpf-next v4 5/8] libbpf: Support opening bpf objects of either endianness
On Fri, Aug 30, 2024 at 06:16:25PM -0700, Eduard Zingerman wrote:
> On Fri, 2024-08-30 at 00:29 -0700, Tony Ambardar wrote:
>
> [...]
>
> > @@ -940,6 +942,21 @@ bpf_object__add_programs(struct bpf_object *obj,
> > Elf_Data *sec_data,
> > return 0;
> > }
> >
> > +static void bpf_object_bswap_progs(struct bpf_object *obj)
> > +{
> > + struct bpf_program *prog = obj->programs;
> > + struct bpf_insn *insn;
> > + int p, i;
> > +
> > + for (p = 0; p < obj->nr_programs; p++, prog++) {
> > + insn = prog->insns;
> > + for (i = 0; i < prog->insns_cnt; i++, insn++)
> > + bpf_insn_bswap(insn);
> > + pr_debug("prog '%s': converted %zu insns to native byte
> > order\n",
> > +prog->name, prog->insns_cnt);
>
> Nit: pr_debug already printed available programs at this point,
> maybe move this call outside of both loops?
>
Good point. Will update to summarize # of programs converted instead.
> > + }
> > +}
> > +
> > static const struct btf_member *
> > find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
> > {
>
> [...]
>
Re: [PATCH v2 02/19] iommufd/viommu: Add IOMMUFD_OBJ_VIOMMU and IOMMU_VIOMMU_ALLOC ioctl
On 2024/8/28 0:59, Nicolin Chen wrote:
+int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+ struct iommu_viommu_alloc *cmd = ucmd->cmd;
+ struct iommufd_hwpt_paging *hwpt_paging;
+ struct iommufd_viommu *viommu;
+ struct iommufd_device *idev;
+ int rc;
+
+ if (cmd->flags)
+ return -EOPNOTSUPP;
+
+ idev = iommufd_get_device(ucmd, cmd->dev_id);
Why does a device reference count is needed here? When is this reference
count released after the VIOMMU is allocated?
+ if (IS_ERR(idev))
+ return PTR_ERR(idev);
+
+ hwpt_paging = iommufd_get_hwpt_paging(ucmd, cmd->hwpt_id);
+ if (IS_ERR(hwpt_paging)) {
+ rc = PTR_ERR(hwpt_paging);
+ goto out_put_idev;
+ }
+
+ if (!hwpt_paging->nest_parent) {
+ rc = -EINVAL;
+ goto out_put_hwpt;
+ }
+
+ if (cmd->type != IOMMU_VIOMMU_TYPE_DEFAULT) {
+ rc = -EOPNOTSUPP;
+ goto out_put_hwpt;
+ }
+
+ viommu = iommufd_object_alloc(ucmd->ictx, viommu, IOMMUFD_OBJ_VIOMMU);
+ if (IS_ERR(viommu)) {
+ rc = PTR_ERR(viommu);
+ goto out_put_hwpt;
+ }
+
+ viommu->type = cmd->type;
+ viommu->ictx = ucmd->ictx;
+ viommu->hwpt = hwpt_paging;
+
+ refcount_inc(&viommu->hwpt->common.obj.users);
+
+ cmd->out_viommu_id = viommu->obj.id;
+ rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+ if (rc)
+ goto out_abort;
+ iommufd_object_finalize(ucmd->ictx, &viommu->obj);
+ goto out_put_hwpt;
+
+out_abort:
+ iommufd_object_abort_and_destroy(ucmd->ictx, &viommu->obj);
+out_put_hwpt:
+ iommufd_put_object(ucmd->ictx, &hwpt_paging->common.obj);
+out_put_idev:
+ iommufd_put_object(ucmd->ictx, &idev->obj);
+ return rc;
+}
Thanks,
baolu
Re: [PATCH bpf-next v4 7/8] libbpf: Support creating light skeleton of either endianness
On Fri, Aug 30, 2024 at 02:30:46PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar
> wrote:
> >
> > Track target endianness in 'struct bpf_gen' and process in-memory data in
> > native byte-order, but on finalization convert the embedded loader BPF
> > insns to target endianness.
> >
> > The light skeleton also includes a target-accessed data blob which is
> > heterogeneous and thus difficult to convert to target byte-order on
> > finalization. Add support functions to convert data to target endianness
> > as it is added to the blob.
> >
> > Also add additional debug logging for data blob structure details and
> > skeleton loading.
> >
> > Signed-off-by: Tony Ambardar
> > ---
> > tools/lib/bpf/bpf_gen_internal.h | 1 +
> > tools/lib/bpf/gen_loader.c | 187 +++
> > tools/lib/bpf/libbpf.c | 1 +
> > tools/lib/bpf/skel_internal.h| 3 +-
> > 4 files changed, 147 insertions(+), 45 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_gen_internal.h
> > b/tools/lib/bpf/bpf_gen_internal.h
> > index fdf44403ff36..6ff963a491d9 100644
> > --- a/tools/lib/bpf/bpf_gen_internal.h
> > +++ b/tools/lib/bpf/bpf_gen_internal.h
> > @@ -34,6 +34,7 @@ struct bpf_gen {
> > void *data_cur;
> > void *insn_start;
> > void *insn_cur;
> > + bool swapped_endian;
> > ssize_t cleanup_label;
> > __u32 nr_progs;
> > __u32 nr_maps;
> > diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
> > index cf3323fd47b8..4374399bc3f8 100644
> > --- a/tools/lib/bpf/gen_loader.c
> > +++ b/tools/lib/bpf/gen_loader.c
> > @@ -401,6 +401,15 @@ int bpf_gen__finish(struct bpf_gen *gen, int nr_progs,
> > int nr_maps)
> > opts->insns_sz = gen->insn_cur - gen->insn_start;
> > opts->data = gen->data_start;
> > opts->data_sz = gen->data_cur - gen->data_start;
> > +
> > + /* use target endianness for embedded loader */
> > + if (gen->swapped_endian) {
> > + struct bpf_insn *insn = (struct bpf_insn
> > *)opts->insns;
> > + int insn_cnt = opts->insns_sz / sizeof(struct
> > bpf_insn);
> > +
> > + for (i = 0; i < insn_cnt; i++)
> > + bpf_insn_bswap(insn++);
> > + }
> > }
> > return gen->error;
> > }
> > @@ -414,6 +423,31 @@ void bpf_gen__free(struct bpf_gen *gen)
> > free(gen);
> > }
> >
> > +/*
> > + * Fields of bpf_attr are set to values in native byte-order before being
> > + * written to the target-bound data blob, and may need endian conversion.
> > + * This macro allows providing the correct value in situ more simply than
> > + * writing a separate converter for *all fields* of *all records* included
> > + * in union bpf_attr. Note that sizeof(rval) should match the assignment
> > + * target to avoid runtime problems.
> > + */
> > +#define tgt_endian(rval) ({\
> > + typeof(rval) _val; \
> > + if (!gen->swapped_endian) \
>
> if/else has to have balanced branches w.r.t. {}. Either both should
> have it or both shouldn't. In this case both should have it.
>
> > + _val = (rval); \
> > + else { \
> > + switch (sizeof(rval)) { \
> > + case 1: _val = (rval); break; \
> > + case 2: _val = bswap_16(rval); break; \
> > + case 4: _val = bswap_32(rval); break; \
> > + case 8: _val = bswap_64(rval); break; \
> > + default:_val = (rval); \
> > + pr_warn("unsupported bswap size!\n"); \
>
> this is a weird formatting, but you can also just unconditionally
> assign _val, and only swap it if gen->swapped_endian
>
> typeof(rval) _val = (rval);
>
> if (gen->swapped_endian) {
> switch (...) {
> case 1: ...
> ...
> case 8: ...
> default: pr_warn("...");
> }
> }
>
> _val;
>
>
> seems simpler and cleaner, imo
>
Yes, agreed. Will update.
> > + } \
> > + } \
> > + _val; \
> > +})
> > +
>
>
> for the rest, Alexei, can you please review and give your ack?
Re: [PATCH net-next] wireguard: allowedips: Add WGALLOWEDIP_F_REMOVE_ME flag
Hi Jordan, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Jordan-Rife/wireguard-allowedips-Add-WGALLOWEDIP_F_REMOVE_ME-flag/20240831-034712 base: net-next/main patch link: https://lore.kernel.org/r/20240830194103.2186774-1-jrife%40google.com patch subject: [PATCH net-next] wireguard: allowedips: Add WGALLOWEDIP_F_REMOVE_ME flag config: i386-randconfig-062-20240901 (https://download.01.org/0day-ci/archive/20240901/202409011256.cwrnquxq-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409011256.cwrnquxq-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202409011256.cwrnquxq-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/wireguard/allowedips.c:257:24: sparse: sparse: incorrect type in >> argument 1 (different address spaces) @@ expected struct list_head >> *entry @@ got struct list_head [noderef] __rcu * @@ drivers/net/wireguard/allowedips.c:257:24: sparse: expected struct list_head *entry drivers/net/wireguard/allowedips.c:257:24: sparse: got struct list_head [noderef] __rcu * >> drivers/net/wireguard/allowedips.c:258:9: sparse: sparse: cast removes >> address space '__rcu' of expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:269:24: sparse: sparse: cast removes address space '__rcu' of expression drivers/net/wireguard/allowedips.c:270:26: sparse: sparse: cast removes address space '__rcu' of expression >> drivers/net/wireguard/allowedips.c:276:19: sparse: sparse: incorrect type in >> argument 1 (different address spaces) @@ expected struct callback_head >> *head @@ got struct callback_head [noderef] __rcu * @@ drivers/net/wireguard/allowedips.c:276:19: sparse: expected struct callback_head *head drivers/net/wireguard/allowedips.c:276:19: sparse: got struct callback_head [noderef] __rcu * drivers/net/wireguard/allowedips.c:294:18: sparse: sparse: incompatible types in comparison expression (different address spaces): drivers/net/wireguard/allowedips.c:294:18: sparse:struct wg_peer * drivers/net/wireguard/allowedips.c:294:18: sparse:struct wg_peer [noderef] __rcu * >> drivers/net/wireguard/allowedips.c:384:25: sparse: sparse: incorrect type in >> argument 1 (different address spaces) @@ expected struct allowedips_node >> [noderef] __rcu *node @@ got struct allowedips_node *[assigned] node @@ >> drivers/net/wireguard/allowedips.c:258:9: sparse: sparse: dereference of >> noderef expression >> drivers/net/wireguard/allowedips.c:258:9: sparse: sparse: dereference of >> noderef expression drivers/net/wireguard/allowedips.c:259:22: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:259:38: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:261:17: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:264:44: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:265:50: sparse: sparse: dereference of noderef expression drivers/net/wireguard/allowedips.c:268:
Re: [PATCH bpf-next v4 5/8] libbpf: Support opening bpf objects of either endianness
On Fri, Aug 30, 2024 at 06:26:10PM -0700, Eduard Zingerman wrote:
> On Fri, 2024-08-30 at 14:25 -0700, Andrii Nakryiko wrote:
>
> [...]
>
> > > err = bpf_object__elf_init(obj);
> > > - err = err ? : bpf_object__check_endianness(obj);
> > > err = err ? : bpf_object__elf_collect(obj);
> > > err = err ? : bpf_object__collect_externs(obj);
> > > err = err ? : bpf_object_fixup_btf(obj);
> > > @@ -8500,6 +8529,10 @@ static int bpf_object_load(struct bpf_object *obj,
> > > int extra_log_level, const ch
> > >
> > > if (obj->gen_loader)
> > > bpf_gen__init(obj->gen_loader, extra_log_level,
> > > obj->nr_programs, obj->nr_maps);
> >
> > nit: add {} around if, both sides should either have or not have {}
> >
> > > + else if (!is_native_endianness(obj)) {
> > > + pr_warn("object '%s' is not native endianness\n",
> > > obj->name);
> >
> > "object '%s': load is not supported in non-native endianness\n"
> >
> >
> > > + return libbpf_err(-LIBBPF_ERRNO__ENDIAN);
> > > + }
>
> Silly question:
> why load is allowed to proceed for non-native endianness when
> obj->gen_loader is set?
>
Not silly, had similar questions. Having obj->gen_loader set means "light
skeleton" is being generated, where it tries to eliminate dependency on
libbpf by skeleton code. In this mode, the code doesn't load anything but
instead tracks "what would libbpf do" so it can later write a pure BPF
loader program. Alexei will correct me or elaborate as needed I hope.
Unconditionally blocking on non-native endianness would break light skel.
Re: [PATCH v1 1/2] mseal: fix mmap(FIXED) error code.
On Fri, Aug 30, 2024 at 06:15:46PM GMT, Andrew Morton wrote: > On Thu, 29 Aug 2024 13:09:41 +0100 Lorenzo Stoakes > wrote: > > > Relevant section from MAINTAINERS: > > > > MEMORY MAPPING > > I always thought it meant "memory management" ;) Ha ha no, I leave the managing to you, we just map and pray! ;)
Re: [PATCH bpf-next v4 6/8] libbpf: Support linking bpf objects of either endianness
On Fri, Aug 30, 2024 at 02:25:07PM -0700, Andrii Nakryiko wrote:
> On Fri, Aug 30, 2024 at 12:30 AM Tony Ambardar
> wrote:
> >
> > Allow static linking object files of either endianness, checking that input
> > files have consistent byte-order, and setting output endianness from input.
> >
> > Linking requires in-memory processing of programs, relocations, sections,
> > etc. in native endianness, and output conversion to target byte-order. This
> > is enabled by built-in ELF translation and recent BTF/BTF.ext endianness
> > functions. Further add local functions for swapping byte-order of sections
> > containing BPF insns.
> >
> > Signed-off-by: Tony Ambardar
> > ---
> > tools/lib/bpf/linker.c | 90 ++
> > 1 file changed, 74 insertions(+), 16 deletions(-)
> >
>
> [...]
>
> >
> > +static bool is_exec_sec(struct dst_sec *sec)
> > +{
> > + if (!sec || sec->ephemeral)
> > + return false;
> > + return (sec->shdr->sh_type == SHT_PROGBITS) &&
> > + (sec->shdr->sh_flags & SHF_EXECINSTR);
> > +}
> > +
> > +static int exec_sec_bswap(void *raw_data, int size)
> > +{
> > + const int insn_cnt = size / sizeof(struct bpf_insn);
> > + struct bpf_insn *insn = raw_data;
> > + int i;
> > +
> > + if (size % sizeof(struct bpf_insn))
> > + return -EINVAL;
>
> this shouldn't be checked here, it should be assumed this is valid and
> was ensured by the caller. And make exec_sec_bswap() a void function,
> please.
>
> > + for (i = 0; i < insn_cnt; i++, insn++)
> > + bpf_insn_bswap(insn);
> > + return 0;
> > +}
> > +
> > static int extend_sec(struct bpf_linker *linker, struct dst_sec *dst,
> > struct src_sec *src)
> > {
> > void *tmp;
> > @@ -1170,6 +1203,10 @@ static int extend_sec(struct bpf_linker *linker,
> > struct dst_sec *dst, struct src
> > memset(dst->raw_data + dst->sec_sz, 0, dst_align_sz -
> > dst->sec_sz);
> > /* now copy src data at a properly aligned offset */
> > memcpy(dst->raw_data + dst_align_sz, src->data->d_buf,
> > src->shdr->sh_size);
> > +
>
> the check for size % sizeof(struct bpf_insn) should be somewhere here
> (if is_exec_sec()), right?
>
> > + /* convert added bpf insns to native byte-order */
> > + if (linker->swapped_endian && is_exec_sec(dst))
> > + exec_sec_bswap(dst->raw_data + dst_align_sz,
> > src->shdr->sh_size);
> > }
> >
> > dst->sec_sz = dst_final_sz;
> > @@ -2630,6 +2667,10 @@ int bpf_linker__finalize(struct bpf_linker *linker)
> > if (!sec->scn)
> > continue;
> >
>
> but no need to check here, we know it's correct, if we got all the way here
>
I did a pass earlier to reorganize sanity checks and remove redundant
ones, and realized linker_sanity_check_elf() already does what we want
but overlooked dropping this from exec_sec_bswap(). Will do so now. Thanks
for catching!
> > + /* restore sections with bpf insns to target byte-order */
> > + if (linker->swapped_endian && is_exec_sec(sec))
> > + exec_sec_bswap(sec->raw_data, sec->sec_sz);
> > +
> > sec->data->d_buf = sec->raw_data;
> > }
> >
> > @@ -2698,6 +2739,7 @@ static int emit_elf_data_sec(struct bpf_linker
> > *linker, const char *sec_name,
> >
> > static int finalize_btf(struct bpf_linker *linker)
> > {
> > + enum btf_endianness link_endianness;
> > LIBBPF_OPTS(btf_dedup_opts, opts);
> > struct btf *btf = linker->btf;
> > const void *raw_data;
> > @@ -2742,6 +2784,22 @@ static int finalize_btf(struct bpf_linker *linker)
> > return err;
> > }
> >
> > + /* Set .BTF and .BTF.ext output byte order */
> > + link_endianness = linker->elf_hdr->e_ident[EI_DATA] == ELFDATA2MSB ?
> > + BTF_BIG_ENDIAN : BTF_LITTLE_ENDIAN;
> > + err = btf__set_endianness(linker->btf, link_endianness);
> > + if (err) {
> > + pr_warn("failed to set .BTF output endianness: %d\n", err);
> > + return err;
> > + }
>
> link_endianness is always well-formed enum, there is no need to check
> errors, here and for btf_ext__set_endianness, please drop both
Right, makes sense.
>
> > + if (linker->btf_ext) {
> > + err = btf_ext__set_endianness(linker->btf_ext,
> > link_endianness);
> > + if (err) {
> > + pr_warn("failed to set .BTF.ext output endianness:
> > %d\n", err);
> > + return err;
> > + }
> > + }
> > +
> > /* Emit .BTF section */
> > raw_data = btf__raw_data(linker->btf, &raw_sz);
> > if (!raw_data)
> > --
> > 2.34.1
> >
Re: [PATCH] selftests/futex: Create test for robust list
Hi André,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/locking/core]
[also build test ERROR on linus/master v6.11-rc5 next-20240830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/selftests-futex-Create-test-for-robust-list/20240830-023631
base: tip/locking/core
patch link:
https://lore.kernel.org/r/20240829183509.446934-1-andrealmeid%40igalia.com
patch subject: [PATCH] selftests/futex: Create test for robust list
:: branch date: 2 days ago
:: commit date: 2 days ago
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project
617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240831/202408311654.me6a2xuf-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes: https://lore.kernel.org/r/202408311654.me6a2xuf-...@intel.com/
All errors (new ones prefixed by >>):
>> robust_list.c:53:17: error: use of undeclared identifier 'SYS_futex_wait';
>> did you mean 'futex_wait'?
53 | return syscall(SYS_futex_wait, futex, val, ~0U,
FUTEX2_SIZE_U32, timo, CLOCK_MONOTONIC);
|^~
|futex_wait
../include/futextest.h:78:1: note: 'futex_wait' declared here
78 | futex_wait(futex_t *uaddr, futex_t val, struct timespec *timeout,
int opflags)
| ^
>> robust_list.c:121:6: error: address argument to atomic operation must be a
>> pointer to _Atomic type ('int *' invalid)
121 | if (atomic_compare_exchange_strong(futex, &zero, tid)) {
| ^ ~
/opt/cross/clang-617a15a9ea/lib/clang/18/include/stdatomic.h:144:67: note:
expanded from macro 'atomic_compare_exchange_strong'
144 | #define atomic_compare_exchange_strong(object, expected, desired)
__c11_atomic_compare_exchange_strong(object, expected, desired,
__ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)
| ^
~~
robust_list.c:147:9: error: address argument to atomic operation must be a
pointer to _Atomic type ('int *' invalid)
147 | tid = atomic_load(futex);
| ^ ~
/opt/cross/clang-617a15a9ea/lib/clang/18/include/stdatomic.h:138:29: note:
expanded from macro 'atomic_load'
138 | #define atomic_load(object) __c11_atomic_load(object,
__ATOMIC_SEQ_CST)
| ^ ~~
robust_list.c:150:3: error: address argument to atomic operation must be a
pointer to _Atomic type ('int *' invalid)
150 | atomic_store(futex, tid);
| ^~
/opt/cross/clang-617a15a9ea/lib/clang/18/include/stdatomic.h:135:39: note:
expanded from macro 'atomic_store'
135 | #define atomic_store(object, desired) __c11_atomic_store(object,
desired, __ATOMIC_SEQ_CST)
| ^ ~~
4 errors generated.
vim +53 tools/testing/selftests/futex/functional/robust_list.c
3306ef10715f1b André Almeida 2024-08-29 50
3306ef10715f1b André Almeida 2024-08-29 51 int futex2_wait(void *futex, int
val, struct timespec *timo)
3306ef10715f1b André Almeida 2024-08-29 52 {
3306ef10715f1b André Almeida 2024-08-29 @53 return syscall(SYS_futex_wait,
futex, val, ~0U, FUTEX2_SIZE_U32, timo, CLOCK_MONOTONIC);
3306ef10715f1b André Almeida 2024-08-29 54 }
3306ef10715f1b André Almeida 2024-08-29 55
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
