Skip to contentSkip to conversation

Collection of fixes

PATCH New Punit Agrawal 3 participants 29 messages Dec 4, 2017 – Dec 8, 2017

lore
Share & tools
lore
#0 lore raw

v3 cover Collection of fixes

Hi again,

This is the third posting of a collection of fixes for issues encountered when running ltp on internal test platforms. Previous versions can be found at [0][1]. This update addresses all the feedback received on the previous version.

Changes from v2:

* Dropped check for THP in Patch 2 for thp01.c
* Updated parent/child synchronisation to cover for cases missed in the previous version of Patch 3
* Improved detection of successful child exit based on feedback in Patch 8
* Patch 9 - renamed helper binary and used TST_RESOURCE_COPY() to copy the helper in setup and before usage
* Patch 10 - Changed loop to open files to be infinite. Test still breaks out of the loop if there is an error opening the file.
* Dropped Patch 12

[0] http://lists.linux.it/pipermail/ltp/2017-November/006414.html
[1] http://lists.linux.it/pipermail/ltp/2017-October/006225.html

James Morse (1):
hotplug/cpu_hotplug: Remove bashism disown from kill_pid()

Lorenzo Pieralisi (1):
hugeshmget02: add missing SHM_HUGETLB flag on segment creation

Punit Agrawal (5):
Move check_hugepage() helper to mem/lib
hugeshmctl01: Convert to LTP synchronisation primitives
hugeshmctl01: Fix warning about signed/unsigned comparison
sigwaitinfo01: catch SEGV and report success for bad_address2 testcase syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Suzuki K. Poulose (3):
hugeshmctl02: Fix allocation size for odd number of hugepages
getdtablesize01: Handle ENFILE errno
perf_event_open: Handle absence of PMU gracefully

Will Deacon (2):
thp: ensure hugetlbfs is available
sigwaitinfo01: fix race between sending and dequeueing RT signals

 .../cpu_hotplug/include/cpuhotplug_testsuite.sh    |  1 -
 .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c   | 75 ++++------------------
 .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c   |  2 +-
 .../kernel/mem/hugetlb/hugeshmget/hugeshmget02.c   |  3 +-
 testcases/kernel/mem/hugetlb/lib/hugetlb.c         |  7 --
 testcases/kernel/mem/hugetlb/lib/hugetlb.h         |  1 -
 testcases/kernel/mem/include/mem.h                 |  2 +
 testcases/kernel/mem/lib/mem.c                     |  6 ++
 testcases/kernel/mem/thp/thp02.c                   |  2 +
 testcases/kernel/mem/thp/thp03.c                   |  2 +
 .../syscalls/getdtablesize/getdtablesize01.c       | 15 +++--
 testcases/kernel/syscalls/mount/mount03.c          | 11 ++--
 .../mount/{setuid_test.c => mount03_setuid_test.c} |  0
 .../syscalls/perf_event_open/perf_event_open01.c   |  3 +-
 .../syscalls/perf_event_open/perf_event_open02.c   |  4 +-
 .../kernel/syscalls/sigwaitinfo/sigwaitinfo01.c    | 28 +++++++-
 16 files changed, 75 insertions(+), 87 deletions(-)
 rename testcases/kernel/syscalls/mount/{setuid_test.c => mount03_setuid_test.c} (100%)
Signature · 1 line
2.15.0
#1 lore raw

PATCH v3 1/12 Move check_hugepage() helper to mem/lib

Move the check_hugepage() helper to mem/lib to be more widely available to testcases. Specifically, it will be used in a subsequent commit to check for the presence of hugepage support when running the thp tests.

---
 testcases/kernel/mem/hugetlb/lib/hugetlb.c | 7 -------
 testcases/kernel/mem/hugetlb/lib/hugetlb.h | 1 -
 testcases/kernel/mem/include/mem.h         | 2 ++
 testcases/kernel/mem/lib/mem.c             | 6 ++++++
 4 files changed, 8 insertions(+), 8 deletions(-)
testcases/kernel/mem/hugetlb/lib/hugetlb.c +0 -7
index f6a909a21..1ea47ebb5 100644
@@ -26,7 +26,6 @@
  *
  *	The library contains the following routines:
  *
- *	check_hugepage()
  *	getipckey()
  *	getuserid()
  *	rm_shm()
@@ -42,12 +41,6 @@
 
 static long orig_hugepages = -1;
 
