Collection of fixes
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
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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v3 1/12 Move check_hugepage() helper to mem/lib
Quoted 6 lines
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.Signed-off-by Punit Agrawal <[email protected]>---
PATCH v3 2/12 thp: ensure hugetlbfs is available
In reply to Punit AgrawalFrom: 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
@@ -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
@@ -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
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/meminfodoes 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.
Signed-off-by Will Deacon <[email protected]>Signed-off-by Punit Agrawal <[email protected]>---
PATCH v3 3/12 hugeshmctl01: Convert to LTP synchronisation primitives
In reply to Punit Agrawalhugeshmctl01 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
@@ -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
PATCH v3 4/12 hugeshmctl01: Fix warning about signed/unsigned comparison
In reply to Punit AgrawalThe 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
@@ -46,7 +46,7 @@#define FIRST 0#define SECOND 1 -#define N_ATTACH 4 +#define N_ATTACH 4U#define NEWMODE 0066static 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
PATCH v3 4/12 hugeshmctl01: Fix warning about signed/unsigned comparison
Quoted 4 lines
The macro
N_ATTACHis 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_ATTACHto be an unsigned integer and fixing the resulting fallout.Signed-off-by Punit Agrawal <[email protected]>---
PATCH v3 5/12 hugeshmctl02: Fix allocation size for odd number of hugepages
In reply to Punit AgrawalFrom: "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
@@ -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
PATCH v3 6/12 hugeshmget02: add missing SHM_HUGETLB flag on segment creation
In reply to Punit AgrawalFrom: 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
@@ -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
PATCH v3 7/12 sigwaitinfo01: fix race between sending and dequeueing RT signals
In reply to Punit AgrawalFrom: 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
@@ -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
PATCH v3 8/12 sigwaitinfo01: catch SEGV and report success for bad_address2 testcase
In reply to Punit AgrawalThe 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
@@ -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
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]
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
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]
PATCH v3 9/12 syscalls/mount03: Copy setuid_test to execute instead of 'TEST FILE'
In reply to Punit AgrawalWhen 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
@@ -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
Signature · 1 line
2.15.0
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
-4case 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]
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
-4case 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?
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
-4case 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]
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
-4case 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. :)
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
_aswellas_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]
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
_aswellas_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.
PATCH v3 10/12 getdtablesize01: Handle ENFILE errno
In reply to Punit AgrawalFrom: "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
@@ -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
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]
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.
PATCH v3 11/12 perf_event_open: Handle absence of PMU gracefully
In reply to Punit AgrawalFrom: "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
@@ -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
@@ -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
PATCH v3 12/12 hotplug/cpu_hotplug: Remove bashism disown from kill_pid()
In reply to Punit AgrawalFrom: 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
@@ -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
PATCH v3 12/12 hotplug/cpu_hotplug: Remove bashism disown from kill_pid()
Quoted 11 lines
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.
Signed-off-by James Morse <[email protected]>Signed-off-by Punit Agrawal <[email protected]>---
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.
Signed-off-by James Morse <[email protected]>Signed-off-by Punit Agrawal <[email protected]>---
Acked-by Petr Vorel <[email protected]>
Thanks for looking through the patches and providing Acks.
Regards,
Punit