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

[email protected]
Newsgroups org.kernel.vger.selinux,org.kernel.vger.bpf,org.kernel.vger.linux-kernel
Message-ID <b8b60c3d613617b1053002d7cd5f0bbbc2d1a18f195ddd8bfdb239b3770cdf84@mail.kernel.org>
> 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.