-void check_hugepage(void)
-{
-	if (access(PATH_HUGEPAGES, F_OK))
-		tst_brk(TCONF, "Huge page is not supported.");
-}
-
 long save_nr_hugepages(void)
 {
 	check_hugepage();
testcases/kernel/mem/hugetlb/lib/hugetlb.h +0 -1
index 53da45d8c..2843f0a8e 100644
@@ -46,7 +46,6 @@
  * from shmid_ds.ipc_perm.mode
  */
 #define MODE_MASK	0x01FF
-#define PATH_HUGEPAGES	"/sys/kernel/mm/hugepages/"
 
 key_t shmkey;			/* an IPC key generated by ftok() */
 
testcases/kernel/mem/include/mem.h +2 -0
index 287f8b3f6..95d0bda72 100644
@@ -56,8 +56,10 @@ void test_ksm_merge_across_nodes(unsigned long nr_pages);
 
 /* HUGETLB */
 
+#define PATH_HUGEPAGES		"/sys/kernel/mm/hugepages/"
 #define PATH_SHMMAX		"/proc/sys/kernel/shmmax"
 
+void check_hugepage(void);
 void write_memcg(void);
 
 /* cpuset/memcg */
testcases/kernel/mem/lib/mem.c +6 -0
index fc2f130f4..7f2099b9e 100644
@@ -347,6 +347,12 @@ static void verify(char **memory, char value, int proc,
 	free(s);
 }
 
+void check_hugepage(void)
+{
+	if (access(PATH_HUGEPAGES, F_OK))
+		tst_brk(TCONF, "Huge page is not supported.");
+}
+
 void write_memcg(void)
 {
 	SAFE_FILE_PRINTF(MEMCG_LIMIT, "%ld", TESTMEM);
Signature · 1 line
2.15.0
#3 lore raw

PATCH v3 2/12 thp: ensure hugetlbfs is available

In reply to Punit Agrawal

From: Will Deacon <[email protected]>

The THP tests thp02 and thp03 depend on the presence of hugetlbfs to get the default hugepage size. If hugetlbfs is not enabled, /proc/meminfo does not contain "Hugepagesize" and the tests fail with -

safe_file_ops.c:220: BROK: Expected 1 conversions got 0 at thp03.c:85

which is hard to relate to the missing support for hugetlbfs.

Add tests for the presence of hugetlbfs for tests relying on this kernel feature.

---
 testcases/kernel/mem/thp/thp02.c | 2 ++
 testcases/kernel/mem/thp/thp03.c | 2 ++
 2 files changed, 4 insertions(+)
testcases/kernel/mem/thp/thp02.c +2 -0
index acc70e215..6502210e5 100644
@@ -90,6 +90,8 @@ static void setup(void)
 	if (access(PATH_THP, F_OK) == -1)
 		tst_brk(TCONF, "THP not enabled in kernel?");
 
+	check_hugepage();
+
 	ps = sysconf(_SC_PAGESIZE);
 	hps = SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
 	size = hps * 4;
testcases/kernel/mem/thp/thp03.c +2 -0
index 19db44944..c6062505f 100644
@@ -82,6 +82,8 @@ static void setup(void)
 	if (access(PATH_THP, F_OK) == -1)
 		tst_brk(TCONF, "THP not enabled in kernel?");
 
+	check_hugepage();
+
 	hugepage_size = SAFE_READ_MEMINFO("Hugepagesize:") * KB;
 	unaligned_size = hugepage_size * 4 - 1;
 	page_size = SAFE_SYSCONF(_SC_PAGESIZE);
Signature · 1 line
2.15.0
#4 lore raw

PATCH v3 2/12 thp: ensure hugetlbfs is available

Quoted 8 lines

From: Will Deacon <[email protected]>

The THP tests thp02 and thp03 depend on the presence of hugetlbfs to get the default hugepage size. If hugetlbfs is not enabled, /proc/meminfo does not contain "Hugepagesize" and the tests fail with -

safe_file_ops.c:220: BROK: Expected 1 conversions got 0 at thp03.c:85

Quoted 8 lines

which is hard to relate to the missing support for hugetlbfs.

Add tests for the presence of hugetlbfs for tests relying on this kernel feature.

---

#5 lore raw

PATCH v3 3/12 hugeshmctl01: Convert to LTP synchronisation primitives

In reply to Punit Agrawal

hugeshmctl01 spawns child processes to perform shm operations and uses signal (SIGUSR1) to communicate the completion of test 1, to the children waiting at sigprocmask(). However, there is no guarantee that a child has reached sigprocmask() before the parent issues a SIGUSR1 and things go wrong.

Fix this by migrating hugeshmctl to LTP synchronisation primitives TST_CHECKPOINT_{WAIT,WAKE,WAIT2} and removing the code that was used for signals based synchronisation.

---
 .../kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c   | 72 ++++------------------
 1 file changed, 12 insertions(+), 60 deletions(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +12 -60
index 248065d2f..78a642973 100644
@@ -55,9 +55,7 @@ static struct shmid_ds buf;
 static time_t save_time;
 static int stat_time;
 static void *set_shared;
-static pid_t pid_arr[N_ATTACH];
 
-static void sighandler(int sig);
 static void stat_setup(void);
 static void stat_cleanup(void);
 static void set_setup(void);
@@ -140,19 +138,10 @@ void *set_shmat(void)
  */
 static void stat_setup(void)
 {
-	int i, rval;
+	int i;
 	void *test;
 	pid_t pid;
-	sigset_t newmask, oldmask;
-	struct sigaction sa;
-
-	memset (&sa, '\0', sizeof(sa));
-	sa.sa_handler = sighandler;
-	sa.sa_flags = 0;
-	TEST(sigaction(SIGUSR1, &sa, NULL));
-	if (TEST_RETURN == -1)
-		tst_brk(TBROK | TRERRNO,
-				"SIGSEGV signal setup failed");
+
 	/*
 	 * The first time through, let the children attach the memory.
 	 * The second time through, attach the memory first and let
@@ -167,21 +156,6 @@ static void stat_setup(void)
 		set_shared = set_shmat();
 	}
 
-	/*
-	 * Block SIGUSR1 before children pause for a signal
-	 * Doing so to avoid the risk that the parent cleans up
-	 * children by calling stat_cleanup() before children call
-	 * call pause() so that children sleep forever(this is a
-	 * side effect of the arbitrary usleep time below).
-	 * In FIRST, children call shmat. If children sleep forever,
-	 * those attached shm can't be released so some other shm
-	 * tests will fail a lot.
-	 */
-	sigemptyset(&newmask);
-	sigaddset(&newmask, SIGUSR1);
-	if (sigprocmask(SIG_BLOCK, &newmask, &oldmask) < 0)
-		tst_brk(TBROK | TERRNO, "block SIGUSR1 error");
-
 	for (i = 0; i < N_ATTACH; i++) {
 		switch (pid = SAFE_FORK()) {
 		case 0:
@@ -190,39 +164,20 @@ static void stat_setup(void)
 			/* do an assignement for fun */
 			*(int *)test = i;
 
-			/*
-			 * sigsuspend until we get a signal from stat_cleanup()
-			 * use sigsuspend instead of pause to avoid children
-			 * infinite sleep without getting SIGUSR1 from parent
-			 */
-			rval = sigsuspend(&oldmask);
-			if (rval != -1)
-				tst_brk(TBROK | TERRNO, "sigsuspend");
-
-			/*
-			 * don't have to block SIGUSR1 any more,
-			 * recover the mask
-			 */
-			if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0)
-				tst_brk(TBROK | TERRNO,
-					 "child sigprocmask");
+			TST_CHECKPOINT_WAKE(0);
+
+			TST_CHECKPOINT_WAIT(1);
 
 			/* now we're back - detach the memory and exit */
 			if (shmdt(test) == -1)
 				tst_brk(TBROK | TERRNO,
 					 "shmdt in stat_setup()");
+
 			exit(0);
 		default:
-			/* save the child's pid for cleanup later */
-			pid_arr[i] = pid;
+			TST_CHECKPOINT_WAIT(0);
 		}
 	}
-
-	/* parent doesn't have to block SIGUSR1, recover the mask */
-	if (sigprocmask(SIG_SETMASK, &oldmask, NULL) < 0)
-		tst_brk(TBROK, "parent sigprocmask");
-
-	usleep(250000);
 }
 
 /*
@@ -282,11 +237,13 @@ fail:
  */
 static void stat_cleanup(void)
 {
-	int i;
+	int i, status;
 
 	/* wake up the childern so they can detach the memory and exit */
+	TST_CHECKPOINT_WAKE2(1, N_ATTACH);
+
 	for (i = 0; i < N_ATTACH; i++)
-		SAFE_KILL(pid_arr[i], SIGUSR1);
+		SAFE_WAIT(&status);
 
 	/* remove the parent's shared memory the second time through */
 	if (stat_time == SECOND)
@@ -295,12 +252,6 @@ static void stat_cleanup(void)
 	stat_time++;
 }
 
-static void sighandler(int sig)
-{
-	if (sig != SIGUSR1)
-		tst_res(TFAIL, "received unexpected signal %d", sig);
-}
-
 /*
  * set_setup() - set up for the IPC_SET command with shmctl()
  */
@@ -385,4 +336,5 @@ static struct tst_test test = {
 	.setup = setup,
 	.cleanup = cleanup,
 	.test_all = test_hugeshmctl,
+	.needs_checkpoints = 1,
 };
Signature · 1 line
2.15.0
#6 lore raw

PATCH v3 4/12 hugeshmctl01: Fix warning about signed/unsigned comparison

In reply to Punit Agrawal

The macro N_ATTACH is assumed to be a signed integer which results in the below warning when compiling -

warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

  if (buf.shm_nattch != N_ATTACH + stat_time) {
                     ^~

Fix this by explicitly marking N_ATTACH to be an unsigned integer and fixing the resulting fallout.

---
 testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl01.c +4 -3
index 78a642973..c639c8e5f 100644
@@ -46,7 +46,7 @@
 
 #define FIRST		0
 #define SECOND		1
-#define N_ATTACH	4
+#define N_ATTACH	4U
 #define NEWMODE		0066
 
 static size_t shm_size;
@@ -138,7 +138,7 @@ void *set_shmat(void)
  */
 static void stat_setup(void)
 {
-	int i;
+	unsigned int i;
 	void *test;
 	pid_t pid;
 
@@ -237,7 +237,8 @@ fail:
  */
 static void stat_cleanup(void)
 {
-	int i, status;
+	unsigned int i;
+	int status;
 
 	/* wake up the childern so they can detach the memory and exit */
 	TST_CHECKPOINT_WAKE2(1, N_ATTACH);
Signature · 1 line
2.15.0
#7 lore raw

PATCH v3 4/12 hugeshmctl01: Fix warning about signed/unsigned comparison

Quoted 4 lines

The macro N_ATTACH is assumed to be a signed integer which results in the below warning when compiling -

warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Quoted 8 lines
  if (buf.shm_nattch != N_ATTACH + stat_time) {
                     ^~

Fix this by explicitly marking N_ATTACH to be an unsigned integer and fixing the resulting fallout.

---

#8 lore raw

PATCH v3 5/12 hugeshmctl02: Fix allocation size for odd number of hugepages

In reply to Punit Agrawal

From: "Suzuki K. Poulose" <[email protected]>

The test tries to allocate 2 * (half the number of available hugepages). If we have odd number hugepages, the calculated allocation size is not page-aligned and allocation fails.

e.g, (use -s option to trigger it manually)

$ hugeshmctl02 -s 7 -i 5
hugeshmctl02    0  TINFO  :  set nr_hugepages to 7
hugeshmctl02    1  TBROK  :  hugeshmctl02.c:153: shmget #2: errno=ENOMEM(12): Cannot allocate memory
hugeshmctl02    2  TBROK  :  hugeshmctl02.c:153: Remaining cases broken
hugeshmctl02    0  TINFO  :  set nr_hugepages to 0

Align the size to hugepage_size.

---
 testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
testcases/kernel/mem/hugetlb/hugeshmctl/hugeshmctl02.c +1 -1
index 8a3eee474..af2bf4a3a 100644
@@ -99,7 +99,7 @@ static void setup(void)
 	set_sys_tune("nr_hugepages", hugepages, 1);
 	hpage_size = SAFE_READ_MEMINFO("Hugepagesize:") * 1024;
 
-	shm_size = hpage_size * hugepages / 2;
+	shm_size = hpage_size * (hugepages / 2);
 	update_shm_size(&shm_size);
 	shmkey = getipckey();
 
Signature · 1 line
2.15.0
#9 lore raw

PATCH v3 6/12 hugeshmget02: add missing SHM_HUGETLB flag on segment creation

In reply to Punit Agrawal

From: Lorenzo Pieralisi <[email protected]>

On test setup, hugeshmget02 detects the number of huge pages available and uses that number to size a shared memory segment that is allocated to carry out subsequent syscalls checks.

The setup() function detects the number of huge pages available so that it can size the share memory segment to allocate with a reasonable size value, but fails to pass the SHM_HUGETLB on shared memory segment creation which defeats the whole purpose of detecting the available number of huge pages before creating the segment.

This omission can result in test failures, eg:

hugeshmget02    0  TINFO  :  set nr_hugepages to 128
hugeshmget02    0  TINFO  :  Using 21 hugepages
hugeshmget02    1  TBROK  :  hugeshmget02.c:155: shmget #setup:
errno=ENOMEM(12): Cannot allocate memory
hugeshmget02    2  TBROK  :  hugeshmget02.c:155: Remaining cases broken
hugeshmget02    0  TINFO  :  set nr_hugepages to 0

This patch adds the missing SHM_HUGETLB flag to the shmget call in the setup() function.

---
 testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget02.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
testcases/kernel/mem/hugetlb/hugeshmget/hugeshmget02.c +2 -1
index 657b66eb3..8df943e0e 100644
@@ -97,7 +97,8 @@ void setup(void)
 
 	shmkey = getipckey();
 	shmkey2 = shmkey + 1;
-	shm_id_1 = shmget(shmkey, shm_size, IPC_CREAT | IPC_EXCL | SHM_RW);
+	shm_id_1 = shmget(shmkey, shm_size,
+			  SHM_HUGETLB | IPC_CREAT | IPC_EXCL | SHM_RW);
 	if (shm_id_1 == -1)
 		tst_brk(TBROK | TERRNO, "shmget #setup");
 }
Signature · 1 line
2.15.0
#10 lore raw

PATCH v3 7/12 sigwaitinfo01: fix race between sending and dequeueing RT signals

In reply to Punit Agrawal

From: Will Deacon <[email protected]>

Although RT signals are dequeued in order, there is a race where only the higher signal may have been sent and will therefore be dequeued first, leading to a false failure.

This patch waits on the signal sending tasks so that we can be sure both of the signals have been sent before dequeuing them.

---
 testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c | 5 +++++
 1 file changed, 5 insertions(+)
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +5 -0
index 35acd67c4..16b5096b8 100644
@@ -249,6 +249,7 @@ void test_masked_matching_rt(swi_func sigwaitinfo, int signo)
 	sigset_t sigs, oldmask;
 	siginfo_t si;
 	pid_t child[2];
+	int status;
 
 	signo = SIGRTMIN + 1;
 
@@ -268,6 +269,10 @@ void test_masked_matching_rt(swi_func sigwaitinfo, int signo)
 	child[0] = create_sig_proc(0, signo, 1);
 	child[1] = create_sig_proc(0, signo + 1, 1);
 
+	/* Ensure that the signals have been sent */
+	waitpid(child[0], &status, 0);
+	waitpid(child[1], &status, 0);
+
 	TEST(sigwaitinfo(&sigs, &si, NULL));
 	REPORT_SUCCESS_COND(signo, 0, si.si_pid == child[0]
 			    && si.si_code == SI_USER
Signature · 1 line
2.15.0
#11 lore raw

PATCH v3 8/12 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase

In reply to Punit Agrawal

The bad_address2 testcase passes (void *)1 as a sigset pointer to the sigwaitinfo syscall. Unsurprisingly, this segfaults in libc rather than returning -1 (errno = EFAULT) as LTP expects.

Instead, fork a child process which registers a default handler for SIGSEGV to catch any SEGV's generated by the sigwaitinfo syscall. The parent checks that the child was terminated by SIGSEGV to evaluate the success of the test.

---
 .../kernel/syscalls/sigwaitinfo/sigwaitinfo01.c    | 23 ++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
testcases/kernel/syscalls/sigwaitinfo/sigwaitinfo01.c +21 -2
index 16b5096b8..95a9436a4 100644
@@ -363,8 +363,27 @@ void test_bad_address(swi_func sigwaitinfo, int signo)
 
 void test_bad_address2(swi_func sigwaitinfo, int signo)
 {
-	TEST(sigwaitinfo((void *)1, NULL, NULL));
-	REPORT_SUCCESS(-1, EFAULT);
+	pid_t pid;
+	int status;
+
+	switch (pid = fork()) {
+	case -1:
+		tst_brkm(TBROK | TERRNO, NULL, "fork() failed");
+	case 0:
+		signal(SIGSEGV, SIG_DFL);
+		TEST(sigwaitinfo((void *)1, NULL, NULL));
+
+		_exit(0);
+		break;
+	default:
+		break;
+	}
+
+	SUCCEED_OR_DIE(waitpid, "waitpid failed", pid, &status, 0);
+	if (WIFSIGNALED(status) && WTERMSIG(status) == SIGSEGV)
+		tst_resm(TPASS, "Test passed");
+	else
+		tst_resm(TFAIL, "Unrecognised child exit code");
 }
 
 void test_bad_address3(swi_func sigwaitinfo, int signo)
Signature · 1 line
2.15.0
#12 lore raw

PATCH v3 8/12 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase

Hi!
I've also added a follow up patch that adds this test to the syscalls runtest file.

Other than the two patches I've commented on the rest of the patchset has been pushed, thanks!

Signature · 2 lines
Cyril Hrubis
[email protected]
#13 lore raw

PATCH v3 8/12 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase

Quoted 6 lines · Cyril Hrubis

Hi!
I've also added a follow up patch that adds this test to the syscalls runtest file.

Other than the two patches I've commented on the rest of the patchset has been pushed, thanks!

Thanks for applying the patches.

I don't see Patch 7 "sigwaitinfo01: fix race between sending and dequeueing RT signals" in the repo.

Did this one get missed out?

Thanks,
Punit

#14 lore raw

PATCH v3 8/12 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase

Hi!

Quoted 1 line

Did this one get missed out?

Yes, sorry, I've pushed it right now.

Signature · 2 lines
Cyril Hrubis
[email protected]
#15 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

In reply to Punit Agrawal

When testing the MS_NOSUID mount flag, mount03 forgets to copy setuid_test into the new filesystem. Instead it writes 'TEST FILE' into a new file and attempts to execute it with S_ISUID. This fails, but not for the reasons ltp expects.

Fix this by copying the test helper binary instead.

While we're touching the test, also rename the helper to mount03_setuid_test so as to follow current ltp convention.

---
 testcases/kernel/syscalls/mount/mount03.c                     | 11 +++++++----
 .../syscalls/mount/{setuid_test.c => mount03_setuid_test.c}   |  0
 2 files changed, 7 insertions(+), 4 deletions(-)
 rename testcases/kernel/syscalls/mount/{setuid_test.c => mount03_setuid_test.c} (100%)
testcases/kernel/syscalls/mount/mount03.c +7 -4
index bf78c797f..0319c6093 100644
@@ -261,8 +261,8 @@ int test_rwflag(int i, int cnt)
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
-		SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
+		TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
 
 		SAFE_STAT(cleanup, file, &file_stat);
 
@@ -291,9 +291,8 @@ int test_rwflag(int i, int cnt)
 				/* reset the setup_uid */
 				if (status)
 					return 0;
-				else
-					return 1;
 			}
+			return 1;
 		}
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */
@@ -373,6 +372,10 @@ static void setup(void)
 	strncpy(path, path_name, PATH_MAX);
 	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 
+	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
+	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+	SAFE_UMOUNT(cleanup, mntpoint);
+
 	TEST_PAUSE;
 }
 
testcases/kernel/syscalls/mount/mount03_setuid_test.c renamed from testcases/kernel/syscalls/mount/setuid_test.c +0 -0
similarity index 100%
rename from testcases/kernel/syscalls/mount/setuid_test.c
rename to testcases/kernel/syscalls/mount/mount03_setuid_test.c
Signature · 1 line
2.15.0
#16 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Hi!

Quoted 22 lines
testcases/kernel/syscalls/mount/mount03.c +3 -4
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
-		SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
+		TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
 
 		SAFE_STAT(cleanup, file, &file_stat);
@@ -291,9 +291,8 @@ int test_rwflag(int i, int cnt)
 				/* reset the setup_uid */
 				if (status)
 					return 0;
-				else
-					return 1;
 			}
+			return 1;
 		}
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */

testcases/kernel/syscalls/mount/mount03.c static void setup(void) · line 377

Quoted 6 lines
testcases/kernel/syscalls/mount/mount03.c +3 -0
@@ -373,6 +372,10 @@ static void setup(void)
 	strncpy(path, path_name, PATH_MAX);
 	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 
+	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
+	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+	SAFE_UMOUNT(cleanup, mntpoint);

I'm, a bit confused now, do we copy the file here in the setup or in the actual test above? Surely we don't have to do both.

Signature · 2 lines
Cyril Hrubis
[email protected]
#17 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Quoted 26 lines · Cyril Hrubis

Hi!

Quoted 25 lines
testcases/kernel/syscalls/mount/mount03.c +3 -4
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
-		SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
+		TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
 
 		SAFE_STAT(cleanup, file, &file_stat);
@@ -291,9 +291,8 @@ int test_rwflag(int i, int cnt)
 				/* reset the setup_uid */
 				if (status)
 					return 0;
-				else
-					return 1;
 			}
