[LTP] [PATCH] pidns32: fix PID namespace nesting depth off-by-one

[LTP] [PATCH] pidns32: fix PID namespace nesting depth off-by-one

[Vasileios Almpanis via ltp]

The 2023 refactor used tst_atomic_inc() before clone with a zero-initial
counter, which only performed 31 CLONE_NEWPID nests while still expecting
MAXNEST (32). Compare the level to MAXNEST first, then increment and clone,
so the shared counter runs 0..32 and the chain reaches 32 nested namespaces
again.

Fixes: 647cfd468c3b ("Refactor pidns32 test using new LTP API")
Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
---
 testcases/kernel/containers/pidns/pidns32.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/containers/pidns/pidns32.c b/testcases/kernel/containers/pidns/pidns32.c
index fc9bf0aaa..3a272f569 100644
--- a/testcases/kernel/containers/pidns/pidns32.c
+++ b/testcases/kernel/containers/pidns/pidns32.c
@@ -27,9 +27,11 @@ static pid_t child_func(void)
 {
 	pid_t cpid = 0;
 
-	if (tst_atomic_inc(level) == MAXNEST)
+	if (tst_atomic_load(level) == MAXNEST)
 		return cpid;
 
+	tst_atomic_inc(level);
+
 	cpid = SAFE_CLONE(&args);
 	if (!cpid) {
 		child_func();
-- 
2.43.0


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

Re: [LTP] [PATCH] pidns32: fix PID namespace nesting depth off-by-one

[Andrea Cervesato via ltp]

Hi Vasileios,

> -	if (tst_atomic_inc(level) == MAXNEST)
> +	if (tst_atomic_load(level) == MAXNEST)
>  		return cpid;
> 
> +	tst_atomic_inc(level);

Makes sense, the old code incremented before checking, so the counter
hit 32 after only 31 clones. Splitting load and increment fixes it.

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH] pidns32: fix PID namespace nesting depth off-by-one

[Petr Vorel]

Hi Vasileios,

> The 2023 refactor used tst_atomic_inc() before clone with a zero-initial
> counter, which only performed 31 CLONE_NEWPID nests while still expecting
> MAXNEST (32). Compare the level to MAXNEST first, then increment and clone,
> so the shared counter runs 0..32 and the chain reaches 32 nested namespaces
> again.

Indeed, good catch.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

> Fixes: 647cfd468c3b ("Refactor pidns32 test using new LTP API")
> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
> ---
>  testcases/kernel/containers/pidns/pidns32.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/containers/pidns/pidns32.c b/testcases/kernel/containers/pidns/pidns32.c
> index fc9bf0aaa..3a272f569 100644
> --- a/testcases/kernel/containers/pidns/pidns32.c
> +++ b/testcases/kernel/containers/pidns/pidns32.c
> @@ -27,9 +27,11 @@ static pid_t child_func(void)
>  {
>  	pid_t cpid = 0;

> -	if (tst_atomic_inc(level) == MAXNEST)
> +	if (tst_atomic_load(level) == MAXNEST)
>  		return cpid;

> +	tst_atomic_inc(level);
> +
>  	cpid = SAFE_CLONE(&args);
>  	if (!cpid) {
>  		child_func();

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

Re: [LTP] [PATCH] pidns32: fix PID namespace nesting depth off-by-one

[Cyril Hrubis]

Hi!
> > The 2023 refactor used tst_atomic_inc() before clone with a zero-initial
> > counter, which only performed 31 CLONE_NEWPID nests while still expecting
> > MAXNEST (32). Compare the level to MAXNEST first, then increment and clone,
> > so the shared counter runs 0..32 and the chain reaches 32 nested namespaces
> > again.
> 
> Indeed, good catch.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

Given that we no longer have to implement atomic operations in assembly
and can rely on compiler we can easily add post increment as well.

Should be as easy as:

diff --git a/include/tst_atomic.h b/include/tst_atomic.h
index 9d255bc44..9433291ca 100644
--- a/include/tst_atomic.h
+++ b/include/tst_atomic.h
@@ -30,6 +30,11 @@ static inline void tst_atomic_store(int32_t i, tst_atomic_t *v)
        __atomic_store_n(v, i, __ATOMIC_SEQ_CST);
 }
 
+static inline tst_atomic_return_add(int32_t i, tst_atomic_t *v)
+{
+       return __atomic_fetch_add(v, i, __ATOMIC_SEQ_CST);
+}
+
 #elif HAVE_SYNC_ADD_AND_FETCH == 1
 
 /* Use __sync built-ins (GCC >= 4.1), with explicit memory barriers. */
@@ -39,6 +44,11 @@ static inline int tst_atomic_add_return(int32_t i, tst_atomic_t *v)
        return __sync_add_and_fetch(v, i);
 }
 
+static inline tst_atomic_return_add(int32_t i, tst_atomic_t *v)
+{
+       return __sync_fetch_and_add(v, i);
+}


Then we can do tst_atomic_return_add(1, &level) instead of two different
atomic operations.

-- 
Cyril Hrubis
chrubis@suse.cz

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