Re: [LTP] [PATCH v2 05/10] Rewrite semtest_2ns.c using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +/*\
> + * [Description]
> + *
> + * Test SysV IPC semaphore usage between namespaces.
> + *
> + * [Algorithm]
> + *
> + * Create 2 'containers'
> + * In container1 create semaphore with a specific key
> + * In container2 try to access the semaphore created in container1
> + *
> + * Test is PASS if flag = none and the semaphore is accessible in container2 or
> + * if flag = unshare/clone and the semaphore is not accessible in container2.
> + * If semaphore is not accessible in container2, creates new semaphore with the
> + * same key to double check isloation in IPCNS.
> + *
> + * Test is FAIL if flag = none and the semaphore is not accessible, if
> + * flag = unshare/clone and semaphore is accessible in container2 or if the new
> + * semaphore creation Fails.
> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <sys/wait.h>
> +#include <sys/msg.h>
> +#include <sys/types.h>
>  #include <sys/sem.h>
> -#include <libclone.h>
> -#include "test.h"
> -#include "ipcns_helper.h"
> -
> -#define MY_KEY     124326L
> -#define UNSHARESTR "unshare"
> -#define CLONESTR   "clone"
> -#define NONESTR    "none"
> -
> -char *TCID = "semtest_2ns";
> -int TST_TOTAL = 1;
> -int p1[2];
> -int p2[2];
> +#include "tst_safe_sysv_ipc.h"
> +#include "tst_test.h"
> +#include "common.h"
> +
> +#define MY_KEY 124326L
> +
> +static char *str_op = "clone";
> +static int use_clone;
> +
>  static struct sembuf semop_lock[2] = {
>  	/* sem_num, sem_op, flag */
> -	{0, 0, 0},		/* wait for sem#0 to become 0 */
> -	{0, 1, SEM_UNDO}	/* then increment sem#0 by 1 */
> +	{ 0, 0, 0 }, /* wait for sem#0 to become 0 */
> +	{ 0, 1, SEM_UNDO } /* then increment sem#0 by 1 */
>  };
>  
>  static struct sembuf semop_unlock[1] = {
>  	/* sem_num, sem_op, flag */
> -	{0, -1, (IPC_NOWAIT | SEM_UNDO)}	/* decrement sem#0 by 1 (sets it to 0) */
> +	{ 0, -1, (IPC_NOWAIT | SEM_UNDO) } /* decrement sem#0 by 1 (sets it to 0) */
>  };
>  
>  /*
>   * sem_lock() - Locks the semaphore for crit-sec updation, and unlocks it later
>   */
> -void sem_lock(int id)
> +static void sem_lock(int id)
>  {
> -	/* Checking the semlock and simulating as if the crit-sec is updated */
> -	if (semop(id, &semop_lock[0], 2) < 0) {
> -		perror("sem lock error");
> -		tst_brkm(TBROK, NULL, "semop failed");
> -	}
> -	tst_resm(TINFO, "Sem1: File locked, Critical section is updated...");
> +	SAFE_SEMOP(id, &semop_lock[0], 2);
> +
> +	tst_res(TINFO, "semaphore1: File locked, Critical section is updated...");
> +
>  	sleep(2);
> -	if (semop(id, &semop_unlock[0], 1) < 0) {
> -		perror("sem unlock error");
> -		tst_brkm(TBROK, NULL, "semop failed");
> -	}
> +
> +	SAFE_SEMOP(id, &semop_unlock[0], 1);
>  }
>  
>  /*
>   * check_sem1 -  does not read -- it writes to check_sem2() when it's done.
>   */
> -int check_sem1(void *vtest)
> +static int check_sem1(LTP_ATTRIBUTE_UNUSED void *vtest)
>  {
> -	int id1;
> -
> -	(void) vtest;
> -
> -	close(p1[0]);
> -	/* 1. Create (or fetch if existing) the binary semaphore */
> -	id1 = semget(MY_KEY, 1, IPC_CREAT | IPC_EXCL | 0666);
> -	if (id1 == -1) {
> -		perror("Semaphore create");
> -		if (errno != EEXIST) {
> -			perror("semget failure");
> -			tst_brkm(TBROK, NULL, "semget failure");
> -		}
> -		id1 = semget(MY_KEY, 1, 0);
> -		if (id1 == -1) {
> -			perror("Semaphore create");
> -			tst_brkm(TBROK, NULL, "semget failure");
> -		}
> +	TEST(semget(MY_KEY, 1, IPC_CREAT | IPC_EXCL | 0666));
> +	if (TST_RET < 0) {
> +		tst_res(TINFO, "semget failure. Checking existing semaphore");
> +
> +		if (TST_ERR != EEXIST)
> +			tst_brk(TBROK | TRERRNO, "Semaphore creation failed");
> +
> +		SAFE_SEMGET(MY_KEY, 1, 0);
>  	}
>  
> -	write(p1[1], "go", 3);
> -	tst_resm(TINFO, "Cont1: Able to create semaphore");
> -	tst_exit();
> +	tst_res(TINFO, "container1: Able to create semaphore");
> +
> +	return 0;
>  }
>  
>  /*
>   * check_sem2() reads from check_sem1() and writes to main() when it's done.
>   */
> -
> -int check_sem2(void *vtest)
> +static int check_sem2(LTP_ATTRIBUTE_UNUSED void *vtest)
>  {
> -	char buf[3];
> -	int id2;
> -
> -	(void) vtest;
> +	int id;
>  
> -	close(p1[1]);
> -	close(p2[0]);
> -	read(p1[0], buf, 3);
> +	id = semget(MY_KEY, 1, 0);
> +	if (id >= 0) {
> +		sem_lock(id);

I wonder why we even call the sem_lock() here, it does not add any
value to the test and only slows it down for two seconds for no reason
in the case of -m none.


Looking at the code the whole test looks strange and does not make much
sense.

Looking at the check_sem1() I guess that it's supposed to create a
semaphore in one namespace, while the check_sem2() is trying to check if
that leaks into the second namespace. But without the checkpoint
synchronization the order is not guaranteed at all.

I guess that this should really be rewritten so that:

* check_sem1() creates the semaphore, then calls CHECKPOINT_WAKE()

* check_sem2() does CHEKPOINT_WAIT() then check if the semaphore does
  exists

That is the very basic test we should do.

On the top of that we can make the check_sem1() to also lock the
semaphore between the call to wake the other process. And the other
process check_sem2() can attempt to lock it as well with IPC_NOWAIT. If
that fails the reference to the semaphore somehow leaked even if the
namespaces are isolated.


-- 
Cyril Hrubis
chrubis@suse.cz

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