+			return 1;
 		}
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */
@@ -373,6 +372,10 @@ static void setup(void)
 	strncpy(path, path_name, PATH_MAX);
 	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 
Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c static void setup(void) · line 377

Quoted 3 lines
testcases/kernel/syscalls/mount/mount03.c +3 -0
+	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
+	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+	SAFE_UMOUNT(cleanup, mntpoint);

I'm, a bit confused now, do we copy the file here in the setup or in the actual test above? Surely we don't have to do both.

In an earlier email you said -

"... secondly you are supposed to use the TST_RESOURCE_COPY() macro instead of the function itself and it should be done once in the test setup as well as the code that sets the actual setuid bit."

And now I'm confused as I updated the patch as per your request (or so I thought). Where's the disconnect?

#18 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Hi!

Quoted 32 lines
Quoted 25 lines
Quoted 25 lines
testcases/kernel/syscalls/mount/mount03.c +3 -4
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
-		SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
+		TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
 
 		SAFE_STAT(cleanup, file, &file_stat);
@@ -291,9 +291,8 @@ int test_rwflag(int i, int cnt)
 				/* reset the setup_uid */
 				if (status)
 					return 0;
-				else
-					return 1;
 			}
+			return 1;
 		}
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */
@@ -373,6 +372,10 @@ static void setup(void)
 	strncpy(path, path_name, PATH_MAX);
 	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 
Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c static void setup(void) · line 377

