Re: [PATCH bpf v2 2/2] selftests/bpf: Detach a trampoline prog while a task sleeps before it

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/tramp_prog_detach.c b/tools/testing/selftests/bpf/prog_tests/tramp_prog_detach.c
> new file mode 100644
> index 000000000000..9a0e1fc44c74
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tramp_prog_detach.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include <pthread.h>
> +#include <poll.h>
> +#include <sys/mman.h>
> +#include <sys/syscall.h>
> +#include <linux/userfaultfd.h>
> +#include "tramp_prog_detach.skel.h"
> +#include "testing_helpers.h"
> +
> +/*
> + * Detach and free a prog while a task sleeps in the prog that runs before it
> + * in the same trampoline image, then let that task continue through the
> + * image. It must not call into the freed prog.
> + *
> + * The task is held in a sleepable fentry prog with userfaultfd, like
> + * bpf_mod_race does.
> + */

This isn't a bug, but does "a sleepable fentry prog" still cover both
subtests?  serial_test_tramp_prog_detach() runs test_detach(false) and
test_detach(true), and the second one holds the task in the prog from
SEC("?fexit.s/bpf_fentry_test1").

Would dropping "fentry" here, so it just reads "a sleepable prog", match
what the file does now?  The per-subtest comments in
serial_test_tramp_prog_detach() already spell out the fentry and fexit
cases.

> +
> +static int test_setup_uffd(void *fault_addr)
> +{
> +	struct uffdio_register uffd_register = {};
> +	struct uffdio_api uffd_api = {};
> +	int uffd;
> +
> +	uffd = syscall(__NR_userfaultfd, O_CLOEXEC);
> +	if (uffd < 0)
> +		return -errno;
> +
> +	uffd_api.api = UFFD_API;
> +	uffd_api.features = 0;
> +	if (ioctl(uffd, UFFDIO_API, &uffd_api)) {
> +		close(uffd);
> +		return -1;
> +	}
> +
> +	uffd_register.range.start = (unsigned long)fault_addr;
> +	uffd_register.range.len = getpagesize();
> +	uffd_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> +	if (ioctl(uffd, UFFDIO_REGISTER, &uffd_register)) {
> +		close(uffd);
> +		return -1;
> +	}
> +	return uffd;
> +}

This isn't a bug either, but this test_setup_uffd() looks like the
same helper that already lives in prog_tests/bpf_mod_race.c: same
name, same uffd_register/uffd_api/uffd locals, same UFFDIO_API then
UFFDIO_REGISTER order, and the same close()/return -1 handling.  The
only difference is that bpf_mod_race.c goes through its local
sys_userfaultfd() wrapper instead of calling
syscall(__NR_userfaultfd, O_CLOEXEC) directly.

There is no shared userfaultfd helper in testing_helpers.h today, so
the two copies would have to be kept in sync by hand.  Would it be
worth moving it into testing_helpers.c so both tests share one copy?

[ ... ]

> +void serial_test_tramp_prog_detach(void)
> +{
> +	/* a task sleeping before the original function is called */
> +	if (test__start_subtest("fentry"))
> +		test_detach(false);
> +	/* a task sleeping after it returned, past the jmp that detach installs */
> +	if (test__start_subtest("fexit"))
> +		test_detach(true);
> +}


---
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/34688142088