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

[Florent Revest]

On Sat Sep 12, 2026 at 10:53 AM UTC,  wrote:
> > 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.

Sure, will say "a sleepable prog" in v3.

> > +
> > +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?

Sure why not, I'll try that in v3.