Quoted 3 lines
testcases/kernel/syscalls/mount/mount03.c +3 -0
+	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
+	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+	SAFE_UMOUNT(cleanup, mntpoint);

I'm, a bit confused now, do we copy the file here in the setup or in the actual test above? Surely we don't have to do both.

Quoted 8 lines

In an earlier email you said -

"... secondly you are supposed to use the TST_RESOURCE_COPY() macro instead of the function itself and it should be done once in the test setup as well as the code that sets the actual setuid bit."

And now I'm confused as I updated the patch as per your request (or so I thought). Where's the disconnect?

As far as I can see, we do TST_RESOURCE_COPY() twice in this patch, once in the test setup and once in the actual test, which is what confuses me.

Signature · 2 lines
Cyril Hrubis
[email protected]
#19 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Quoted 39 lines · Cyril Hrubis

Hi!

Quoted 38 lines
Quoted 25 lines
Quoted 25 lines
testcases/kernel/syscalls/mount/mount03.c +3 -4
 	case 5:
 		/* Validate MS_NOSUID flag of mount call */
 
-		snprintf(file, PATH_MAX, "%ssetuid_test", path_name);
-		SAFE_FILE_PRINTF(cleanup, file, "TEST FILE");
+		TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+		snprintf(file, PATH_MAX, "%smount03_setuid_test", path_name);
 
 		SAFE_STAT(cleanup, file, &file_stat);
