Re: [LTP] semctl01: look up SEM_STAT index for this test's sem_id

[linuxtestproject.agent@gmail.com]

Hi Stephen,

On Jul 28, 2026, Stephen Bertram wrote:
> semctl01: look up SEM_STAT index for this test's sem_id

> Walk 0..max_idx with SEM_STAT until the returned id equals this test's
> sem_id.

The body describes what the code does, but not why it is needed. Could it
state the root cause instead, e.g. that IPC_INFO returns the globally
highest used index, which may belong to another process's semaphore set,
so SAFE_SEMCTL(sem_index, 0, SEM_STAT, ...) can fail with EINVAL/EIDRM/
EACCES and abort the test with TBROK when IPC tests run in parallel?

> Test: ./kirk -w 4 -f syscalls_8 -p semctl01 -i 1000
>
> Results summary before:
[...]

This is test-run metadata rather than part of the change log. Would it be
better placed below the "---" line?

> +static int sem_stat_for_id(int id, union semun arg)
> +{
> +	struct seminfo info;
> +	union semun info_arg;
> +	int max_idx, i, ret;
> +
> +	info_arg.__buf = &info;
> +	max_idx = SAFE_SEMCTL(id, 0, IPC_INFO, info_arg);
> +
> +	for (i = 0; i <= max_idx; i++) {
> +		ret = semctl(i, 0, SEM_STAT, arg);
> +		if (ret == id)
> +			return ret;

The helper returns the semid, not the index, and it only returns when
ret == id. Combined with do_sem_stat() aborting otherwise, the value
reaching func_sstat() is always sem_id.

> +static int do_sem_stat(union semun arg)
> +{
> +	int ret;
> +
> +	ret = sem_stat_for_id(sem_id, arg);
> +	if (ret < 0)
> +		tst_brk(TBROK, "Failed to find SEM_STAT for sem_id %d",
> +			sem_id);
> +
> +	return ret;
> +}

>  static void func_sstat(int semidx)
>  {
> -	if (semidx >= 0)
> +	if (semidx == sem_id)
>  		tst_res(TPASS, "id of the semaphore set is correct");
>  	else
> -		tst_res(TFAIL, "id of the semaphore set is incorrect");
> +		tst_res(TFAIL, "expected sem_id %d, got %d", sem_id, semidx);
>  }

Doesn't this make the check tautological? semidx == sem_id holds by
construction, so the TFAIL branch is unreachable and the SEM_STAT test
case no longer asserts anything about the syscall result. A kernel
returning a wrong id would now show up as TBROK ("Failed to find
SEM_STAT for sem_id") rather than TFAIL.

testcases/kernel/syscalls/shmctl/shmctl01.c already solves the same
problem with get_shm_idx_from_id(), which returns the *index*:

	static int get_shm_idx_from_id(int shm_id)
	{
		struct shm_info dummy;
		struct shmid_ds dummy_ds;
		int max_idx, i;

		max_idx = SAFE_SHMCTL(shm_id, SHM_INFO, (void *)&dummy);

		for (i = 0; i <= max_idx; i++) {
			if (shmctl(i, SHM_STAT, &dummy_ds) == shm_id)
				return i;
		}

		return -1;
	}

Following that shape here would keep the assertion intact: resolve
sem_idx, then keep SAFE_SEMCTL(sem_idx, 0, SEM_STAT, tc->arg) in
verify_semctl() so func_sstat() compares an actual syscall return value
against sem_id.

It would also collapse sem_stat_for_id() and do_sem_stat() into a single
helper. As it stands, do_sem_stat() is a one-line wrapper and
sem_stat_for_id() is only ever called with the global sem_id, so the
"id" parameter adds no flexibility.

> +		if (ret == -1 && errno != EIDRM && errno != EINVAL &&
> +			errno != EACCES)
> +			tst_brk(TBROK | TERRNO, "semctl(%d, SEM_STAT)", i);

This aborts on any errno outside the allow-list while probing IPC objects
owned by other processes, which is a new source of TBROK. shmctl01
ignores every failure in the scan loop and simply continues. Is there a
reason to be stricter here?

> +/*
> + * SEM_STAT takes an idr index. Walk 0..max_idx until SEM_STAT
> + * returns this test's sem_id so parallel IPC tests cannot make us exercise
> + * some other process's set (EIDRM/EINVAL/EACCES on a moving max_idx).
> + * Returns that semid.
> + */

The parenthetical looks inaccurate. Per ipc/sem.c semctl_stat(), EINVAL
comes from an unused index, EIDRM from a set removed concurrently, and
EACCES from a set the caller cannot read - none of them from "a moving
max_idx".

Also, semctl(2) describes the argument as an index into the kernel's
internal array; "idr index" is kernel-internal terminology.

>  	if (sem_id == -1)
>  		sem_id = SAFE_SEMGET(IPC_PRIVATE, PSEMS, IPC_CREAT | IPC_EXCL | SEM_RA);
> +
>  	if (tc->func_setup) {

This blank line is unrelated to the change. Could it be dropped to keep
the patch to one logical change?

Verdict - Needs revision

Pre-existing issues, not introduced by this patch:

- func_iinfo(): "hidx >= 0" is always true, since SAFE_SEMCTL() already
  aborts when semctl() returns -1.

- cleanup(): "if (sem_id >= 0)" rather than the "!= -1" convention used
  for the initialiser.

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp