Re: [RFC PATCH v2 3/3] selftests: prctl: add process-wide bounding-set drop tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests: prctl: test fails to verify concurrent thread creation due to synchronous pthread_join
- [Medium] selftests: prctl: deadlock on drop_cap() error path in conc_test_child()
- [Medium] selftests: prctl: uninitialized thread ID passed to pthread_join() on pthread_create() failure
- [Medium] selftests: prctl: unmaterialized drop inheritance test is nullified by syscall before fork
--

commit efe86d5c1a9f1d9daea13c0d844c2621fc972d8d
Author: Jinjie Ruan <ruanjinjie@huawei.com>
Subject: selftests: prctl: add process-wide bounding-set drop tests

This commit adds tests for PR_CAPBSET_DROP_MASK. It covers argument validation,
permission checking, and process-wide application of the bounding set drop
across sibling threads and forked children.

> diff --git a/tools/testing/selftests/prctl/cap-bset-drop-test.c b/tools/testing/selftests/prctl/cap-bset-drop-test.c
> new file mode 100644
> index 0000000000000..d0432f9b58377
> --- /dev/null
> +++ b/tools/testing/selftests/prctl/cap-bset-drop-test.c

[ ... ]

> +static void *fork_sibling(void *arg)
> +{
> +	pid_t pid;
> +	int status;
> +
> +	(void)arg;
> +
> +	while (!atomic_load(&fork_go))
> +		sched_yield();
> +
> +	if (bset_has(dropped_cap))
> +		atomic_store(&fork_ok, 0);
> +
> +	pid = fork();

[Severity: Medium]
Does calling bset_has() here defeat the purpose of the test? bset_has() invokes
prctl(), which is a syscall. Returning to userspace from this syscall forces
pending task work (TIF_NOTIFY_RESUME) to be processed, fully materializing the
credentials before fork() occurs. This nullifies the intent of verifying that an
unmaterialized drop is properly inherited by a child thread.

> +	if (pid == 0)
> +		_exit(bset_has(dropped_cap) ? 1 : 0);

[ ... ]

> +static void *conc_spawner(void *arg)
> +{
> +	(void)arg;
> +
> +	while (!atomic_load(&conc_stop)) {
> +		pthread_t t;
> +
> +		if (pthread_create(&t, NULL, conc_worker, NULL) == 0)
> +			pthread_join(t, NULL);

[Severity: Medium]
Does this synchronous wait serialize thread creation? Calling pthread_join()
immediately after pthread_create() blocks the spawner thread until the spawned
worker unblocks. As a result, no new threads are actively created during the
capability drop, nullifying the concurrent thread creation test.

> +	}
> +	return NULL;
> +}
> +
> +static int conc_test_child(void)
> +{
> +	pthread_t sp[4];
> +	int i;
> +
> +	atomic_store(&conc_stop, 0);
> +	atomic_store(&conc_dropped, 0);
> +	atomic_store(&conc_bad, 0);
> +
> +	for (i = 0; i < 4; i++) {
> +		if (pthread_create(&sp[i], NULL, conc_spawner, NULL))
> +			return CHILD_FAIL;
> +	}
> +
> +	if (drop_cap(dropped_cap)) {
> +		atomic_store(&conc_stop, 1);
> +		for (i = 0; i < 4; i++)
> +			pthread_join(sp[i], NULL);
> +		return CHILD_FAIL;
> +	}

[Severity: Medium]
Can this error path lead to an infinite hang? If drop_cap() fails, conc_dropped
is never set to 1. The worker threads will spin infinitely waiting for it to
change. This in turn causes the spawner threads to block infinitely in
pthread_join() waiting for the workers, and the main thread blocks infinitely
here waiting for the spawners.

> +	atomic_store(&conc_dropped, 1);
> +	usleep(20000);

[ ... ]

> +static void *multi_spawner(void *arg)
> +{
> +	(void)arg;
> +
> +	while (!atomic_load(&multi_done)) {
> +		pthread_t t;
> +
> +		if (pthread_create(&t, NULL, multi_reader, NULL) == 0)
> +			pthread_join(t, NULL);

[Severity: Medium]
Similar to the issue in conc_spawner(), does this synchronous wait prevent the
intended concurrency test? Waiting on the worker prevents thread creation races
with the concurrent capability drops.

> +	}
> +	return NULL;
> +}

[ ... ]

> +static int multi_drop_test_child(void)
> +{
> +	pthread_t dr[MULTI_DROP], sp[4], late;
> +	int i;

[ ... ]

> +	if (multi_bset_has_any()) /* the calling thread itself */
> +		return CHILD_FAIL;
> +	if (atomic_load(&multi_bad))
> +		return CHILD_FAIL;
> +
> +	pthread_create(&late, NULL, multi_reader, NULL);
> +	pthread_join(late, NULL);

[Severity: Medium]
Is it safe to unconditionally join this thread? If pthread_create() fails, late
remains uninitialized. Passing an uninitialized thread ID to pthread_join()
causes undefined behavior and could result in a crash or hang.

> +	if (atomic_load(&multi_bad))
> +		return CHILD_FAIL;
> +
> +	return CHILD_PASS;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929130200.1638343-1-ruanjinjie@huawei.com?part=3