Re: [LTP] syscalls/listmount04: Detect backported commit 78f0e33cd6c9 dynamically

[linuxtestproject.agent@gmail.com]

Hi Darren,

Thank you for the patch. The overall approach — probing the kernel at runtime
instead of relying solely on tst_kvercmp() — is the right fix for backport
scenarios. Two issues need to be addressed before this can be accepted.

---

[PATCH] syscalls/listmount04: Detect backported commit 78f0e33cd6c9 dynamically

> +	/* 2. Runtime probe for older kernels:
> +	 * Check if commit 78f0e33cd6c9 was backported by manually issuing
> +	 * a syscall with an invalid mnt_ns_fd (-1).
> +	 */
> +	mnt_id_req probe_req = {
> +		.size = MNT_ID_REQ_SIZE_VER0,
> +		.mnt_id = LSMT_ROOT,
> +		.mnt_ns_fd = -1,
> +		.param = 0
> +	};
> +
> +	tst_syscall(__NR_listmount, &probe_req, mnt_ids, MNT_SIZE, 0);
> +
> +	/* 3. Set global behavior flag based on actual kernel response */
> +	if (errno == EBADF) {
> +		tst_res(TINFO, "Detected backported commit 78f0e33cd6c9 (returns EBADF)");
> +		current_fd_err_behavior = INVALID_FD_RET_EBADF;
> +	} else {
> +		current_fd_err_behavior = INVALID_FD_RET_EINVAL;
> +	}

>>> The `else` branch silently maps *any* unexpected errno to
>>> INVALID_FD_RET_EINVAL. If the probe returns something other than EBADF
>>> or EINVAL (e.g. EPERM, ENOMEM, or a hypothetical kernel bug), the test
>>> will silently classify the kernel as "pre-backport" and run the EINVAL
>>> test case against wrong expectations, yielding a false FAIL or a false
>>> PASS.
>>>
>>> tst_syscall() already handles ENOSYS (it calls tst_brk()), but all other
>>> unexpected errnos fall through to this else branch unchecked.
>>>
>>> Please make the check explicit and abort on unexpected values:
>>>
>>>     if (errno == EBADF) {
>>>         tst_res(TINFO, "Detected backported commit 78f0e33cd6c9 (returns EBADF)");
>>>         current_fd_err_behavior = INVALID_FD_RET_EBADF;
>>>     } else if (errno == EINVAL) {
>>>         current_fd_err_behavior = INVALID_FD_RET_EINVAL;
>>>     } else {
>>>         tst_brk(TBROK, "Unexpected errno %d from mnt_ns_fd probe", errno);
>>>     }

---

> +/*
>   * For commit 78f0e33cd6c9 ("fs/namespace: correctly handle errors returned
>   * by grab_requested_mnt_ns") from v6.18-rc7 backported to v6.17.9.
> + * Defines the expected error code behavior when an invalid file descriptor
> + * is passed to mnt_ns_fd (previously known as spare).
>   */

>>> "previously known as spare" is inaccurate. The field in both the kernel's
>>> struct mnt_id_req and LTP's mnt_id_req_fallback has always been named
>>> mnt_ns_fd. The local helper struct tcase still has a field named `spare`
>>> in this very patch — it was not renamed — so "previously" is wrong in
>>> both directions.
>>>
>>> If the intent is to connect the tcase.spare field to the kernel's
>>> mnt_ns_fd field, say so directly:
>>>
>>>     * is passed to mnt_ns_fd (mapped from struct tcase's spare field).
>>>
>>> Or simply drop the parenthetical — the sentence is clear without it.

---

Commit message:

> Commit 78f0e33cd6c9 ("fs/namespace: correctly handle errors returned
> by grab_requested_mnt_ns") changed the expected errno for an invalid
> mnt_ns_fd from EINVAL to EBADF. This was introduced in v6.17.9.

Minor nit (not a blocker): "This was introduced in v6.17.9" only tells
half the story. The change landed in mainline at v6.18 (via v6.18-rc7) and
was concurrently backported to stable v6.17.9. The sentence could read:

    This landed in mainline v6.18 and was backported to stable v6.17.9.

The code in setup() is correct regardless (tst_kvercmp(6, 17, 9) >= 0
is true for all v6.17.9+, v6.18+, and later), but the commit message
description of the timeline is incomplete.

---

Verdict: Needs revision

LTP AI Reviewer

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