@@ -291,9 +291,8 @@ int test_rwflag(int i, int cnt)
 				/* reset the setup_uid */
 				if (status)
 					return 0;
-				else
-					return 1;
 			}
+			return 1;
 		}
 	case 6:
 		/* Validate MS_NOATIME flag of mount call */
@@ -373,6 +372,10 @@ static void setup(void)
 	strncpy(path, path_name, PATH_MAX);
 	snprintf(path_name, PATH_MAX, "%s/%s/", path, mntpoint);
 
Quoted 6 lines

testcases/kernel/syscalls/mount/mount03.c static void setup(void) · line 377

Quoted 3 lines
testcases/kernel/syscalls/mount/mount03.c +3 -0
+	SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
+	TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);
+	SAFE_UMOUNT(cleanup, mntpoint);

I'm, a bit confused now, do we copy the file here in the setup or in the actual test above? Surely we don't have to do both.

In an earlier email you said -

"... secondly you are supposed to use the TST_RESOURCE_COPY() macro instead of the function itself and it should be done once in the test setup as well as the code that sets the actual setuid bit."

Quoted 6 lines
Quoted 2 lines

And now I'm confused as I updated the patch as per your request (or so I thought). Where's the disconnect?

As far as I can see, we do TST_RESOURCE_COPY() twice in this patch, once in the test setup and once in the actual test, which is what confuses me.

