Re: [PATCH bpf-next 5/5] selftests/bpf: add loader_load_fd tests

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 14394 bytes --]

> diff --git a/tools/testing/selftests/bpf/loader_setup.sh b/tools/testing/selftests/bpf/loader_setup.sh
> new file mode 100755
> index 0000000000000..f86651b0718f6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/loader_setup.sh
> @@ -0,0 +1,68 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2026 Google LLC
> +#
> +# loader_setup.sh - Light skeleton data extraction helper for selftests/bpf
> +#
> +# This script parses an autogenerated light skeleton header (.lskel.h) produced
> +# by bpftool ('bpftool gen skeleton -L'). It extracts the embedded loader data
> +# ('opts_data') and loader BPF instructions ('opts_insn') string literals and
> +# generates a C header file containing:
> +#   - loader_test_opts_data[]: Data for the '__loader.map' section.
> +#   - loader_test_opts_data_sz: Size of opts_data.
> +#   - loader_test_opts_insn[]: Instructions for the '__loader.prog' section.
> +#   - loader_test_opts_insn_sz: Size of opts_insn.

The comment documents loader_test_opts_data[], loader_test_opts_data_sz,
loader_test_opts_insn[], and loader_test_opts_insn_sz, but the awk body
below emits test_loader_opts_data, test_loader_opts_data_sz,
test_loader_opts_insn and test_loader_opts_insn_sz. Could the comment use
the actual symbol names?

[ ... ]

