From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B196A328B7B; Thu, 13 Aug 2026 01:25:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786584324; cv=none; b=NfrwPUfWoQC4tlcxTBpl47kPYXIFApnZnt3k0J+mjcK3seWsEm+C0eCnWYIRpK7LHZnx4OJm62jNdPKkcGZ11tpViPOvzkVcF98EbQaQA+W9rgj29byD1LErCGVLDo1wBp5uZ6QwjXRK35rdYmNkXfzO3DV5vPQk9YcX7aclHgQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1786584324; c=relaxed/simple; bh=V8QjiYyjZJSJPtDMTJIF3lAgXLrjnyq0YJac8qpTtP0=; h=Content-Type:MIME-Version:Message-Id:In-Reply-To:References: Subject:From:To:Cc:Date; b=iFNv5Zst+iJsM830rVOTTBNvF7Di4M2ZW/HW6Fc7/SNTpf3sN9ixTmODcOg9Hy01ygKJ7QpFKFYcMwTzF4p2GD14vExVnL6sX3ipjOs3xU+SgHzpl8QBqrxrhCyION6dHK+w7ZIciN3Muk8mFeKJAxRtWikfxGr6T/H/XSEV3ho= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ZIdPjNQL; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ZIdPjNQL" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5F24B1F000E9; Thu, 13 Aug 2026 01:25:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1786584322; bh=zwn+VeHDfLFGdLX/+Sf8Stf93zux/4iZdFx/CRW3xjA=; h=In-Reply-To:References:Subject:From:To:Cc:Date; b=ZIdPjNQLLcpayx/WHsDYiZCfIfb0AWV3xFH7nXLDK9cF4rcNpdfcXJCA08pmy2vm9 oeuH/gKrhdXz6uN5lZu1GKE3Zb5KynH7KEInRFRKkuAp8ldqf3LD3mNrevISAAhC/y Xxr9RrbmqLLyS4ZxbtBQQGK+HsWM23BEpgPuTakfoZpajBJ3OXNi9V39AY3EBF8eWc UlONoyp0lB0rGCgCxKkyaJfMh7UKxdbZLrpN+CO8sA+k5B7J6gWzhbgtLKhAFShaI3 VEseK8FPdwYEK9MFnRcLJlodn71N/xsDfGdkLpCKXbIA52COpZ7xSEl/T3uM5qzoSH 7aLies0sv6JLw== Content-Type: multipart/mixed; boundary="===============8749948781067238914==" Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: In-Reply-To: <20260813002618.3755631-6-tweek@google.com> References: <20260813002618.3755631-6-tweek@google.com> Subject: Re: [PATCH bpf-next 5/5] selftests/bpf: add loader_load_fd tests From: bot+bpf-ci@kernel.org To: tweek@google.com,paul@paul-moore.com,stephen.smalley.work@gmail.com,ast@kernel.org,daniel@iogearbox.net,andrii@kernel.org,jeffv@google.com Cc: tweek@google.com,omosnace@redhat.com,ericsu@linux.microsoft.com,bboscaccy@linux.microsoft.com,sidnayyar@google.com,nkapron@google.com,ebiggers@google.com,gregkh@linuxfoundation.org,kpsingh@kernel.org,bpf@vger.kernel.org,selinux@vger.kernel.org,linux-kernel@vger.kernel.org,ast@kernel.org,andrii@kernel.org,daniel@iogearbox.net,martin.lau@kernel.org,eddyz87@gmail.com,yonghong.song@linux.dev,clm@meta.com,ihor.solodrai@linux.dev Date: Thu, 13 Aug 2026 01:25:21 +0000 (UTC) --===============8749948781067238914== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > 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[] = "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 > + > +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 --===============8749948781067238914==--