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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5064 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
> index ecc3d47919ad..f8497e764beb 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_mod_race.c

[ ... ]

> 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..002eb4920c91
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tramp_prog_detach.c

[ ... ]

> +static void test_detach(bool sleepable_fexit, bool victim_fexit)
> +{
> +	struct tramp_prog_detach *sleepable = NULL, *victim = NULL;
> +	struct pollfd pfd = { .events = POLLIN };
> +	struct uffdio_copy uffd_copy = {};
> +	struct uffd_msg uffd_msg;
> +	void *fault_page, *src_page = MAP_FAILED;
> +	long page_size = getpagesize();
> +	bool started = false;
> +	void *thread_ret;
> +	pthread_t thread;
> +	int uffd = -1;

[ ... ]

> +	sleepable_prog = pick_prog(sleepable, sleepable_fexit, true);
> +
> +	/* Not armed yet so this doesn't block, make sure sleepable runs first */
> +	if (!ASSERT_OK((long)run_sleepable(NULL), "dry run"))
> +		goto out;
> +	if (!ASSERT_LT(sleepable->bss->ts, victim->bss->ts, "prog order"))
> +		goto out;
> +
> +	uffd = uffd_block_page(fault_page);
> +	if (!ASSERT_GE(uffd, 0, "userfaultfd open + register address"))
> +		goto out;

A subsystem pattern flags this as potentially concerning: uffd_block_page()
returns -errno when syscall(__NR_userfaultfd, O_CLOEXEC) fails. On a kernel
without CONFIG_USERFAULTFD that is -ENOSYS, and the test then fails through
ASSERT_GE() instead of calling test__skip() with a message saying userfaultfd
is not available. While tools/testing/selftests/bpf/config sets
CONFIG_USERFAULTFD=y so the BPF CI kernel always has it, and the same
ASSERT_GE pattern already exists in bpf_mod_race.c (serial_test_bpf_mod_race)
from before this commit, is this the intended behavior for environments
without userfaultfd support?

> +	sleepable->bss->fault_addr = fault_page;
> +
> +	if (!ASSERT_OK(pthread_create(&thread, NULL, run_sleepable, NULL),
> +		       "pthread_create"))
> +		goto out;
> +	started = true;
> +
> +	/* Wait for the thread to sleep in bpf_copy_from_user() */
> +	pfd.fd = uffd;
> +	if (!ASSERT_EQ(poll(&pfd, 1, 10000), 1, "poll uffd"))
> +		goto out;
> +	if (!ASSERT_EQ(read(uffd, &uffd_msg, sizeof(uffd_msg)), sizeof(uffd_msg),
> +		       "read uffd"))
> +		goto out;
> +	if (!ASSERT_EQ(uffd_msg.event, UFFD_EVENT_PAGEFAULT, "uffd pagefault"))
> +		goto out;
> +
> +	/* Detach and unload the victim prog, and make sure it is gone */
> +	tramp_prog_detach__destroy(victim);
> +	victim = NULL;
> +	kern_sync_rcu();
> +	usleep(100 * 1000);
> +	kern_sync_rcu();

Does this wait actually ensure the victim prog is gone before the task
resumes?

The test only catches the use-after-free if the victim prog has actually been
freed before UFFDIO_COPY releases the sleeping task. The wait here does not
guarantee that, and on the kernel this series produces it most likely is not
freed yet.

Commit 1c2316cf73ba ("bpf: Wait for an RCU tasks grace period before freeing
trampoline progs") changed __bpf_prog_put_noref() so that a tramp_linked
prog is freed through call_rcu_tasks(&prog->aux->rcu, __bpf_prog_put_rcu_tasks),
then call_rcu(), then bpf_prog_free(), which calls schedule_work() for the
final vfree.

RCU Tasks callbacks are lazy. call_rcu_tasks_generic() only sets needwake
for wakeme_after_rcu (a synchronous waiter) or when the queue reaches
rcu_task_lazy_lim. Otherwise it arms lazy_timer with rcu_tasks.lazy_jiffies
= DIV_ROUND_UP(HZ, 4) (kernel/rcu/tasks.h:145), which is about 250ms by
default.

Nothing on the detach path forces a synchronous RCU Tasks wait. Detaching
the victim while the sleepable prog stays attached goes through
modify_ftrace_direct(), and its tmp_ops is static, so it is not
FTRACE_OPS_FL_DYNAMIC and ftrace_shutdown() does not call
synchronize_rcu_tasks().

The test waits only kern_sync_rcu() + 100ms + kern_sync_rcu().
kern_sync_rcu() is membarrier(MEMBARRIER_CMD_SHARED), which is
synchronize_rcu() on SMP and a no-op with one CPU online. That is well short
of 250ms lazy delay + RCU Tasks GP + RCU GP + workqueue.

So when the thread resumes in the old image, the victim's bpf_prog and JIT
image are usually still allocated. On a kernel that has 1c2316cf73ba but
lacks 2ece3357872c (or breaks it later), the thread would run the victim
prog without a KASAN report and the test would pass.

Consider waiting well beyond the RCU Tasks lazy period (for example about 1s,
or polling until the victim prog ID is no longer returned by
bpf_prog_get_next_id()/bpf_prog_get_fd_by_id()) before releasing the thread.

[ ... ]


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