Andrii Nakryiko [Tue, 6 Aug 2024 22:38:12 +0000 (15:38 -0700)]
security,bpf: constify struct path in bpf_token_create() LSM hook
There is no reason why struct path pointer shouldn't be const-qualified
when being passed into bpf_token_create() LSM hook. Add that const.
Acked-by: Paul Moore <paul@paul-moore.com> (LSM/SELinux) Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Al Viro [Tue, 30 Jul 2024 05:16:04 +0000 (01:16 -0400)]
bpf: switch maps to CLASS(fd, ...)
Calling conventions for __bpf_map_get() would be more convenient
if it left fpdut() on failure to callers. Makes for simpler logics
in the callers.
Among other things, the proof of memory safety no longer has to
rely upon file->private_data never being ERR_PTR(...) for bpffs files.
Original calling conventions made it impossible for the caller to tell
whether __bpf_map_get() has returned ERR_PTR(-EINVAL) because it has found
the file not be a bpf map one (in which case it would've done fdput())
or because it found that ERR_PTR(-EINVAL) in file->private_data of a
bpf map file (in which case fdput() would _not_ have been done).
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Reviewed-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Andrii Nakryiko [Tue, 6 Aug 2024 21:31:34 +0000 (14:31 -0700)]
bpf: factor out fetching bpf_map from FD and adding it to used_maps list
Factor out the logic to extract bpf_map instances from FD embedded in
bpf_insns, adding it to the list of used_maps (unless it's already
there, in which case we just reuse map's index). This simplifies the
logic in resolve_pseudo_ldimm64(), especially around `struct fd`
handling, as all that is now neatly contained in the helper and doesn't
leak into a dozen error handling paths.
Al Viro [Fri, 31 May 2024 20:34:25 +0000 (16:34 -0400)]
add struct fd constructors, get rid of __to_fd()
Make __fdget() et.al. return struct fd directly.
New helpers: BORROWED_FD(file) and CLONED_FD(file), for
borrowed and cloned file references resp.
NOTE: this might need tuning; in particular, inline on
__fget_light() is there to keep the code generation same as
before - we probably want to keep it inlined in fdget() et.al.
(especially so in fdget_pos()), but that needs profiling.
Reviewed-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Al Viro [Fri, 31 May 2024 19:45:12 +0000 (15:45 -0400)]
struct fd: representation change
We want the compiler to see that fdput() on empty instance
is a no-op. The emptiness check is that file reference is NULL,
while fdput() is "fput() if FDPUT_FPUT is present in flags".
The reason why fdput() on empty instance is a no-op is something
compiler can't see - it's that we never generate instances with
NULL file reference combined with non-zero flags.
It's not that hard to deal with - the real primitives behind
fdget() et.al. are returning an unsigned long value, unpacked by (inlined)
__to_fd() into the current struct file * + int. The lower bits are
used to store flags, while the rest encodes the pointer. Linus suggested
that keeping this unsigned long around with the extractions done by inlined
accessors should generate a sane code and that turns out to be the case.
Namely, turning struct fd into a struct-wrapped unsinged long, with
fd_empty(f) => unlikely(f.word == 0)
fd_file(f) => (struct file *)(f.word & ~3)
fdput(f) => if (f.word & 1) fput(fd_file(f))
ends up with compiler doing the right thing. The cost is the patch
footprint, of course - we need to switch f.file to fd_file(f) all over
the tree, and it's not doable with simple search and replace; there are
false positives, etc.
Note that the sole member of that structure is an opaque
unsigned long - all accesses should be done via wrappers and I don't
want to use a name that would invite manual casts to file pointers,
etc. The value of that member is equal either to (unsigned long)p | flags,
p being an address of some struct file instance, or to 0 for an empty fd.
For now the new predicate (fd_empty(f)) has no users; all the
existing checks have form (!fd_file(f)). We will convert to fd_empty()
use later; here we only define it (and tell the compiler that it's
unlikely to return true).
This commit only deals with representation change; there will
be followups.
Reviewed-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Al Viro [Fri, 31 May 2024 18:12:01 +0000 (14:12 -0400)]
introduce fd_file(), convert all accessors to it.
For any changes of struct fd representation we need to
turn existing accesses to fields into calls of wrappers.
Accesses to struct fd::flags are very few (3 in linux/file.h,
1 in net/socket.c, 3 in fs/overlayfs/file.c and 3 more in
explicit initializers).
Those can be dealt with in the commit converting to
new layout; accesses to struct fd::file are too many for that.
This commit converts (almost) all of f.file to
fd_file(f). It's not entirely mechanical ('file' is used as
a member name more than just in struct fd) and it does not
even attempt to distinguish the uses in pointer context from
those in boolean context; the latter will be eventually turned
into a separate helper (fd_empty()).
NOTE: mass conversion to fd_empty(), tempting as it
might be, is a bad idea; better do that piecewise in commit
that convert from fdget...() to CLASS(...).
[conflicts in fs/fhandle.c, kernel/bpf/syscall.c, mm/memcontrol.c
caught by git; fs/stat.c one got caught by git grep]
[fs/xattr.c conflict]
Reviewed-by: Christian Brauner <brauner@kernel.org> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Song Liu [Tue, 6 Aug 2024 23:09:04 +0000 (16:09 -0700)]
selftests/bpf: Add tests for bpf_get_dentry_xattr
Add test for bpf_get_dentry_xattr on hook security_inode_getxattr.
Verify that the kfunc can read the xattr. Also test failing getxattr
from user space by returning non-zero from the LSM bpf program.
====================
bpf: introduce new VFS based BPF kfuncs
G'day!
A respin based off v3, which can be found here [0]. Original
motivations for introducing this suite of BPF kfuncs can be found here
[1].
The primary difference in this version of the patch series is that the
suite of VFS related BPF kfuncs added can be used from both sleepable
and non-sleepable BPF LSM program types. IOW, the KF_SLEEPABLE
annotation has been removed from all of them.
Changes sinve v3:
* KF_SLEEPABLE annotation has been dropped from all newly introduced
VFS related BPF kfuncs. This includes bpf_get_task_exe_file(),
bpf_put_file(), and bpf_path_d_path(). Both negative and positive
selftests backing these new BPF kfuncs have also been updated
accordingly.
* buf__sz conditional in bpf_path_d_path() has been updated from
buf__sz <= 0, to !buf__sz.
* Syntax issues as reported so here [2] have been corrected.
Matt Bobrowski [Wed, 31 Jul 2024 11:08:33 +0000 (11:08 +0000)]
selftests/bpf: add positive tests for new VFS based BPF kfuncs
Add a bunch of positive selftests which extensively cover the various
contexts and parameters in which the new VFS based BPF kfuncs may be
used from.
Again, the following VFS based BPF kfuncs are thoroughly tested within
this new selftest:
* struct file *bpf_get_task_exe_file(struct task_struct *);
* void bpf_put_file(struct file *);
* int bpf_path_d_path(struct path *, char *, size_t);
Matt Bobrowski [Wed, 31 Jul 2024 11:08:32 +0000 (11:08 +0000)]
selftests/bpf: add negative tests for new VFS based BPF kfuncs
Add a bunch of negative selftests responsible for asserting that the
BPF verifier successfully rejects a BPF program load when the
underlying BPF program misuses one of the newly introduced VFS based
BPF kfuncs.
The following VFS based BPF kfuncs are extensively tested within this
new selftest:
Matt Bobrowski [Wed, 31 Jul 2024 11:08:31 +0000 (11:08 +0000)]
bpf: introduce new VFS based BPF kfuncs
Add a new variant of bpf_d_path() named bpf_path_d_path() which takes
the form of a BPF kfunc and enforces KF_TRUSTED_ARGS semantics onto
its arguments.
This new d_path() based BPF kfunc variant is intended to address the
legacy bpf_d_path() BPF helper's susceptability to memory corruption
issues [0, 1, 2] by ensuring to only operate on supplied arguments
which are deemed trusted by the BPF verifier. Typically, this means
that only pointers to a struct path which have been referenced counted
may be supplied.
In addition to the new bpf_path_d_path() BPF kfunc, we also add a
KF_ACQUIRE based BPF kfunc bpf_get_task_exe_file() and KF_RELEASE
counterpart BPF kfunc bpf_put_file(). This is so that the new
bpf_path_d_path() BPF kfunc can be used more flexibily from within the
context of a BPF LSM program. It's rather common to ascertain the
backing executable file for the calling process by performing the
following walk current->mm->exe_file while instrumenting a given
operation from the context of the BPF LSM program. However, walking
current->mm->exe_file directly is never deemed to be OK, and doing so
from both inside and outside of BPF LSM program context should be
considered as a bug. Using bpf_get_task_exe_file() and in turn
bpf_put_file() will allow BPF LSM programs to reliably get and put
references to current->mm->exe_file.
As of now, all the newly introduced BPF kfuncs within this patch are
limited to BPF LSM program types. These can be either sleepable or
non-sleepable variants of BPF LSM program types.
Yonghong Song [Fri, 2 Aug 2024 18:54:34 +0000 (11:54 -0700)]
selftests/bpf: Fix a btf_dump selftest failure
Jakub reported bpf selftest "btf_dump" failure after forwarding to
v6.11-rc1 with netdev.
Error: #33 btf_dump
Error: #33/15 btf_dump/btf_dump: var_data
btf_dump_data:FAIL:find type id unexpected find type id: actual -2 < expected 0
The reason for the failure is due to
commit 94ede2a3e913 ("profiling: remove stale percpu flip buffer variables")
where percpu static variable "cpu_profile_flip" is removed.
Let us replace "cpu_profile_flip" with a variable in bpf subsystem
so whenever that variable gets deleted or renamed, we can detect the
failure immediately. In this case, I picked a static percpu variable
"bpf_cgrp_storage_busy" which is defined in kernel/bpf/bpf_cgrp_storage.c.
Martin KaFai Lau [Wed, 31 Jul 2024 17:00:20 +0000 (10:00 -0700)]
Merge branch 'selftests/bpf: convert test_dev_cgroup to test_progs'
Alexis Lothoré (eBPF Foundation) says:
====================
Hello,
this small series aims to integrate test_dev_cgroup in test_progs so it
could be run automatically in CI. The new version brings a few differences
with the current one:
- test now uses directly syscalls instead of wrapping commandline tools
into system() calls
- test_progs manipulates /dev/null (eg: redirecting test logs into it), so
disabling access to it in the bpf program confuses the tests. To fix this,
the first commit modifies the bpf program to allow access to char devices
1:3 (/dev/null), and disable access to char devices 1:5 (/dev/zero)
- once test is converted, add a small subtest to also check for device type
interpretation (char or block)
- paths used in mknod tests are now in /dev instead of /tmp: due to the CI
runner organisation and mountpoints manipulations, trying to create nodes
in /tmp leads to errors unrelated to the test (ie, mknod calls refused by
kernel, not the bpf program). I don't understand exactly the root cause
at the deepest point (all I see in CI is an -ENXIO error on mknod when trying to
create the node in tmp, and I can not make sense out of it neither
replicate it locally), so I would gladly take inputs from anyone more
educated than me about this.
The new test_progs part has been tested in a local qemu environment as well
as in upstream CI:
./test_progs -a cgroup_dev
47/1 cgroup_dev/allow-mknod:OK
47/2 cgroup_dev/allow-read:OK
47/3 cgroup_dev/allow-write:OK
47/4 cgroup_dev/deny-mknod:OK
47/5 cgroup_dev/deny-read:OK
47/6 cgroup_dev/deny-write:OK
47/7 cgroup_dev/deny-mknod-wrong-type:OK
47 cgroup_dev:OK
Summary: 1/7 PASSED, 0 SKIPPED, 0 FAILED
---
Changes in v4:
- Fix mixup between ret and errno by testing both
- Properly apply ack tag from Stanislas
- Link to v3: https://lore.kernel.org/r/20240730-convert_dev_cgroup-v3-0-93e573b74357@bootlin.com
Changes in v3:
- delete mknod file only if it has been created
- use bpf_program__attach_cgroup() instead of bpf_prog_attach
- reorganize subtests order
- collect review/ack tags from Alan and Stanislas
- Link to v2: https://lore.kernel.org/r/20240729-convert_dev_cgroup-v2-0-4c1fc0520545@bootlin.com
Changes in v2:
- directly pass expected ret code to subtests instead of boolean pass/not
pass
- fix faulty fd check in subtest expected to fail on open
- fix wrong subtest name
- pass test buffer and corresponding size to read/write subtests
- use correct series prefix
- Link to v1: https://lore.kernel.org/r/20240725-convert_dev_cgroup-v1-0-2c8cbd487c44@bootlin.com
====================
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
Current cgroup_dev test mostly tests that device operation is accepted or
refused base on passed major/minor (and so, any operation performed during
test involves only char device)
Add a small subtest ensuring that the device type passed to bpf program
allows it to take decisions as well.
selftests/bpf: convert test_dev_cgroup to test_progs
test_dev_cgroup is defined as a standalone test program, and so is not
executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and
remove the old test. In order to be able to run it in test_progs, /dev/null
must remain usable, so change the new test to test operations on devices
1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
selftests/bpf: do not disable /dev/null device access in cgroup dev test
test_dev_cgroup currently loads a small bpf program allowing any access on
urandom and zero devices, disabling access to any other device. It makes
migrating this test to test_progs impossible, since this one manipulates
extensively /dev/null.
Allow /dev/null manipulation in dev_cgroup program to make its usage in
test_progs framework possible. Update test_dev_cgroup.c as well to match
this change while it has not been removed.
Stanislav Fomichev [Fri, 26 Jul 2024 22:20:48 +0000 (15:20 -0700)]
xsk: Try to make xdp_umem_reg extension a bit more future-proof
We recently found out that extending xsk_umem_reg might be a bit
complicated due to not enforcing padding to be zero [0]. Add
a couple of things to make it less error-prone:
1. Remove xdp_umem_reg_v2 since its sizeof is the same as xdp_umem_reg
2. Add BUILD_BUG_ON that checks that the size of xdp_umem_reg_v1 is less
than xdp_umem_reg; presumably, when we get to v2, there is gonna
be a similar line to enforce that sizeof(v2) > sizeof(v1)
3. Add BUILD_BUG_ON to make sure the last field plus its size matches
the overall struct size. The intent is to demonstrate that we don't
have any lingering padding.
Reported-by: Julian Schindel <mail@arctic-alpaca.de> Cc: Magnus Karlsson <magnus.karlsson@gmail.com> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> Link: https://lore.kernel.org/r/20240726222048.1397869-1-sdf@fomichev.me Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
bpf: kprobe: Remove unused declaring of bpf_kprobe_override
After the commit 66665ad2f102 ("tracing/kprobe: bpf: Compare instruction
pointer with original one"), "bpf_kprobe_override" is not used anywhere
anymore, and we can remove it now.
Tony Ambardar [Mon, 29 Jul 2024 09:24:24 +0000 (02:24 -0700)]
selftests/bpf: Fix error compiling tc_redirect.c with musl libc
Linux 5.1 implemented 64-bit time types and related syscalls to address the
Y2038 problem generally across archs. Userspace handling of Y2038 varies
with the libc however. While musl libc uses 64-bit time across all 32-bit
and 64-bit platforms, GNU glibc uses 64-bit time on 64-bit platforms but
defaults to 32-bit time on 32-bit platforms unless they "opt-in" to 64-bit
time or explicitly use 64-bit syscalls and time structures.
One specific area is the standard setsockopt() call, SO_TIMESTAMPNS option
used for timestamping, and the related output 'struct timespec'. GNU glibc
defaults as above, also exposing the SO_TIMESTAMPNS_NEW flag to explicitly
use a 64-bit call and 'struct __kernel_timespec'. Since these are not
exposed or needed with musl libc, their use in tc_redirect.c leads to
compile errors building for mips64el/musl:
tc_redirect.c: In function 'rcv_tstamp':
tc_redirect.c:425:32: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
425 | cmsg->cmsg_type == SO_TIMESTAMPNS_NEW)
| ^~~~~~~~~~~~~~~~~~
| SO_TIMESTAMPNS
tc_redirect.c:425:32: note: each undeclared identifier is reported only once for each function it appears in
tc_redirect.c: In function 'test_inet_dtime':
tc_redirect.c:491:49: error: 'SO_TIMESTAMPNS_NEW' undeclared (first use in this function); did you mean 'SO_TIMESTAMPNS'?
491 | err = setsockopt(listen_fd, SOL_SOCKET, SO_TIMESTAMPNS_NEW,
| ^~~~~~~~~~~~~~~~~~
| SO_TIMESTAMPNS
However, using SO_TIMESTAMPNS_NEW isn't strictly needed, nor is Y2038 being
explicitly tested. The timestamp checks in tc_redirect.c are simple: the
packet receive timestamp is non-zero and processed/handled in less than 5
seconds.
Switch to using the standard setsockopt() call and SO_TIMESTAMPNS option to
ensure compatibility across glibc and musl libc. In the worst-case, there
is a 5-second window 14 years from now where tc_redirect tests may fail on
32-bit systems. However, we should reasonably expect glibc to adopt a
64-bit mandate rather than the current "opt-in" policy before the Y2038
roll-over.
Tony Ambardar [Mon, 29 Jul 2024 09:24:23 +0000 (02:24 -0700)]
selftests/bpf: Fix using stdout, stderr as struct field names
Typically stdin, stdout, stderr are treated as reserved identifiers under
ISO/ANSI C and libc implementations further define these as macros, both in
glibc and musl <stdio.h>.
However, while glibc defines:
...
/* Standard streams. */
extern FILE *stdin; /* Standard input stream. */
extern FILE *stdout; /* Standard output stream. */
extern FILE *stderr; /* Standard error output stream. */
/* C89/C99 say they're macros. Make them happy. */
#define stdin stdin
#define stdout stdout
#define stderr stderr
...
The latter results in compile errors when the names are reused as fields of
'struct test_env' and elsewhere in test_progs.[ch] and reg_bounds.c.
Rename the fields to stdout_saved and stderr_saved to avoid many errors
seen building against musl, e.g.:
In file included from test_progs.h:6,
from test_progs.c:5:
test_progs.c: In function 'print_test_result':
test_progs.c:237:21: error: expected identifier before '(' token
237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
| ^~~~~~
test_progs.c:237:9: error: too few arguments to function 'fprintf'
237 | fprintf(env.stdout, "#%-*d %s:", TEST_NUM_WIDTH, test->test_num, test->test_name);
| ^~~~~~~
Tony Ambardar [Mon, 29 Jul 2024 09:24:22 +0000 (02:24 -0700)]
selftests/bpf: Fix compile if backtrace support missing in libc
Include GNU <execinfo.h> header only with glibc and provide weak, stubbed
backtrace functions as a fallback in test_progs.c. This allows for non-GNU
replacements while avoiding compile errors (e.g. with musl libc) like:
test_progs.c:13:10: fatal error: execinfo.h: No such file or directory
13 | #include <execinfo.h> /* backtrace */
| ^~~~~~~~~~~~
test_progs.c: In function 'crash_handler':
test_progs.c:1034:14: error: implicit declaration of function 'backtrace' [-Werror=implicit-function-declaration]
1034 | sz = backtrace(bt, ARRAY_SIZE(bt));
| ^~~~~~~~~
test_progs.c:1045:9: error: implicit declaration of function 'backtrace_symbols_fd' [-Werror=implicit-function-declaration]
1045 | backtrace_symbols_fd(bt, sz, STDERR_FILENO);
| ^~~~~~~~~~~~~~~~~~~~
Compiling lwt_reroute.c with GCC 12.3 for mips64el/musl-libc yields errors:
In file included from .../include/arpa/inet.h:9,
from ./test_progs.h:18,
from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:11,
from tools/testing/selftests/bpf/prog_tests/lwt_reroute.c:52:
.../include/netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
23 | struct in6_addr {
| ^~~~~~~~
In file included from .../include/linux/icmp.h:24,
from tools/testing/selftests/bpf/prog_tests/lwt_helpers.h:9:
.../include/linux/in6.h:33:8: note: originally defined here
33 | struct in6_addr {
| ^~~~~~~~
.../include/netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
34 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../include/linux/in6.h:50:8: note: originally defined here
50 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../include/netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
42 | struct ipv6_mreq {
| ^~~~~~~~~
.../include/linux/in6.h:60:8: note: originally defined here
60 | struct ipv6_mreq {
| ^~~~~~~~~
These errors occur because <linux/in6.h> is included before <netinet/in.h>,
bypassing the Linux uapi/libc compat mechanism's partial musl support. As
described in [1] and [2], fix these errors by including <netinet/in.h> in
lwt_reroute.c before any uapi headers.
Tony Ambardar [Mon, 29 Jul 2024 09:24:20 +0000 (02:24 -0700)]
selftests/bpf: Fix C++ compile error from missing _Bool type
While building, bpftool makes a skeleton from test_core_extern.c, which
itself includes <stdbool.h> and uses the 'bool' type. However, the skeleton
test_core_extern.skel.h generated *does not* include <stdbool.h> or use the
'bool' type, instead using the C-only '_Bool' type. Compiling test_cpp.cpp
with g++ 12.3 for mips64el/musl-libc then fails with error:
In file included from test_cpp.cpp:9:
test_core_extern.skel.h:45:17: error: '_Bool' does not name a type
45 | _Bool CONFIG_BOOL;
| ^~~~~
This was likely missed previously because glibc uses a GNU extension for
<stdbool.h> with C++ (#define _Bool bool), not supported by musl libc.
Normally, a C fragment would include <stdbool.h> and use the 'bool' type,
and thus cleanly work after import by C++. The ideal fix would be for
'bpftool gen skeleton' to output the correct type/include supporting C++,
but in the meantime add a conditional define as above.
Tony Ambardar [Mon, 29 Jul 2024 09:24:19 +0000 (02:24 -0700)]
selftests/bpf: Fix error compiling test_lru_map.c
Although the post-increment in macro 'CPU_SET(next++, &cpuset)' seems safe,
the sequencing can raise compile errors, so move the increment outside the
macro. This avoids an error seen using gcc 12.3.0 for mips64el/musl-libc:
In file included from test_lru_map.c:11:
test_lru_map.c: In function 'sched_next_online':
test_lru_map.c:129:29: error: operation on 'next' may be undefined [-Werror=sequence-point]
129 | CPU_SET(next++, &cpuset);
| ^
cc1: all warnings being treated as errors
where logic assumes the 'state' var can distinguish between first and
subsequent strtok_r() calls, and adjusts parameters accordingly. However,
'state' is strictly internal context for strtok_r() and no such assumptions
are supported in the man page. Moreover, the exact behaviour of 'state'
depends on the libc implementation, making the above code fragile.
Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang,
with the above code in an infinite loop.
Similarly, we see strange behaviour running 'veristat' on mips64el/musl:
$ ./veristat -e file,prog,verdict,insns -C two-ok add-failure
Can't specify more than 9 stats
Rewrite code using a counter to distinguish between strtok_r() calls.
Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests") Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat") Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files") Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Link: https://lore.kernel.org/bpf/392d8bf5559f85fa37926c1494e62312ef252c3d.1722244708.git.tony.ambardar@gmail.com
Tony Ambardar [Mon, 29 Jul 2024 09:24:17 +0000 (02:24 -0700)]
selftests/bpf: Use portable POSIX basename()
Use the POSIX version of basename() to allow compilation against non-gnu
libc (e.g. musl). Include <libgen.h> ahead of <string.h> to enable using
functions from the latter while preferring POSIX over GNU basename().
In veristat.c, rely on strdupa() to avoid basename() altering the passed
"const char" argument. This is not needed in xskxceiver.c since the arg
is mutable and the program exits immediately after usage.
David Vernet [Thu, 25 Jul 2024 03:22:14 +0000 (22:22 -0500)]
selftests/bpf: Load struct_ops map in global_maps_resize test
In prog_tests/test_global_maps_resize.c, we test various use cases for
resizing global maps. Commit 7244100e0389 ("libbpf: Don't take direct
pointers into BTF data from st_ops") updated libbpf to not store pointers
to volatile BTF data, which for some users, was causing a UAF when resizing
a datasec array.
Let's ensure we have coverage for resizing datasec arrays with struct_ops
progs by also including a struct_ops map and struct_ops prog in the
test_global_map_resize skeleton. The map is automatically loaded, so we
don't need to do anything other than add it to the BPF prog being tested
to get the coverage.
selftests/bpf: Integrate test_xdp_veth into test_progs
test_xdp_veth.sh tests that XDP return codes work as expected, by bringing
up multiple veth pairs isolated in different namespaces, attaching specific
xdp programs to each interface, and ensuring that the whole chain allows to
ping one end interface from the first one. The test runs well but is
currently not integrated in test_progs, which prevents it from being run
automatically in the CI infrastructure.
Rewrite it as a C test relying on libbpf to allow running it in the CI
infrastructure. The new code brings up the same network infrastructure and
reuses the same eBPF programs as test_xdp_veth.sh, for which skeletons are
already generated by the bpf tests makefile.
selftests/bpf: Update xdp_redirect_map prog sections for libbpf
xdp_redirect_map.c is a bpf program used by test_xdp_veth.sh, which is not
handled by the generic test runner (test_progs). To allow converting this
test to test_progs, the corresponding program must be updated to allow
handling it through skeletons generated by bpftool and libbpf.
Update programs section names to allow to manipulate those with libbpf.
David Vernet [Wed, 24 Jul 2024 17:14:58 +0000 (12:14 -0500)]
libbpf: Don't take direct pointers into BTF data from st_ops
In struct bpf_struct_ops, we have take a pointer to a BTF type name, and
a struct btf_type. This was presumably done for convenience, but can
actually result in subtle and confusing bugs given that BTF data can be
invalidated before a program is loaded. For example, in sched_ext, we
may sometimes resize a data section after a skeleton has been opened,
but before the struct_ops scheduler map has been loaded. This may cause
the BTF data to be realloc'd, which can then cause a UAF when loading
the program because the struct_ops map has pointers directly into the
BTF data.
We're already storing the BTF type_id in struct bpf_struct_ops. Because
type_id is stable, we can therefore just update the places where we were
looking at those pointers to instead do the lookups we need from the
type_id.
====================
selftests/bpf: Improve libc portability / musl support (part 1)
Hello all,
This series includes the bulk of libc-related compile fixes accumulated to
support systems using musl, with smaller numbers to follow. These patches
are simple and straightforward, and the series has been tested with the
kernel-patches/bpf CI and locally using mips64el-gcc/musl-libc and QEMU
with an OpenWrt rootfs.
The patches address a few general categories of libc portability issues:
- missing, redundant or incorrect include headers
- disabled GNU header extensions (i.e. missing #define _GNU_SOURCE)
- issues with types and casting
Feedback and suggestions for improvement are welcome!
Tony Ambardar [Tue, 23 Jul 2024 05:54:46 +0000 (22:54 -0700)]
selftests/bpf: Fix errors compiling cg_storage_multi.h with musl libc
Remove a redundant include of '<asm/types.h>', whose needed definitions are
already included (via '<linux/types.h>') in cg_storage_multi_egress_only.c,
cg_storage_multi_isolated.c, and cg_storage_multi_shared.c. This avoids
redefinition errors seen compiling for mips64el/musl-libc like:
In file included from progs/cg_storage_multi_egress_only.c:13:
In file included from progs/cg_storage_multi.h:6:
In file included from /usr/mips64el-linux-gnuabi64/include/asm/types.h:23:
/usr/include/asm-generic/int-l64.h:29:25: error: typedef redefinition with different types ('long' vs 'long long')
29 | typedef __signed__ long __s64;
| ^
/usr/include/asm-generic/int-ll64.h:30:44: note: previous definition is here
30 | __extension__ typedef __signed__ long long __s64;
| ^
Tony Ambardar [Tue, 23 Jul 2024 05:54:45 +0000 (22:54 -0700)]
selftests/bpf: Fix errors compiling crypto_sanity.c with musl libc
Remove a redundant include of '<linux/in6.h>', whose needed definitions are
already provided by 'test_progs.h'. This avoids errors seen compiling for
mips64el/musl-libc:
In file included from .../arpa/inet.h:9,
from ./test_progs.h:17,
from prog_tests/crypto_sanity.c:10:
.../netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
23 | struct in6_addr {
| ^~~~~~~~
In file included from crypto_sanity.c:7:
.../linux/in6.h:33:8: note: originally defined here
33 | struct in6_addr {
| ^~~~~~~~
.../netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
34 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../linux/in6.h:50:8: note: originally defined here
50 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
42 | struct ipv6_mreq {
| ^~~~~~~~~
.../linux/in6.h:60:8: note: originally defined here
60 | struct ipv6_mreq {
| ^~~~~~~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:44 +0000 (22:54 -0700)]
selftests/bpf: Fix errors compiling decap_sanity.c with musl libc
Remove a redundant include of '<linux/in6.h>', whose needed definitions are
already provided by 'test_progs.h'. This avoids errors seen compiling for
mips64el/musl-libc:
In file included from .../arpa/inet.h:9,
from ./test_progs.h:17,
from prog_tests/decap_sanity.c:9:
.../netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
23 | struct in6_addr {
| ^~~~~~~~
In file included from decap_sanity.c:7:
.../linux/in6.h:33:8: note: originally defined here
33 | struct in6_addr {
| ^~~~~~~~
.../netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
34 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../linux/in6.h:50:8: note: originally defined here
50 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
42 | struct ipv6_mreq {
| ^~~~~~~~~
.../linux/in6.h:60:8: note: originally defined here
60 | struct ipv6_mreq {
| ^~~~~~~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:43 +0000 (22:54 -0700)]
selftests/bpf: Fix errors compiling lwt_redirect.c with musl libc
Remove a redundant include of '<linux/icmp.h>' which is already provided in
'lwt_helpers.h'. This avoids errors seen compiling for mips64el/musl-libc:
In file included from .../arpa/inet.h:9,
from lwt_redirect.c:51:
.../netinet/in.h:23:8: error: redefinition of 'struct in6_addr'
23 | struct in6_addr {
| ^~~~~~~~
In file included from .../linux/icmp.h:24,
from lwt_redirect.c:50:
.../linux/in6.h:33:8: note: originally defined here
33 | struct in6_addr {
| ^~~~~~~~
.../netinet/in.h:34:8: error: redefinition of 'struct sockaddr_in6'
34 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../linux/in6.h:50:8: note: originally defined here
50 | struct sockaddr_in6 {
| ^~~~~~~~~~~~
.../netinet/in.h:42:8: error: redefinition of 'struct ipv6_mreq'
42 | struct ipv6_mreq {
| ^~~~~~~~~
.../linux/in6.h:60:8: note: originally defined here
60 | struct ipv6_mreq {
| ^~~~~~~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:42 +0000 (22:54 -0700)]
selftests/bpf: Fix compiling core_reloc.c with musl-libc
The type 'loff_t' is a GNU extension and not exposed by the musl 'fcntl.h'
header unless _GNU_SOURCE is defined. Add this definition to fix errors
seen compiling for mips64el/musl-libc:
In file included from tools/testing/selftests/bpf/prog_tests/core_reloc.c:4:
./bpf_testmod/bpf_testmod.h:10:9: error: unknown type name 'loff_t'
10 | loff_t off;
| ^~~~~~
./bpf_testmod/bpf_testmod.h:16:9: error: unknown type name 'loff_t'
16 | loff_t off;
| ^~~~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:41 +0000 (22:54 -0700)]
selftests/bpf: Fix compiling tcp_rtt.c with musl-libc
The GNU version of 'struct tcp_info' in 'netinet/tcp.h' is not exposed by
musl headers unless _GNU_SOURCE is defined.
Add this definition to fix errors seen compiling for mips64el/musl-libc:
tcp_rtt.c: In function 'wait_for_ack':
tcp_rtt.c:24:25: error: storage size of 'info' isn't known
24 | struct tcp_info info;
| ^~~~
tcp_rtt.c:24:25: error: unused variable 'info' [-Werror=unused-variable]
cc1: all warnings being treated as errors
Tony Ambardar [Tue, 23 Jul 2024 05:54:40 +0000 (22:54 -0700)]
selftests/bpf: Fix compiling flow_dissector.c with musl-libc
The GNU version of 'struct tcphdr' has members 'doff', 'source' and 'dest',
which are not exposed by musl libc headers unless _GNU_SOURCE is defined.
Add this definition to fix errors seen compiling for mips64el/musl-libc:
flow_dissector.c:118:30: error: 'struct tcphdr' has no member named 'doff'
118 | .tcp.doff = 5,
| ^~~~
flow_dissector.c:119:30: error: 'struct tcphdr' has no member named 'source'
119 | .tcp.source = 80,
| ^~~~~~
flow_dissector.c:120:30: error: 'struct tcphdr' has no member named 'dest'
120 | .tcp.dest = 8080,
| ^~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:39 +0000 (22:54 -0700)]
selftests/bpf: Fix compiling kfree_skb.c with musl-libc
The GNU version of 'struct tcphdr' with member 'doff' is not exposed by
musl headers unless _GNU_SOURCE is defined. Add this definition to fix
errors seen compiling for mips64el/musl-libc:
In file included from kfree_skb.c:2:
kfree_skb.c: In function 'on_sample':
kfree_skb.c:45:30: error: 'struct tcphdr' has no member named 'doff'
45 | if (CHECK(pkt_v6->tcp.doff != 5, "check_tcp",
| ^
Tony Ambardar [Tue, 23 Jul 2024 05:54:38 +0000 (22:54 -0700)]
selftests/bpf: Fix compiling parse_tcp_hdr_opt.c with musl-libc
The GNU version of 'struct tcphdr', with members 'doff' and 'urg_ptr', is
not exposed by musl headers unless _GNU_SOURCE is defined.
Add this definition to fix errors seen compiling for mips64el/musl-libc:
parse_tcp_hdr_opt.c:18:21: error: 'struct tcphdr' has no member named 'urg_ptr'
18 | .pk6_v6.tcp.urg_ptr = 123,
| ^~~~~~~
parse_tcp_hdr_opt.c:19:21: error: 'struct tcphdr' has no member named 'doff'
19 | .pk6_v6.tcp.doff = 9, /* 16 bytes of options */
| ^~~~
Tony Ambardar [Tue, 23 Jul 2024 05:54:37 +0000 (22:54 -0700)]
selftests/bpf: Fix include of <sys/fcntl.h>
Update ns_current_pid_tgid.c to use '#include <fcntl.h>' and avoid compile
error against mips64el/musl libc:
In file included from .../prog_tests/ns_current_pid_tgid.c:14:
.../include/sys/fcntl.h:1:2: error: #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h> [-Werror=cpp]
1 | #warning redirecting incorrect #include <sys/fcntl.h> to <fcntl.h>
| ^~~~~~~
cc1: all warnings being treated as errors
Explicitly include '<linux/build_bug.h>' to fix errors seen compiling with
gcc targeting mips64el/musl-libc:
user_ringbuf.c: In function 'test_user_ringbuf_loop':
user_ringbuf.c:426:9: error: implicit declaration of function 'BUILD_BUG_ON' [-Werror=implicit-function-declaration]
426 | BUILD_BUG_ON(total_samples <= c_max_entries);
| ^~~~~~~~~~~~
cc1: all warnings being treated as errors
Tony Ambardar [Tue, 23 Jul 2024 05:54:35 +0000 (22:54 -0700)]
selftests/bpf: Fix missing UINT_MAX definitions in benchmarks
Include <limits.h> in 'bench.h' to provide a UINT_MAX definition and avoid
multiple compile errors against mips64el/musl-libc like:
benchs/bench_local_storage.c: In function 'parse_arg':
benchs/bench_local_storage.c:40:38: error: 'UINT_MAX' undeclared (first use in this function)
40 | if (ret < 1 || ret > UINT_MAX) {
| ^~~~~~~~
benchs/bench_local_storage.c:11:1: note: 'UINT_MAX' is defined in header '<limits.h>'; did you forget to '#include <limits.h>'?
10 | #include <test_btf.h>
+++ |+#include <limits.h>
11 |
seen with bench_local_storage.c, bench_local_storage_rcu_tasks_trace.c, and
bench_bpf_hashmap_lookup.c.
Tony Ambardar [Tue, 23 Jul 2024 05:54:34 +0000 (22:54 -0700)]
selftests/bpf: Fix missing ARRAY_SIZE() definition in bench.c
Add a "bpf_util.h" include to avoid the following error seen compiling for
mips64el with musl libc:
bench.c: In function 'find_benchmark':
bench.c:590:25: error: implicit declaration of function 'ARRAY_SIZE' [-Werror=implicit-function-declaration]
590 | for (i = 0; i < ARRAY_SIZE(benchs); i++) {
| ^~~~~~~~~~
cc1: all warnings being treated as errors
Tony Ambardar [Tue, 23 Jul 2024 05:54:31 +0000 (22:54 -0700)]
selftests/bpf: Drop unneeded error.h includes
The addition of general support for unprivileged tests in test_loader.c
breaks building test_verifier on non-glibc (e.g. musl) systems, due to the
inclusion of glibc extension '<error.h>' in 'unpriv_helpers.c'. However,
the header is actually not needed, so remove it to restore building.
Similarly for sk_lookup.c and flow_dissector.c, error.h is not necessary
and causes problems, so drop them.
Fixes: 1d56ade032a4 ("selftests/bpf: Unprivileged tests for test_loader.c") Fixes: 0ab5539f8584 ("selftests/bpf: Tests for BPF_SK_LOOKUP attach point") Fixes: 0905beec9f52 ("selftests/bpf: run flow dissector tests in skb-less mode") Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Link: https://lore.kernel.org/bpf/5664367edf5fea4f3f4b4aec3b182bcfc6edff9c.1721713597.git.tony.ambardar@gmail.com
Tony Ambardar [Tue, 23 Jul 2024 05:54:30 +0000 (22:54 -0700)]
selftests/bpf: Fix error compiling bpf_iter_setsockopt.c with musl libc
Existing code calls getsockname() with a 'struct sockaddr_in6 *' argument
where a 'struct sockaddr *' argument is declared, yielding compile errors
when building for mips64el/musl-libc:
bpf_iter_setsockopt.c: In function 'get_local_port':
bpf_iter_setsockopt.c:98:30: error: passing argument 2 of 'getsockname' from incompatible pointer type [-Werror=incompatible-pointer-types]
98 | if (!getsockname(fd, &addr, &addrlen))
| ^~~~~
| |
| struct sockaddr_in6 *
In file included from .../netinet/in.h:10,
from .../arpa/inet.h:9,
from ./test_progs.h:17,
from bpf_iter_setsockopt.c:5:
.../sys/socket.h:391:23: note: expected 'struct sockaddr * restrict' but argument is of type 'struct sockaddr_in6 *'
391 | int getsockname (int, struct sockaddr *__restrict, socklen_t *__restrict);
| ^
cc1: all warnings being treated as errors
This compiled under glibc only because the argument is declared to be a
"funky" transparent union which includes both types above. Explicitly cast
the argument to allow compiling for both musl and glibc.
Tony Ambardar [Tue, 23 Jul 2024 05:54:29 +0000 (22:54 -0700)]
selftests/bpf: Fix compile error from rlim_t in sk_storage_map.c
Cast 'rlim_t' argument to match expected type of printf() format and avoid
compile errors seen building for mips64el/musl-libc:
In file included from map_tests/sk_storage_map.c:20:
map_tests/sk_storage_map.c: In function 'test_sk_storage_map_stress_free':
map_tests/sk_storage_map.c:414:56: error: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'rlim_t' {aka 'long long unsigned int'} [-Werror=format=]
414 | CHECK(err, "setrlimit(RLIMIT_NOFILE)", "rlim_new:%lu errno:%d",
| ^~~~~~~~~~~~~~~~~~~~~~~
415 | rlim_new.rlim_cur, errno);
| ~~~~~~~~~~~~~~~~~
| |
| rlim_t {aka long long unsigned int}
./test_maps.h:12:24: note: in definition of macro 'CHECK'
12 | printf(format); \
| ^~~~~~
map_tests/sk_storage_map.c:414:68: note: format string is defined here
414 | CHECK(err, "setrlimit(RLIMIT_NOFILE)", "rlim_new:%lu errno:%d",
| ~~^
| |
| long unsigned int
| %llu
cc1: all warnings being treated as errors
Tony Ambardar [Tue, 23 Jul 2024 05:54:28 +0000 (22:54 -0700)]
selftests/bpf: Use pid_t consistently in test_progs.c
Use pid_t rather than __pid_t when allocating memory for 'worker_pids' in
'struct test_env', as this is its declared type and also avoids compile
errors seen building against musl libc on mipsel64:
test_progs.c:1738:49: error: '__pid_t' undeclared (first use in this function); did you mean 'pid_t'?
1738 | env.worker_pids = calloc(sizeof(__pid_t), env.workers);
| ^~~~~~~
| pid_t
test_progs.c:1738:49: note: each undeclared identifier is reported only once for each function it appears in
====================
no_caller_saved_registers attribute for helper calls
This patch-set seeks to allow using no_caller_saved_registers gcc/clang
attribute with some BPF helper functions (and kfuncs in the future).
As documented in [1], this attribute means that function scratches
only some of the caller saved registers defined by ABI.
For BPF the set of such registers could be defined as follows:
- R0 is scratched only if function is non-void;
- R1-R5 are scratched only if corresponding parameter type is defined
in the function prototype.
The goal of the patch-set is to implement no_caller_saved_registers
(nocsr for short) in a backwards compatible manner:
- for kernels that support the feature, gain some performance boost
from better register allocation;
- for kernels that don't support the feature, allow programs execution
with minor performance losses.
To achieve this, use a scheme suggested by Alexei Starovoitov:
- for nocsr calls clang allocates registers as-if relevant r0-r5
registers are not scratched by the call;
- as a post-processing step, clang visits each nocsr call and adds
spill/fill for every live r0-r5;
- stack offsets used for spills/fills are allocated as lowest
stack offsets in whole function and are not used for any other
purpose;
- when kernel loads a program, it looks for such patterns
(nocsr function surrounded by spills/fills) and checks if
spill/fill stack offsets are used exclusively in nocsr patterns;
- if so, and if current JIT inlines the call to the nocsr function
(e.g. a helper call), kernel removes unnecessary spill/fill pairs;
- when old kernel loads a program, presence of spill/fill pairs
keeps BPF program valid, albeit slightly less efficient.
Corresponding clang/llvm changes are available in [2].
The patch-set uses bpf_get_smp_processor_id() function as a canary,
making it the first helper with nocsr attribute.
Change list:
- v3 -> v4:
- When nocsr spills/fills are removed in the subprogram, allow these
spills/fills to reside in [-MAX_BPF_STACK-48..MAX_BPF_STACK) range
(suggested by Alexei);
- Dropped patches with special handling for bpf_probe_read_kernel()
(requested by Alexei);
- Reset aux .nocsr_pattern and .nocsr_spills_num fields in
check_nocsr_stack_contract() (requested by Andrii).
Andrii, I have not added an additional flag to
struct bpf_subprog_info, it currently does not have holes
and I really don't like adding a bool field there just as an
alternative indicator that nocsr is disabled.
Indicator at the moment:
- nocsr_stack_off >= S16_MIN means that nocsr rewrite is enabled;
- nocsr_stack_off == S16_MIN means that nocsr rewrite is disabled.
- v2 -> v3:
- As suggested by Andrii, 'nocsr_stack_off' is no longer checked at
rewrite time, instead mark_nocsr_patterns() now does two passes
over BPF program:
- on a first pass it computes the lowest stack spill offset for
the subprogram;
- on a second pass this offset is used to recognize nocsr pattern.
- As suggested by Alexei, a new mechanic is added to work around a
situation mentioned by Andrii, when more helper functions are
marked as nocsr at compile time than current kernel supports:
- all {spill*,helper call,fill*} patterns are now marked as
insn_aux_data[*].nocsr_pattern, thus relaxing failure condition
for check_nocsr_stack_contract();
- spill/fill pairs are not removed for patterns where helper can't
be inlined;
- see mark_nocsr_pattern_for_call() for details an example.
- As suggested by Alexei, subprogram stack depth is now adjusted
if all spill/fill pairs could be removed. This adjustment has
to take place before optimize_bpf_loop(), hence the rewrite
is moved from do_misc_fixups() to remove_nocsr_spills_fills()
(again).
- As suggested by Andrii, special measures are taken to work around
bpf_probe_read_kernel() access to BPF stack, see patches 11, 12.
Patch #11 is very simplistic, a more comprehensive solution would
be to change the type of the third parameter of the
bpf_probe_read_kernel() from ARG_ANYTHING to something else and
not only check nocsr contract, but also propagate stack slot
liveness information. However, such change would require update in
struct bpf_call_arg_meta processing, which currently implies that
every memory parameter is followed by a size parameter.
I can work on these changes, please comment.
- Stylistic changes suggested by Andrii.
- Added acks from Andrii.
- Dropped RFC tag.
- v1 -> v2:
- assume that functions inlined by either jit or verifier
conform to no_caller_saved_registers contract (Andrii, Puranjay);
- allow nocsr rewrite for bpf_get_smp_processor_id()
on arm64 and riscv64 architectures (Puranjay);
- __arch_{x86_64,arm64,riscv64} macro for test_loader;
- moved remove_nocsr_spills_fills() inside do_misc_fixups() (Andrii);
- moved nocsr pattern detection from check_cfg() to a separate pass
(Andrii);
- various stylistic/correctness changes according to Andrii's
comments.
Eduard Zingerman [Mon, 22 Jul 2024 23:38:44 +0000 (16:38 -0700)]
selftests/bpf: test no_caller_saved_registers spill/fill removal
Tests for no_caller_saved_registers processing logic
(see verifier.c:match_and_mark_nocsr_pattern()):
- a canary positive test case;
- a canary test case for arm64 and riscv64;
- various tests with broken patterns;
- tests with read/write fixed/varying stack access that violate nocsr
stack access contract;
- tests with multiple subprograms;
- tests using nocsr in combination with may_goto/bpf_loop,
as all of these features affect stack depth;
- tests for nocsr stack spills below max stack depth.
Eduard Zingerman [Mon, 22 Jul 2024 23:38:43 +0000 (16:38 -0700)]
selftests/bpf: __arch_* macro to limit test cases to specific archs
Add annotations __arch_x86_64, __arch_arm64, __arch_riscv64
to specify on which architecture the test case should be tested.
Several __arch_* annotations could be specified at once.
When test case is not run on current arch it is marked as skipped.
For example, the following would be tested only on arm64 and riscv64:
Eduard Zingerman [Mon, 22 Jul 2024 23:38:42 +0000 (16:38 -0700)]
selftests/bpf: allow checking xlated programs in verifier_* tests
Add a macro __xlated("...") for use with test_loader tests.
When such annotations are present for the test case:
- bpf_prog_get_info_by_fd() is used to get BPF program after all
rewrites are applied by verifier.
- the program is disassembled and patterns specified in __xlated are
searched for in the disassembly text.
__xlated matching follows the same mechanics as __msg:
each subsequent pattern is matched from the point where
previous pattern ended.
This allows to write tests like below, where the goal is to verify the
behavior of one of the of the transformations applied by verifier:
Eduard Zingerman [Mon, 22 Jul 2024 23:38:41 +0000 (16:38 -0700)]
selftests/bpf: extract test_loader->expect_msgs as a data structure
Non-functional change: use a separate data structure to represented
expected messages in test_loader.
This would allow to use the same functionality for expected set of
disassembled instructions in the follow-up commit.
Eduard Zingerman [Mon, 22 Jul 2024 23:38:40 +0000 (16:38 -0700)]
selftests/bpf: no need to track next_match_pos in struct test_loader
The call stack for validate_case() function looks as follows:
- test_loader__run_subtests()
- process_subtest()
- run_subtest()
- prepare_case(), which does 'tester->next_match_pos = 0';
- validate_case(), which increments tester->next_match_pos.
Hence, each subtest is run with next_match_pos freshly set to zero.
Meaning that there is no need to persist this variable in the
struct test_loader, use local variable instead.
Disassembles instruction 'insn' to a text buffer 'buf'.
Removes insn->code hex prefix added by kernel disassembly routine.
Returns a pointer to the next instruction
(increments insn by either 1 or 2).
The source code:
int iter_arr_with_actual_elem_count(const void *ctx)
{
int i, n = loop_data.n, sum = 0;
if (n > ARRAY_SIZE(loop_data.data))
return 0;
bpf_for(i, 0, n) {
/* no rechecking of i against ARRAY_SIZE(loop_data.n) */
sum += loop_data.data[i];
}
return sum;
}
The insn #14 is a sign-extenstion load which is related to 'int i'.
The insn #15 did a subreg comparision. Note that smin=0xffffffff80000000 and this caused later
insn #23 failed verification due to unbounded min value.
Actually insn #15 R1 smin range can be better. Before insn #15, we have
R1_w=scalar(smin=0xffffffff80000000,smax=0x7fffffff)
With the above range, we know for R1, upper 32bit can only be 0xffffffff or 0.
Otherwise, the value range for R1 could be beyond [smin=0xffffffff80000000,smax=0x7fffffff].
After insn #15, for the true patch, we know smin32=0 and smax32=32. With the upper 32bit 0xffffffff,
then the corresponding value is [0xffffffff00000000, 0xffffffff00000020]. The range is
obviously beyond the original range [smin=0xffffffff80000000,smax=0x7fffffff] and the
range is not possible. So the upper 32bit must be 0, which implies smin = smin32 and
smax = smax32.
This patch fixed the issue by adding additional register deduction after 32-bit compare
insn. If the signed 32-bit register range is non-negative then 64-bit smin is
in range of [S32_MIN, S32_MAX], then the actual 64-bit smin/smax should be the same
as 32-bit smin32/smax32.
With this patch, iters/iter_arr_with_actual_elem_count succeeded with better register range:
from 15 to 20: R0=rdonly_mem(id=7,ref_obj_id=2,sz=4) R1_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=31,var_off=(0x0; 0x1f)) R6=scalar(id=1,smin=umin=smin32=umin32=1,smax=umax=smax32=umax32=32,var_off=(0x0; 0x3f)) R7=scalar(id=9,smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R8=scalar(id=9,smin=0,smax=umax=0xffffffff,var_off=(0x0; 0xffffffff)) R10=fp0 fp-8=iter_num(ref_id=2,state=active,depth=3) refs=2
Eduard Zingerman [Mon, 22 Jul 2024 23:38:37 +0000 (16:38 -0700)]
bpf, x86, riscv, arm: no_caller_saved_registers for bpf_get_smp_processor_id()
The function bpf_get_smp_processor_id() is processed in a different
way, depending on the arch:
- on x86 verifier replaces call to bpf_get_smp_processor_id() with a
sequence of instructions that modify only r0;
- on riscv64 jit replaces call to bpf_get_smp_processor_id() with a
sequence of instructions that modify only r0;
- on arm64 jit replaces call to bpf_get_smp_processor_id() with a
sequence of instructions that modify only r0 and tmp registers.
These rewrites satisfy attribute no_caller_saved_registers contract.
Allow rewrite of no_caller_saved_registers patterns for
bpf_get_smp_processor_id() in order to use this function as a canary
for no_caller_saved_registers tests.
Yonghong Song [Tue, 23 Jul 2024 15:34:44 +0000 (08:34 -0700)]
selftests/bpf: Add tests for ldsx of pkt data/data_end/data_meta accesses
The following tests are added to verifier_ldsx.c:
- sign extension of data/data_end/data_meta for tcx programs.
The actual checking is in bpf_skb_is_valid_access() which
is called by sk_filter, cg_skb, lwt, tc(tcx) and sk_skb.
- sign extension of data/data_end/data_meta for xdp programs.
- sign extension of data/data_end for flow_dissector programs.
All newly-added tests have verification failure with message
"invalid bpf_context access". Without previous patch, all these
tests succeeded verification.
Eduard Zingerman [Mon, 22 Jul 2024 23:38:36 +0000 (16:38 -0700)]
bpf: no_caller_saved_registers attribute for helper calls
GCC and LLVM define a no_caller_saved_registers function attribute.
This attribute means that function scratches only some of
the caller saved registers defined by ABI.
For BPF the set of such registers could be defined as follows:
- R0 is scratched only if function is non-void;
- R1-R5 are scratched only if corresponding parameter type is defined
in the function prototype.
This commit introduces flag bpf_func_prot->allow_nocsr.
If this flag is set for some helper function, verifier assumes that
it follows no_caller_saved_registers calling convention.
The contract between kernel and clang allows to simultaneously use
such functions and maintain backwards compatibility with old
kernels that don't understand no_caller_saved_registers calls
(nocsr for short):
- clang generates a simple pattern for nocsr calls, e.g.:
- kernel removes unnecessary spills and fills, if called function is
inlined by verifier or current JIT (with assumption that patch
inserted by verifier or JIT honors nocsr contract, e.g. does not
scratch r3-r5 for the example above), e.g. the code above would be
transformed to:
Technically, the transformation is split into the following phases:
- function mark_nocsr_patterns(), called from bpf_check()
searches and marks potential patterns in instruction auxiliary data;
- upon stack read or write access,
function check_nocsr_stack_contract() is used to verify if
stack offsets, presumably reserved for nocsr patterns, are used
only from those patterns;
- function remove_nocsr_spills_fills(), called from bpf_check(),
applies the rewrite for valid patterns.
See comment in mark_nocsr_pattern_for_call() for more details.
Yonghong Song [Tue, 23 Jul 2024 15:34:39 +0000 (08:34 -0700)]
bpf: Fail verification for sign-extension of packet data/data_end/data_meta
syzbot reported a kernel crash due to
commit 1f1e864b6555 ("bpf: Handle sign-extenstin ctx member accesses").
The reason is due to sign-extension of 32-bit load for
packet data/data_end/data_meta uapi field.
The original code looks like:
r2 = *(s32 *)(r1 + 76) /* load __sk_buff->data */
r3 = *(u32 *)(r1 + 80) /* load __sk_buff->data_end */
r0 = r2
r0 += 8
if r3 > r0 goto +1
...
Note that __sk_buff->data load has 32-bit sign extension.
After verification and convert_ctx_accesses(), the final asm code looks like:
r2 = *(u64 *)(r1 +208)
r2 = (s32)r2
r3 = *(u64 *)(r1 +80)
r0 = r2
r0 += 8
if r3 > r0 goto pc+1
...
Note that 'r2 = (s32)r2' may make the kernel __sk_buff->data address invalid
which may cause runtime failure.
Currently, in C code, typically we have
void *data = (void *)(long)skb->data;
void *data_end = (void *)(long)skb->data_end;
...
and it will generate
r2 = *(u64 *)(r1 +208)
r3 = *(u64 *)(r1 +80)
r0 = r2
r0 += 8
if r3 > r0 goto pc+1
If we allow sign-extension,
void *data = (void *)(long)(int)skb->data;
void *data_end = (void *)(long)skb->data_end;
...
the generated code looks like
r2 = *(u64 *)(r1 +208)
r2 <<= 32
r2 s>>= 32
r3 = *(u64 *)(r1 +80)
r0 = r2
r0 += 8
if r3 > r0 goto pc+1
and this will cause verification failure since "r2 <<= 32" is not allowed
as "r2" is a packet pointer.
To fix this issue for case
r2 = *(s32 *)(r1 + 76) /* load __sk_buff->data */
this patch added additional checking in is_valid_access() callback
function for packet data/data_end/data_meta access. If those accesses
are with sign-extenstion, the verification will fail.
Tony Ambardar [Tue, 23 Jul 2024 00:30:45 +0000 (17:30 -0700)]
tools/runqslower: Fix LDFLAGS and add LDLIBS support
Actually use previously defined LDFLAGS during build and add support for
LDLIBS to link extra standalone libraries e.g. 'argp' which is not provided
by musl libc.
Tony Ambardar [Sat, 20 Jul 2024 05:25:35 +0000 (22:25 -0700)]
selftests/bpf: Fix wrong binary in Makefile log output
Make log output incorrectly shows 'test_maps' as the binary name for every
'CLNG-BPF' build step, apparently picking up the last value defined for the
$(TRUNNER_BINARY) variable. Update the 'CLANG_BPF_BUILD_RULE' variants to
fix this confusing output.
Fixes: a5d0c26a2784 ("selftests/bpf: Add a cpuv4 test runner for cpu=v4 testing") Fixes: 89ad7420b25c ("selftests/bpf: Drop the need for LLVM's llc") Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: Eduard Zingerman <eddyz87@gmail.com> Link: https://lore.kernel.org/bpf/20240720052535.2185967-1-tony.ambardar@gmail.com
selftests/bpf: Fix compilation failure when CONFIG_NET_FOU!=y
Without CONFIG_NET_FOU bpf selftests are unable to build because of
missing definitions. Add ___local versions of struct bpf_fou_encap and
enum bpf_fou_encap_type to fix the issue.
Tony Ambardar [Tue, 23 Jul 2024 00:13:29 +0000 (17:13 -0700)]
selftests/bpf: Fix error linking uprobe_multi on mips
Linking uprobe_multi.c on mips64el fails due to relocation overflows, when
the GOT entries required exceeds the default maximum. Add a specific CFLAGS
(-mxgot) for uprobe_multi.c on MIPS that allows using a larger GOT and
avoids errors such as:
/tmp/ccBTNQzv.o: in function `bench':
uprobe_multi.c:49:(.text+0x1d7720): relocation truncated to fit: R_MIPS_GOT_DISP against `uprobe_multi_func_08188'
uprobe_multi.c:49:(.text+0x1d7730): relocation truncated to fit: R_MIPS_GOT_DISP against `uprobe_multi_func_08189'
...
collect2: error: ld returned 1 exit status
Tony Ambardar [Tue, 23 Jul 2024 00:13:28 +0000 (17:13 -0700)]
selftests/bpf: Add missing system defines for mips
Update get_sys_includes in Makefile with missing MIPS-related definitions
to fix many, many compilation errors building selftests/bpf. The following
added defines drive conditional logic in system headers for word-size and
endianness selection:
Song Liu [Tue, 23 Jul 2024 05:14:55 +0000 (22:14 -0700)]
selftests/bpf: Add a test for mmap-able map in map
Regular BPF hash map is not mmap-able from user space. However, map-in-map
with outer map of type BPF_MAP_TYPE_HASH_OF_MAPS and mmap-able array as
inner map can perform similar operations as a mmap-able hash map. This
can be used by applications that benefit from fast accesses to some local
data.
Martin KaFai Lau [Tue, 23 Jul 2024 17:45:51 +0000 (10:45 -0700)]
Merge branch 'use network helpers, part 10'
Geliang Tang says:
====================
This set is part 10 of series "use network helpers" all BPF selftests
wide.
Patches 1-3 drop local functions make_client(), make_socket() and
inetaddr_len() in sk_lookup.c. Patch 4 drops a useless function
__start_server() in network_helpers.c.
====================
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
selftests/bpf: Drop __start_server in network_helpers
The helper start_server_addr() is a wrapper of __start_server(), the
only difference between them is __start_server() accepts a sockaddr type
address parameter, but start_server_addr() accepts a sockaddr_storage one.
This patch drops __start_server(), and updates the callers to invoke
start_server_addr() instead.
This patch uses the public network helers client_socket() + make_sockaddr()
in sk_lookup.c to create the client socket, set the timeout sockopts, and
make the connecting address. The local defined function make_socket()
can be dropped then.
This patch uses the new helper connect_to_addr_str() in sk_lookup.c to
create the client socket and connect to the server, instead of using local
defined function make_client(). This local function can be dropped then.
====================
Add BPF LSM return value range check, BPF part
From: Xu Kuohai <xukuohai@huawei.com>
LSM BPF prog may make kernel panic when returning an unexpected value,
such as returning positive value on hook file_alloc_security.
To fix it, series [1] refactored LSM hook return values and added
BPF return value check on top of that. Since the refactoring of LSM
hooks and checking BPF prog return value patches is not closely related,
this series separates BPF-related patches from [1].
selftests/bpf: Add return value checks for failed tests
The return ranges of some bpf lsm test progs can not be deduced by
the verifier accurately. To avoid erroneous rejections, add explicit
return value checks for these progs.
The compiler optimized the two bpf progs in token_lsm.c to make return
value from the bool variable in the "return -1" path, causing an
unexpected rejection:
24: (b4) w0 = -1 ; R0_w=0xffffffff
; int BPF_PROG(test_int_hook, struct vm_area_struct *vma, @ lsm.c:89
25: (95) exit
At program exit the register R0 has smin=4294967295 smax=4294967295 should have been in [-4095, 0]
It can be seen that instruction "w0 = -1" zero extended -1 to 64-bit
register r0, setting both smin and smax values of r0 to 4294967295.
This resulted in a false reject when r0 was checked with range [-4095, 0].
Given bpf lsm does not return 64-bit values, this patch fixes it by changing
the compare between r0 and return range from 64-bit operation to 32-bit
operation for bpf lsm.
bpf: Prevent tail call between progs attached to different hooks
bpf progs can be attached to kernel functions, and the attached functions
can take different parameters or return different return values. If
prog attached to one kernel function tail calls prog attached to another
kernel function, the ctx access or return value verification could be
bypassed.
For example, if prog1 is attached to func1 which takes only 1 parameter
and prog2 is attached to func2 which takes two parameters. Since verifier
assumes the bpf ctx passed to prog2 is constructed based on func2's
prototype, verifier allows prog2 to access the second parameter from
the bpf ctx passed to it. The problem is that verifier does not prevent
prog1 from passing its bpf ctx to prog2 via tail call. In this case,
the bpf ctx passed to prog2 is constructed from func1 instead of func2,
that is, the assumption for ctx access verification is bypassed.
Another example, if BPF LSM prog1 is attached to hook file_alloc_security,
and BPF LSM prog2 is attached to hook bpf_lsm_audit_rule_known. Verifier
knows the return value rules for these two hooks, e.g. it is legal for
bpf_lsm_audit_rule_known to return positive number 1, and it is illegal
for file_alloc_security to return positive number. So verifier allows
prog2 to return positive number 1, but does not allow prog1 to return
positive number. The problem is that verifier does not prevent prog1
from calling prog2 via tail call. In this case, prog2's return value 1
will be used as the return value for prog1's hook file_alloc_security.
That is, the return value rule is bypassed.
This patch adds restriction for tail call to prevent such bypasses.
A bpf prog returning a positive number attached to file_alloc_security
hook makes kernel panic.
This happens because file system can not filter out the positive number
returned by the LSM prog using IS_ERR, and misinterprets this positive
number as a file pointer.
Given that hook file_alloc_security never returned positive number
before the introduction of BPF LSM, and other BPF LSM hooks may
encounter similar issues, this patch adds LSM return value check
in verifier, to ensure no unexpected value is returned.
projects / users / dwmw2 / linux.git / log