Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] selftests/bpf: module_get_base_addr() desyncs on trailing /proc/modules tokens - [Medium] bpf: btf_inline test incorrectly allows memory-spilled parameter --
commit 4adb006744e35941ca2b357431675f3202d1100f Author: Alan Maguire <[email protected]> Subject: selftests/bpf: Add a test verifying inline information This commit adds a selftest to verify inline information for bpf_testmod. It checks that an always-inlined function has a single location parameter encoded in a register, and that the LOCSEC descriptor offset is within the expected range. > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_inline.c > b/tools/testing/selftests/bpf/prog_tests/btf_inline.c > new file mode 100644 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/btf_inline.c [ ... ] > + lp = btf_loc_param(t); > + if (!ASSERT_TRUE(lp->flags & BTF_LOC_PARAM_REG, "param_has_reg")) > + goto out; [Severity: Medium] Because the variable in testmod_inline_len() below is forced into memory, its BTF location is encoded as a register-relative memory offset, which sets both BTF_LOC_PARAM_REG and BTF_LOC_PARAM_OFFSET. Does this bitwise AND accidentally pass for memory offsets since they have a base register? If so, this check might be masking the fact that the parameter was actually spilled to the stack instead of being purely encoded in a register as the commit message describes. [ ... ] > diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c > b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c > index 93847ca6293b4..b4f0af2aeca45 100644 > --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c > +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c [ ... ] > @@ -735,6 +735,13 @@ bpf_testmod_test_read(struct file *file, struct kobject > *kobj, > EXPORT_SYMBOL(bpf_testmod_test_read); > ALLOW_ERROR_INJECTION(bpf_testmod_test_read, ERRNO); > > +static __always_inline size_t testmod_inline_len(size_t len) > +{ > + /* Keep an instruction in the inline site without changing len. */ > + asm volatile("" : "+m"(len)); [Severity: Medium] Does the "+m" constraint here force the len variable into memory rather than a register? The commit message explicitly states the intent is to verify a parameter "encoded in a register", but this constraint forces a stack spill instead. > + return len; > +} [ ... ] > diff --git a/tools/testing/selftests/bpf/trace_helpers.c > b/tools/testing/selftests/bpf/trace_helpers.c > index 679008b310d9a..f3a1d627699da 100644 > --- a/tools/testing/selftests/bpf/trace_helpers.c > +++ b/tools/testing/selftests/bpf/trace_helpers.c [ ... ] > +unsigned long long module_get_base_addr(const char *module) > +{ > + unsigned long long addr, base_addr = 0; > + char name[500]; > + FILE *f; > + > + f = fopen("/proc/modules", "r"); > + if (!f) > + return 0; > + > + while (fscanf(f, "%499s %*s %*s %*s %*s 0x%llx", name, &addr) == 2) { [Severity: Medium] Will this loop terminate prematurely if it encounters trailing data on a line in /proc/modules? If a module (like an out-of-tree or unsigned module) has an optional trailing taint string such as "(O)", this fscanf format lacks a line consumer (like %*[^\n]\n) to read to the end of the line. On the next iteration, the %499s token will mistakenly consume the leftover taint string instead of the next module's name, shifting all subsequent fields by one token. When the 0x%llx field then attempts to parse the state string (e.g., "Live"), it will fail to match the "0x" prefix and return 1, causing the loop to exit early and return 0 instead of the correct base address. > + if (strcmp(module, name) == 0) { > + base_addr = addr; > + break; > + } > + } > + fclose(f); > + return base_addr; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
