Re: [LTP] [PATCH v3] tst_clone: Fix child detection on sparc

[Petr Vorel <pvorel@suse.cz>]

Hi Anders,

> On sparc the clone syscall returns the caller's thread id in the child,
> not 0. So the child thinks it is the parent.

> Compare with tst_gettid() in the clone fallback. clone3 returns 0, but
> sparc only has it since v7.0.

> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
> v2 -> v3: use tst_gettid(), the kernel returns the caller's thread id
> to the child. And only get it in the clone fallback, as the AI review
> suggested.

> v1 -> v2: fixed the AI review comments, use tst_getpid() since glibc
> before 2.25 caches getpid(), and only check after clone, so the goto
> is gone.

>  lib/tst_clone.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

> diff --git a/lib/tst_clone.c b/lib/tst_clone.c
> index 296cd40ac0a0..a04c4042653b 100644
> --- a/lib/tst_clone.c
> +++ b/lib/tst_clone.c
> @@ -35,6 +35,10 @@ pid_t tst_clone(const struct tst_clone_args *tst_args)

>  	flags = args.exit_signal | args.flags;

> +#ifdef __sparc__
> +	pid_t tid = tst_gettid();

Unfortunately, we haven't realized that this will not work in the PID namespace
(CLONE_NEWPID). Agent's finding (run locally):

	- In the child, the old sparc clone puts current->pid into %o0
	  (arch/sparc/kernel/process_64.c:634, process_32.c:365). That is the
	  parent's global thread id.
	- tst_gettid() returns the thread id as seen inside the namespace
	  (kernel/sys.c:1007).
	- Inside a namespace the two numbers differ, so the child takes the parent's
	  code path.

And concrete way how this can be manifested is e.g.
testcases/kernel/containers/pidns/pidns05.c:

	pidns05: child_func() runs as pid 1 in a new namespace and calls
	SAFE_CLONE(). The grandchild sees a number other than 1, behaves as the
	parent and sits in pause() until the checkpoint times out.

Sounds reasonable, could you please verify it?

Agent suggests to check the second return value in %o1 instead (1 in the child,
0 in the parent), as glibc's sparc fork/clone wrappers do).

Looking into glibc sources there is indeed clone() sparc specific assembly:
sysdeps/unix/sysv/linux/sparc/sparc32/clone.S
sysdeps/unix/sysv/linux/sparc/sparc64/clone.S

Agent even suggested assembly which has similar fragments, but of course we'd
strive to avoid using it unless really necessary.

#ifdef __sparc__
/*
 * On sparc, clone() returns the parent's global thread id in %o0 in the
 * child, which cannot be compared with gettid() inside a pid namespace.
 * The kernel sets %o1 to 1 in the child and 0 in the parent, but syscall()
 * drops %o1, hence inline assembly.
 */
static pid_t sparc_clone(unsigned long flags, unsigned long pidfd)
{
	register long g1 __asm__("g1") = __NR_clone;
	register long o0 __asm__("o0") = flags;
	register long o1 __asm__("o1") = 0;
	register long o2 __asm__("o2") = pidfd;
	register long o3 __asm__("o3") = 0;
	register long o4 __asm__("o4") = 0;

	__asm__ __volatile__(
# ifdef __arch64__
		"ta 0x6d\n\t"
		"bcc,pt %%xcc, 1f\n\t"
# else
		"ta 0x10\n\t"
		"bcc 1f\n\t"
# endif
		" mov 0, %%g1\n\t"
		"mov 1, %%g1\n"
		"1:"
		: "+r" (g1), "+r" (o0), "+r" (o1), "+r" (o2), "+r" (o3), "+r" (o4)
		:
		: "cc", "memory");

	if (g1) {
		errno = o0;
		return -1;
	}

	return o1 ? 0 : o0;
}
#endif

Regard that use of tst_gettid() instead of tst_getpid() suggested for v2 by
agent [1]:

	the SPARC kernel returns the caller's TID (current->pid) in %o0 to the
	child, so use tst_gettid() instead of tst_getpid() to support multithreaded
	callers.

Sure, LTP has tests which use threads, but many of them use
pthread_create() instead of SAFE_CLONE()/tst_clone() (which you're trying to
fix).

IMHO there is only testcases/kernel/syscalls/fanotify/fanotify21.c, but even
that test is safe, because SAFE_CLONE() is call for a different test case than
pthread usage.

But of course it does not harm to have it.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20260929111636.7777-1-linuxtestproject.agent@gmail.com/

> +#endif
> +
>  #ifdef __s390x__
>  	pid = syscall(__NR_clone, NULL, flags, args.pidfd, NULL, NULL);
>  #else
> @@ -44,5 +48,10 @@ pid_t tst_clone(const struct tst_clone_args *tst_args)
>  	if (pid == -1)
>  		return -2;

> +#ifdef __sparc__
> +	if (pid == tid)
> +		pid = 0;
> +#endif
> +
>  	return pid;
>  }

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