Apologies for not being more specific. We do it twice because in the quoted text you say (emphasis mine) -

"it should be done _once_ in the test setup _as well as_ the code that sets the actual setuid bit."

I took that to mean that you're asking for the file to be copied in setup() as well as in the code before it gets used. Obviously I've misunderstood your statement. What are you actually suggesting there?

If you aren't actually asking for the file to be copied twice, I am happy for the superfluous instance to be dropped. :)

#20 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Hi!

Quoted 9 lines

Apologies for not being more specific. We do it twice because in the quoted text you say (emphasis mine) -

"it should be done _once_ in the test setup _as well as_ the code that sets the actual setuid bit."

I took that to mean that you're asking for the file to be copied in setup() as well as in the code before it gets used. Obviously I've misunderstood your statement. What are you actually suggesting there?

Ah, that's where the misunderstanding happened, "the code that sets the actual setuid bit" == chmod(file, SUID_MODE);

I was trying to state that the actual test setup that could be done only once should be put into the test setup.

Signature · 2 lines
Cyril Hrubis
[email protected]
#21 lore raw

PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'

Quoted 11 lines · Cyril Hrubis

Hi!

Quoted 9 lines

Apologies for not being more specific. We do it twice because in the quoted text you say (emphasis mine) -

"it should be done _once_ in the test setup _as well as_ the code that sets the actual setuid bit."