> +/opts_data\[\]/ {
> +	in_data = 1
> +	sub(/^.*opts_data\[\][^="]*=\s*"/, "")
> +	printf "static const char test_loader_opts_data[] __attribute__((__aligned__(8))) = \""
> +}

Does the awk sub() regex work with mawk or busybox awk? mawk is the
default awk on Debian and Ubuntu, and Documentation/process/changes.rst
lists GNU awk as optional. The \s escape (here and again below in the
opts_insn block) is a GNU awk extension that mawk does not implement.

In mawk, \s matches a literal 's', so the regex =\s*" cannot match
bpftool's output = ", and the sub() fails silently. When the prefix is
not stripped, the emitted line becomes two adjacent string literals
followed by a bare hex-escape token outside any string:

  static const char test_loader_opts_data[] = "<TAB>static const char opts_data[] = "\
  \x7f\x45\x4c\x46...";"

That is not valid C. This breaks the entire `make -C
tools/testing/selftests/bpf` build because Makefile:580 adds
test_loader_processed.lskel.h to TRUNNER_EXTRA_HDRS for every runner, not
just the loader_load_fd test.

The portable form used across the kernel tree is [[:space:]] - see
scripts/syscallnr.sh, scripts/syscalltbl.sh, scripts/headers_install.sh,
and scripts/tags.sh. Code that genuinely needs GNU awk calls gawk
explicitly.

> +/opts_insn\[\]/ {
> +	in_insn = 1
> +	sub(/^.*opts_insn\[\][^="]*=\s*"/, "")
> +	printf "static const char test_loader_opts_insn[] __attribute__((__aligned__(8))) = \""
> +}

Same \s issue here.

> diff --git a/tools/testing/selftests/bpf/prog_tests/loader_load_fd.c b/tools/testing/selftests/bpf/prog_tests/loader_load_fd.c
> new file mode 100644
> index 0000000000000..ab971662dd047
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/loader_load_fd.c

[ ... ]

> +static int create_loader_elf(const void *insns, size_t insns_sz,
> +			     const void *map_data, size_t map_data_sz,
> +			     const char *license, size_t license_sz,
> +			     bool omit_prog, bool omit_map, bool omit_license)
> +{

[ ... ]

> +	ehdr->e_ident[EI_MAG0] = ELFMAG0;
> +	ehdr->e_ident[EI_MAG1] = ELFMAG1;
> +	ehdr->e_ident[EI_MAG2] = ELFMAG2;
> +	ehdr->e_ident[EI_MAG3] = ELFMAG3;
> +	ehdr->e_ident[EI_CLASS] = ELFCLASS64;
> +	ehdr->e_ident[EI_DATA] = ELFDATA2LSB;
> +	ehdr->e_ident[EI_VERSION] = EV_CURRENT;
> +	ehdr->e_machine = EM_BPF;
> +	ehdr->e_type = ET_REL;

Does hardcoding ELFDATA2LSB work on big-endian hosts?

libelf converts the in-memory structures to the encoding named in
e_ident[EI_DATA] when elf_update() writes the file. With EI_DATA forced
to ELFDATA2LSB, a big-endian host emits byte-swapped headers.

The kernel side parses those headers with native loads and never inspects
e_ident[EI_DATA]:

  static int bpf_elf_validity_ehdr(const struct elf_info *info)
  {
          ...
          if (info->hdr->e_type != ET_REL) {
                  pr_err("Invalid ELF header type: %u != %u\n", ...);
                  return -ENOEXEC;
          }

On s390x, ET_REL (1) stored little-endian reads back as 0x0100 == 256, so
loader_load_fd() returns -ENOEXEC. The missing_prog_sec and
missing_map_sec subtests assert EINVAL (not ENOEXEC), so they would fail
against a correct kernel on big-endian hardware.

The section payloads are handed to elf_newdata() as ELF_T_BYTE and copied
in host order, while the headers around them get swapped to LSB. Since the
kernel consumes the file in host byte order, does EI_DATA need to come
from __BYTE_ORDER__? tools/lib/bpf/linker.c and usdt.c use this pattern:

  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
          const unsigned char host_byteorder = ELFDATA2LSB;
  #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
          const unsigned char host_byteorder = ELFDATA2MSB;
  #else
  #error "Unknown __BYTE_ORDER__"
  #endif

[ ... ]

> +static void test_loader_load_fd_oversized_ctx(void)
> +{
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	};
> +	char map_data[] = "data";
> +	char license[] = "GPL";
> +	struct bpf_loader_ctx ctx = {};
> +	int elf_fd, err;
> +
> +	elf_fd = create_loader_elf(insns, sizeof(insns),
> +				   map_data, sizeof(map_data),
> +				   license, sizeof(license),
> +				   false, false, false);
> +
> +	if (!ASSERT_GE(elf_fd, 0, "create_loader_elf"))
> +		return;
> +
> +	err = sys_bpf_loader_load_fd(elf_fd, &ctx, 65536U);
> +	ASSERT_EQ(err, -1, "oversized ctx sys_bpf return");
> +	ASSERT_EQ(errno, EINVAL, "oversized ctx errno");

A subsystem pattern flags this as potentially concerning: the subtest
asserts EINVAL for ctx_size == 65536, targeting loader_load_fd()'s guard

  if (attr->load_fd.ctx_size > U16_MAX)
          return -EINVAL;

but EINVAL is also produced further down the same path. With the guard
removed, the request reaches bpf_prog_test_run_syscall(), which
independently rejects the same value:

  if (ctx_size_in < prog->aux->max_ctx_offset ||
      ctx_size_in > U16_MAX)
          return -EINVAL;

errno is EINVAL either way, so the subtest passes whether or not
loader_load_fd() validates ctx_size. Could the test assert on a value that
only loader_load_fd() rejects, or check the ordering explicitly (e.g. pass
an oversized ctx_size together with an invalid loader_fd and confirm
EINVAL still comes back before any ELF is read)?

Also: ctx is a 24-byte struct on the stack, and ctx_size is 65536.
loader_load_fd() copies ctx_size bytes back on success:

  if (copy_to_user((void *) attr->load_fd.ctx, kctx, ctx_sz) != 0)
          err = -EFAULT;

The only thing standing between this and a 64 KiB stack overwrite is the
check the subtest is meant to verify. If that check regresses, the
subtest's failure mode is stack corruption in test_progs instead of a
clean assertion failure. Would a 65536-byte heap buffer make the bad-kernel
case report cleanly?

> +static void test_loader_load_fd_invalid_elf(void)
> +{
> +	char garbage[] = "not_an_elf_file_content";
> +	struct bpf_loader_ctx ctx = {};
> +	int fd, err;
> +
> +	fd = memfd_create("garbage_file", 0);
> +	if (!ASSERT_GE(fd, 0, "memfd_create"))
> +		return;
> +
> +	if (!ASSERT_EQ(write(fd, garbage, sizeof(garbage)), sizeof(garbage), "write garbage")) {
> +		close(fd);
> +		return;
> +	}
> +	lseek(fd, 0, SEEK_SET);
> +
> +	err = sys_bpf_loader_load_fd(fd, &ctx, sizeof(ctx));
> +	ASSERT_EQ(err, -1, "invalid elf sys_bpf return");
> +	ASSERT_EQ(errno, ENOEXEC, "invalid elf errno");

A subsystem pattern flags this as potentially concerning: the subtest
writes 24 bytes and asserts ENOEXEC, but that input never reaches the
magic-number check it appears to target. bpf_elf_validity_ehdr() bails out
on the length test first:

  if (info->len < sizeof(*(info->hdr))) {
          pr_err("Invalid ELF header len %lu\n", info->len);
          return -ENOEXEC;
  }
  if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0) {
          pr_err("Invalid ELF header magic: != %s\n", ELFMAG);
          return -ENOEXEC;
  }

sizeof(Elf64_Ehdr) is 64, so with 24 bytes the memcmp() is unreachable.
Would padding the input to at least 64 bytes make the subtest exercise the
magic check as its name implies?

> +static void test_loader_load_fd_missing_license_sec(void)
> +{
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	};
> +	char map_data[] = "data";
> +	char license[] = "GPL";
> +	struct bpf_loader_ctx ctx = {};
> +	int elf_fd, err;
> +
> +	elf_fd = create_loader_elf(insns, sizeof(insns),
> +				   map_data, sizeof(map_data),
> +				   license, sizeof(license),
> +				   false, false, true);
> +	if (!ASSERT_GE(elf_fd, 0, "create_loader_elf"))
> +		return;
> +
> +	err = sys_bpf_loader_load_fd(elf_fd, &ctx, sizeof(ctx));
> +	ASSERT_EQ(err, -1, "missing license sec sys_bpf return");
> +	ASSERT_EQ(errno, EINVAL, "missing license sec errno");

A subsystem pattern flags this as potentially concerning: the subtest
asserts errno == EINVAL for an ELF that has no license section. EINVAL is
also what the bpf(2) syscall returns for a command it does not know - in
__sys_bpf() the switch ends with `default: err = -EINVAL;`. So this
assertion is satisfied both by a kernel that correctly rejects the
malformed loader ELF and by a kernel that has never heard of
BPF_LOADER_LOAD_FD at all.

The same is true of invalid_fd, oversized_ctx, missing_prog_sec, and
missing_map_sec, so 5 of the 8 subtests pass unchanged on a kernel without
this patch series. Only invalid_elf (ENOEXEC) and lskel genuinely
discriminate. Could a feature probe up front (e.g. issue one
BPF_LOADER_LOAD_FD call that must not fail with EINVAL-for-unknown-command,
or test__skip() when the well-formed lskel path returns EINVAL) keep
unknown-command kernels from silently passing?

> +static void test_loader_load_fd_loader_failure(void)
> +{
> +	struct bpf_insn insns[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, -EPERM),
> +		BPF_EXIT_INSN(),
> +	};
> +	char map_data[] = "data";
> +	char license[] = "GPL";
> +	struct bpf_loader_ctx ctx = {};
> +	int elf_fd, err;
> +
> +	elf_fd = create_loader_elf(insns, sizeof(insns),
> +				   map_data, sizeof(map_data),
> +				   license, sizeof(license),
> +				   false, false, false);
> +	if (!ASSERT_GE(elf_fd, 0, "create_loader_elf"))
> +		return;
> +
> +	err = sys_bpf_loader_load_fd(elf_fd, &ctx, sizeof(ctx));
> +	ASSERT_EQ(err, -1, "loader failure sys_bpf return");
> +	ASSERT_EQ(errno, EPERM, "loader failure errno");

A subsystem pattern flags this as potentially concerning: the subtest
builds a loader program that returns -EPERM and asserts errno == EPERM to
prove that loader_load_fd() propagates the loader program's return value.
But EPERM is also the first thing loader_load_fd() can return:

  if (!capable(CAP_BPF))
          return -EPERM;

That fires before the fd is even resolved. So the assertion is equally
satisfied by a kernel that never ran the loader program at all. Would using
a distinctive errno for the loader's return value (e.g. -E2BIG or
-EBADMSG, something loader_load_fd() itself never produces) make the check
discriminating?

[ ... ]

> +struct test_loader_lskel {
> +	struct bpf_loader_ctx ctx;
> +	struct {
> +		struct bpf_map_desc test_map;
> +	} maps;
> +	struct {
> +		struct bpf_prog_desc probe;
> +	} progs;
> +	struct {
> +		int probe_fd;
> +	} links;
> +};
> +
> +static void test_loader_load_fd_lskel(void)
> +{
> +	struct test_loader_lskel skel = {};
> +	int elf_fd, err;
> +
> +	skel.ctx.sz = (char *)&skel.links - (char *)&skel;
> +
> +	/* Build fake ELF using extracted opts_insn (__loader.prog) and opts_data (__loader.map) */
> +	elf_fd = create_loader_elf(test_loader_opts_insn, test_loader_opts_insn_sz,
> +				   test_loader_opts_data, test_loader_opts_data_sz,
> +				   "GPL", sizeof("GPL"),
> +				   false, false, false);
> +	if (!ASSERT_GE(elf_fd, 0, "create_loader_elf_lskel"))
> +		return;
> +
> +	err = sys_bpf_loader_load_fd(elf_fd, &skel.ctx, skel.ctx.sz);
> +	ASSERT_OK(err, "sys_bpf_loader_load_fd lskel");

Does this cover loader context copy-in?

progs/test_loader.c is the only BPF object the lskel subtest exercises,
and it is deliberately minimal: one ARRAY map with a compile-time
max_entries and no global variables, so the generated object has no
.rodata/.data/.bss map.

That makes the subtest blind to the place where the kernel-side
loader_load_fd() diverges from userspace bpf_load_and_run(): the kernel
does not copy the caller's loader context in, it allocates a fresh zeroed
one:

  kctx = kzalloc(ctx_sz, GFP_KERNEL);
  ...
  sattr.test.ctx_in = (unsigned long) kctx;

There is no copy_from_user() of attr->load_fd.ctx. The generated loader
program reads two caller-supplied ctx inputs per map via move_ctx2blob():
struct bpf_map_desc::max_entries and struct bpf_map_desc::initial_value.
With a zeroed ctx, initial_value is 0 and the copy is silently skipped, so
any real light skeleton with global/const data would come up with an
all-zero .rodata/.data map instead of its initialised contents. Because
test_loader.c has no such map, the subtest passes either way.

Would adding a global variable to progs/test_loader.c (e.g. const volatile
__u64 expected = 0x1234; plus a check on it) turn this into a covered case?

> diff --git a/tools/testing/selftests/bpf/progs/test_loader.c b/tools/testing/selftests/bpf/progs/test_loader.c
> new file mode 100644
> index 0000000000000..515e15ca042ec
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_loader.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 4);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} test_map SEC(".maps");
> +
> +SEC("socket")
> +int probe(void *ctx)
> +{
> +	__u32 key = 0;
> +	__u64 *val = bpf_map_lookup_elem(&test_map, &key);
> +
> +	return val ? (int)*val : 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31655295294