I took that to mean that you're asking for the file to be copied in setup() as well as in the code before it gets used. Obviously I've misunderstood your statement. What are you actually suggesting there?

Quoted 5 lines

Ah, that's where the misunderstanding happened, "the code that sets the actual setuid bit" == chmod(file, SUID_MODE);

I was trying to state that the actual test setup that could be done only once should be put into the test setup.

Right that makes sense.

I'll send an update on this and the getdtablesize patch shortly.

#22 lore raw

PATCH v3 10/12 getdtablesize01: Handle ENFILE errno

In reply to Punit Agrawal

From: "Suzuki K. Poulose" <[email protected]>

getdtablesize01 testcase attempts to open RLIMIT_NOFILE-1 (Maximum open files for a process) file descriptors. However if we hit an ENFILE (system wide limit of maximum number of open files) we should break the test, rather than failing. This is more relevant for runs on VMs where the rootfs could be a 9p fs or any other emulated fs.

While at it also drop a hunk that spuriously closes a single file.

---
 testcases/kernel/syscalls/getdtablesize/getdtablesize01.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +9 -6
index 7ee231adf..3b319d91b 100644
@@ -54,7 +54,7 @@ int TST_TOTAL = 1;
 
 int main(void)
 {
-	int table_size, loop, fd, count = 0;
+	int table_size, loop, fd = 0, count = 0;
 	int max_val_opfiles;
 	struct rlimit rlp;
 
@@ -80,15 +80,16 @@ int main(void)
 
 	tst_resm(TINFO,
 		 "Checking Max num of files that can be opened by a process.Should be: RLIMIT_NOFILE - 1");
-	for (loop = 1; loop <= max_val_opfiles; loop++) {
+	for (loop = 0; ; loop++) {
 		fd = open("/etc/hosts", O_RDONLY);
+
+		if (fd == -1)
+			break;
+		count = fd;
+
 #ifdef DEBUG
 		printf("Opened file num %d\n", fd);
 #endif
-		if (fd == -1)
-			break;
-		else
-			count = fd;
 	}
 
 //Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read getdtablesize man page
@@ -97,6 +98,8 @@ int main(void)
 		close(count);
 	if (count == (max_val_opfiles - 1))
 		tst_resm(TPASS, "%d = %d", count, (max_val_opfiles - 1));
+	else if (fd < 0 && errno == ENFILE)
+		tst_brkm(TBROK, cleanup, "Reached maximum number of open files for the system");
 	else
 		tst_resm(TFAIL, "%d != %d", count, (max_val_opfiles - 1));
 
Signature · 1 line
2.15.0
#23 lore raw

PATCH v3 10/12 getdtablesize01: Handle ENFILE errno

Hi!

testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 83

Quoted 2 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +1 -1
-	for (loop = 1; loop <= max_val_opfiles; loop++) {
+	for (loop = 0; ; loop++) {

Why not just for (;;) ?

I do not see that we use the loop variable anywhere in the code.

Signature · 2 lines
Cyril Hrubis
[email protected]
#24 lore raw

PATCH v3 10/12 getdtablesize01: Handle ENFILE errno

Quoted 7 lines · Cyril Hrubis

Hi!

testcases/kernel/syscalls/getdtablesize/getdtablesize01.c int main(void) · line 83

Quoted 2 lines
testcases/kernel/syscalls/getdtablesize/getdtablesize01.c +1 -1
-	for (loop = 1; loop <= max_val_opfiles; loop++) {
+	for (loop = 0; ; loop++) {

Why not just for (;;) ?

I do not see that we use the loop variable anywhere in the code.

You're right - the loop variable can be dropped entirely.

#25 lore raw

PATCH v3 11/12 perf_event_open: Handle absence of PMU gracefully

In reply to Punit Agrawal

From: "Suzuki K. Poulose" <[email protected]>

The perf_event_open0{1,2} syscall tests fail for an absence of a PMU (which returns ENODEV). Handle the return code gracefully as we check for ENOENT.

Without the patch:

$ perf_event_open01
perf_event_open01    1  TFAIL  :  perf_event_open01.c:162: perf_event_open \
		failed unexpectedly: TEST_ERRNO=ENODEV(19): No such device

With the patch:

$ perf_event_open01
perf_event_open01    1  TCONF  :  perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_INSTRUCTIONS : No such device
perf_event_open01    2  TCONF  :  perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_CACHE_REFERENCES : No such device
perf_event_open01    3  TCONF  :  perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_CACHE_MISSES : No such device
perf_event_open01    4  TCONF  :  perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_BRANCH_INSTRUCTIONS : No such file or directory
perf_event_open01    5  TCONF  :  perf_event_open01.c:159: perf_event_open for PERF_COUNT_HW_BRANCH_MISSES : No such device
perf_event_open01    0  TINFO  :  read event counter succeeded, value: 833765520
perf_event_open01    6  TPASS  :  test PERF_TYPE_HARDWARE: PERF_COUNT_SW_CPU_CLOCK succeeded
perf_event_open01    0  TINFO  :  read event counter succeeded, value: 833967380
perf_event_open01    7  TPASS  :  test PERF_TYPE_HARDWARE: PERF_COUNT_SW_TASK_CLOCK succeeded
---
 testcases/kernel/syscalls/perf_event_open/perf_event_open01.c | 3 ++-
 testcases/kernel/syscalls/perf_event_open/perf_event_open02.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)
testcases/kernel/syscalls/perf_event_open/perf_event_open01.c +2 -1
index 5c814b60c..5568035d9 100644
@@ -149,7 +149,8 @@ static void verify(struct test_case_t *tc)
 
 	TEST(perf_event_open(&pe, 0, -1, -1, 0));
 	if (TEST_RETURN == -1) {
-		if (TEST_ERRNO == ENOENT || TEST_ERRNO == EOPNOTSUPP) {
+		if (TEST_ERRNO == ENOENT || TEST_ERRNO == EOPNOTSUPP ||
+		    TEST_ERRNO == ENODEV) {
 			tst_resm(TCONF | TTERRNO,
 			         "perf_event_open for %s not supported",
 			         tc->config_name);
testcases/kernel/syscalls/perf_event_open/perf_event_open02.c +2 -2
index 7d54cbd52..13a17948a 100644
@@ -160,8 +160,8 @@ static int count_hardware_counters(void)
 	for (i = 0; i < MAX_CTRS; i++) {
 		fdarry[i] = perf_event_open(&hw_event, 0, -1, -1, 0);
 		if (fdarry[i] == -1) {
-			if (errno == ENOENT) {
-				tst_brkm(TCONF, cleanup,
+			if (errno == ENOENT || errno == ENODEV) {
+				tst_brkm(TCONF | TERRNO, cleanup,
 				         "PERF_COUNT_HW_INSTRUCTIONS not supported");
 			}
 			tst_brkm(TBROK | TERRNO, cleanup,
Signature · 1 line
2.15.0
#26 lore raw

PATCH v3 12/12 hotplug/cpu_hotplug: Remove bashism disown from kill_pid()

In reply to Punit Agrawal

From: James Morse <[email protected]>

disown is a bashism not present in dash. kill_pid() already sends the process SIGKILL.

Remove the "disown" to allow running in shells that don't support disown.

---
 testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_testsuite.sh | 1 -
 1 file changed, 1 deletion(-)
testcases/kernel/hotplug/cpu_hotplug/include/cpuhotplug_testsuite.sh +0 -1
index 2d0166cc6..7dd0ebaae 100644
@@ -74,7 +74,6 @@ pid_is_valid()
 kill_pid()
 {
     PID=$1
-    disown $PID
     kill -9 $PID > /dev/null 2>&1
 }
 
Signature · 1 line
2.15.0
#28 lore raw

PATCH v3 12/12 hotplug/cpu_hotplug: Remove bashism disown from kill_pid()

Hi Petr,

Quoted 5 lines · Petr Vorel
Quoted 4 lines

From: James Morse <[email protected]>

disown is a bashism not present in dash. kill_pid() already sends the process SIGKILL.

Quoted 7 lines
Quoted 6 lines

Remove the "disown" to allow running in shells that don't support disown.

---

Thanks for looking through the patches and providing Acks.

Regards,
Punit