| Message ID | 1522070895-2289-1-git-send-email-xzhou@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fcntl37: test posix lock across execve | expand |
On Mon, Mar 26, 2018 at 09:28:15PM +0800, Xiong Zhou wrote: > Signed-off-by: Xiong Zhou <xzhou@redhat.com> > --- > testcases/kernel/syscalls/fcntl/Makefile | 3 + > testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++ > 2 files changed, 149 insertions(+) > create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c > > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile > index ae37214..7a06aa7 100644 > --- a/testcases/kernel/syscalls/fcntl/Makefile > +++ b/testcases/kernel/syscalls/fcntl/Makefile > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread > fcntl36: LDLIBS += -lpthread > fcntl36_64: LDLIBS += -lpthread > > +fcntl37: LDLIBS += -lpthread > +fcntl37_64: LDLIBS += -lpthread > + > include $(top_srcdir)/include/mk/testcases.mk > include $(abs_srcdir)/../utils/newer_64.mk > > diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c > new file mode 100644 > index 0000000..bac2168 > --- /dev/null > +++ b/testcases/kernel/syscalls/fcntl/fcntl37.c > @@ -0,0 +1,146 @@ > +/* > + * Copyright (c) 2018 RedHat Inc. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it would be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > + * Author: Xiong Zhou <xzhou@redhat.com> > + * > + * This is testing for > + * > + * "Record locks are not inherited by a child created via fork(2), > + * but are preserved across an execve(2)." > + * > + * from fcntl(2) man page. > + * > + */ > + > +#include <sys/types.h> > +#include <sys/stat.h> > +#include <sys/wait.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <errno.h> > +#include <unistd.h> > +#include <fcntl.h> > +#include <pthread.h> > +#include <sched.h> > +#include <limits.h> > + > +#include "lapi/fcntl.h" > +#include "tst_safe_pthread.h" > +#include "tst_test.h" > + > +static const char fname[] = "tst_lock_execve"; > +static const char flag_fname[] = "/tmp/execved"; > > +static void cleanup(void); > + > +static void *thread_fn(void *arg) > +{ > + int fd = *(int *)arg; > + tst_res(TINFO, "Thread running. fd %d", fd); > + /* Just need to be alive when execve. */ > + sleep(5); Since we don't expect this thread to complete running before execve() takes place, feels more robust to put put it into an infinite loop eg while (1) sleep (1); > + SAFE_CLOSE(fd); and ditch this since it is not supposed to be reachable if the test is operating correctly. > + return NULL; > +} > + > +static void checklock(int fd) > +{ > + pid_t pid = SAFE_FORK(); > + if (pid == 0) { > + struct flock flck = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 1, > + }; > + SAFE_FCNTL(fd, F_GETLK, &flck); > + if (flck.l_type == F_UNLCK) > + tst_res(TFAIL, "Record lock gets lost after execve"); > + else > + tst_res(TPASS, "Record lock survives execve"); > + SAFE_CLOSE(fd); > + _exit(0); > + } > + waitpid(pid, NULL, 0); > +} > + > +static void test01(void) > +{ > + int fd, ret; > + struct stat stat_buf; > + > + /* > + * If tmp/tst_lock_execve exists, execve to run checklock. > + */ > + ret = stat(flag_fname, &stat_buf); > + if (ret == 0) { > + checklock(fd); Nothing seems to have initialized the 'fd' variable when it is used here. > + cleanup(); > + _exit(0); > + } > + > + /* > + * If tmp/tst_lock_execve does not exist, initialize it. > + */ > + SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); > + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); What's the benefit in using 2 temporary files - feels like using one would be sufficient, particularly since this code is never calling unlink(fname) so leaking the 2nd file on disk > + struct flock64 flck = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 1, > + }; > + SAFE_FCNTL(fd, F_SETLK, &flck); > + > + /* > + * Creat thread and keep it running after placing posix > + * write lock. > + */ > + pthread_t th; > + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd); > + sleep(1); > + > + /* > + * Clear CLOEXEC > + */ > + int flags=SAFE_FCNTL(fd, F_GETFD); > + flags &= ~FD_CLOEXEC; > + SAFE_FCNTL(fd, F_SETFD, flags); > + > + /* > + * Get full path name of running programm then execve. > + */ > + char prog_name[PATH_MAX]; > + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); > + char * const newargv[] = { prog_name, NULL, NULL }; > + tst_res(TINFO, "execve %s with %s locked", prog_name, fname); > + execve(prog_name, newargv, NULL); > + /* > + * Failure info for debug > + */ > + perror("execve "); > +} > + > +static void cleanup(void) > +{ > + SAFE_UNLINK(flag_fname); > +} > + > +static struct tst_test test = { > + .needs_tmpdir = 1, > + .forks_child = 1, > + .test_all = test01, > + .cleanup = cleanup, > +}; > -- > 1.8.3.1 > Regards, Daniel
Hi! > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile > index ae37214..7a06aa7 100644 > --- a/testcases/kernel/syscalls/fcntl/Makefile > +++ b/testcases/kernel/syscalls/fcntl/Makefile > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread > fcntl36: LDLIBS += -lpthread > fcntl36_64: LDLIBS += -lpthread > > +fcntl37: LDLIBS += -lpthread > +fcntl37_64: LDLIBS += -lpthread This is wrong, we should use CFLAGS += -pthread instead, also the rest of the Makefile should be fixed as well. > include $(top_srcdir)/include/mk/testcases.mk > include $(abs_srcdir)/../utils/newer_64.mk ... > +#include "lapi/fcntl.h" > +#include "tst_safe_pthread.h" > +#include "tst_test.h" > + > +static const char fname[] = "tst_lock_execve"; > +static const char flag_fname[] = "/tmp/execved"; ^ You must not create files in random places. Once you set the .needs_tmpdir in the test structure the test is executed with unique temporary directory as working directory so all you need to do for temporary files is to work with relative filenames. > +static void cleanup(void); > + > +static void *thread_fn(void *arg) > +{ > + int fd = *(int *)arg; ^ This is broken, you have to use intptr_t > + tst_res(TINFO, "Thread running. fd %d", fd); > + /* Just need to be alive when execve. */ > + sleep(5); > + SAFE_CLOSE(fd); > + return NULL; > +} > + > +static void checklock(int fd) > +{ > + pid_t pid = SAFE_FORK(); > + if (pid == 0) { > + struct flock flck = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 1, > + }; > + SAFE_FCNTL(fd, F_GETLK, &flck); > + if (flck.l_type == F_UNLCK) > + tst_res(TFAIL, "Record lock gets lost after execve"); > + else > + tst_res(TPASS, "Record lock survives execve"); > + SAFE_CLOSE(fd); > + _exit(0); Why _exit() ? > + } > + waitpid(pid, NULL, 0); SAFE_WAITPID() > +} Also having the check in a separate function and calling it several times makes it kind of hard for debugging. if one of the assertions fails the error messages would be the same. Maybe we should pass a string with describes assertion type to the function and use it int the tst_res() messages. > +static void test01(void) > +{ > + int fd, ret; > + struct stat stat_buf; > + > + /* > + * If tmp/tst_lock_execve exists, execve to run checklock. > + */ > + ret = stat(flag_fname, &stat_buf); > + if (ret == 0) { > + checklock(fd); > + cleanup(); You must not call the cleanup from the test. Cleanup function is called from the test library. > + _exit(0); Again why _exit() ? > + } Don't do this. You must not reexecute the testcase and put file flags into random places. If you need execve() a test helper, you have to write simple binary helper. See testcases/kernel/syscalls/creat/creat07_child.c > + /* > + * If tmp/tst_lock_execve does not exist, initialize it. > + */ Please no useless comments like this one. > + SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); > + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); > + struct flock64 flck = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 1, > + }; > + SAFE_FCNTL(fd, F_SETLK, &flck); > + > + /* > + * Creat thread and keep it running after placing posix > + * write lock. > + */ Please no useless comments. > + pthread_t th; > + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd); ^ Here as well, we have to cast the fd to intptr_t first. > + sleep(1); Please no random sleeps() for thread synchronization, either use real thread synchronization primitives or LTP checkpoint synchronization library which is based on futexes. See: https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#229-fork-and-parent-child-synchronization > + /* > + * Clear CLOEXEC > + */ Please no useless comments. > + int flags=SAFE_FCNTL(fd, F_GETFD); > + flags &= ~FD_CLOEXEC; > + SAFE_FCNTL(fd, F_SETFD, flags); > + > + /* > + * Get full path name of running programm then execve. > + */ Please no useless comments. > + char prog_name[PATH_MAX]; > + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); > + char * const newargv[] = { prog_name, NULL, NULL }; This is far to complicated for no reason. The path to LTP test binaries is in $PATH so you can execute any LTP binary just by it's name. Absolute path is absolutely unneeded. > + tst_res(TINFO, "execve %s with %s locked", prog_name, fname); > + execve(prog_name, newargv, NULL); > + /* > + * Failure info for debug > + */ Please no useless comments. > + perror("execve "); No perror() please. Any error messages should be printed via tst_res() or tst_brk(). > +} > + > +static void cleanup(void) > +{ > + SAFE_UNLINK(flag_fname); > +} > + > +static struct tst_test test = { > + .needs_tmpdir = 1, > + .forks_child = 1, > + .test_all = test01, > + .cleanup = cleanup, > +}; > -- > 1.8.3.1 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > > + pthread_t th; > > + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd); > ^ > Here as well, we > have to cast the fd > to intptr_t first. Scratch that, you are passing the fd by pointer not by value, which is questionable practice for a value on the function stack. What is usually done when you need to pass integers to threads as paramters is to cast the integer to a type with the same width as a pointer then casting it to a void*, which is (void*)(intptr_t)fd in this case.
Hi, Thanks for reviewing! On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote: > Hi! > > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile > > index ae37214..7a06aa7 100644 > > --- a/testcases/kernel/syscalls/fcntl/Makefile > > +++ b/testcases/kernel/syscalls/fcntl/Makefile > > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread > > fcntl36: LDLIBS += -lpthread > > fcntl36_64: LDLIBS += -lpthread > > > > +fcntl37: LDLIBS += -lpthread > > +fcntl37_64: LDLIBS += -lpthread > > This is wrong, we should use CFLAGS += -pthread instead, also the rest ^ This is not working. gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o fcntl37.c gcc -L../../../../lib fcntl37_64.o -lltp -o fcntl37_64 ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create': /home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create' ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join': /home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join' collect2: error: ld returned 1 exit status > of the Makefile should be fixed as well. > > > include $(top_srcdir)/include/mk/testcases.mk > > include $(abs_srcdir)/../utils/newer_64.mk > > ... > > > +#include "lapi/fcntl.h" > > +#include "tst_safe_pthread.h" > > +#include "tst_test.h" > > + > > +static const char fname[] = "tst_lock_execve"; > > +static const char flag_fname[] = "/tmp/execved"; > ^ > You must not create files in random > places. Once you set the > .needs_tmpdir in the test structure > the test is executed with unique temporary > directory as working directory so > all you need to do for temporary > files is to work with relative filenames. > > > +static void cleanup(void); > > + > > +static void *thread_fn(void *arg) > > +{ > > + int fd = *(int *)arg; > ^ > This is broken, you have to use intptr_t > > > + tst_res(TINFO, "Thread running. fd %d", fd); > > + /* Just need to be alive when execve. */ > > + sleep(5); > > + SAFE_CLOSE(fd); > > + return NULL; > > +} > > + > > +static void checklock(int fd) > > +{ > > + pid_t pid = SAFE_FORK(); > > + if (pid == 0) { > > + struct flock flck = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_start = 0, > > + .l_len = 1, > > + }; > > + SAFE_FCNTL(fd, F_GETLK, &flck); > > + if (flck.l_type == F_UNLCK) > > + tst_res(TFAIL, "Record lock gets lost after execve"); > > + else > > + tst_res(TPASS, "Record lock survives execve"); > > + SAFE_CLOSE(fd); > > + _exit(0); > > Why _exit() ? > > > + } > > + waitpid(pid, NULL, 0); > > SAFE_WAITPID() > > > +} > > Also having the check in a separate function and calling it several > times makes it kind of hard for debugging. if one of the assertions > fails the error messages would be the same. Maybe we should pass a > string with describes assertion type to the function and use it > int the tst_res() messages. > > > +static void test01(void) > > +{ > > + int fd, ret; > > + struct stat stat_buf; > > + > > + /* > > + * If tmp/tst_lock_execve exists, execve to run checklock. > > + */ > > + ret = stat(flag_fname, &stat_buf); > > + if (ret == 0) { > > + checklock(fd); > > + cleanup(); > > You must not call the cleanup from the test. Cleanup function is called > from the test library. > > > + _exit(0); > > Again why _exit() ? > > > + } > > Don't do this. > > You must not reexecute the testcase and put file flags into random > places. If you need execve() a test helper, you have to write simple > binary helper. See testcases/kernel/syscalls/creat/creat07_child.c Thanks for the pointer. If I can define TST_NO_DEFAULT_MAIN and have main() function in this testcase, file flag is not needed. Passing args would do the trick, as Daniel's original reproducer. Thanks, Xiong > > > + /* > > + * If tmp/tst_lock_execve does not exist, initialize it. > > + */ > > Please no useless comments like this one. > > > + SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); > > + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); > > + struct flock64 flck = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_start = 0, > > + .l_len = 1, > > + }; > > + SAFE_FCNTL(fd, F_SETLK, &flck); > > + > > + /* > > + * Creat thread and keep it running after placing posix > > + * write lock. > > + */ > > Please no useless comments. > > > + pthread_t th; > > + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd); > ^ > Here as well, we > have to cast the fd > to intptr_t first. > > + sleep(1); > > Please no random sleeps() for thread synchronization, either use real > thread synchronization primitives or LTP checkpoint synchronization > library which is based on futexes. > > See: > https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#229-fork-and-parent-child-synchronization > > > + /* > > + * Clear CLOEXEC > > + */ > > Please no useless comments. > > > + int flags=SAFE_FCNTL(fd, F_GETFD); > > + flags &= ~FD_CLOEXEC; > > + SAFE_FCNTL(fd, F_SETFD, flags); > > + > > + /* > > + * Get full path name of running programm then execve. > > + */ > > Please no useless comments. > > > > + char prog_name[PATH_MAX]; > > + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); > > + char * const newargv[] = { prog_name, NULL, NULL }; > > This is far to complicated for no reason. The path to LTP test binaries > is in $PATH so you can execute any LTP binary just by it's name. > Absolute path is absolutely unneeded. > > > + tst_res(TINFO, "execve %s with %s locked", prog_name, fname); > > + execve(prog_name, newargv, NULL); > > + /* > > + * Failure info for debug > > + */ > > Please no useless comments. > > > + perror("execve "); > > No perror() please. Any error messages should be printed via tst_res() > or tst_brk(). > > > +} > > + > > +static void cleanup(void) > > +{ > > + SAFE_UNLINK(flag_fname); > > +} > > + > > +static struct tst_test test = { > > + .needs_tmpdir = 1, > > + .forks_child = 1, > > + .test_all = test01, > > + .cleanup = cleanup, > > +}; > > -- > > 1.8.3.1 > > > > > > -- > > Mailing list info: https://lists.linux.it/listinfo/ltp > > -- > Cyril Hrubis > chrubis@suse.cz
Hi! > > > +fcntl37: LDLIBS += -lpthread > > > +fcntl37_64: LDLIBS += -lpthread > > > > This is wrong, we should use CFLAGS += -pthread instead, also the rest > ^ This is not working. > > gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o fcntl37.c > gcc -L../../../../lib fcntl37_64.o -lltp -o fcntl37_64 > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create': > /home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create' > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join': > /home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join' > collect2: error: ld returned 1 exit status Hmm, looks like something is wrong with the newer_64.mk. It picks up a linker rule that does not include CFLAGS. Following patch fixes that, but I'm not 100% sure that it's correct, I will look further into that. diff --git a/testcases/kernel/syscalls/utils/newer_64.mk b/testcases/kernel/syscalls/utils/newer_64.mk index 8cd7e03c8..c70c2af53 100644 --- a/testcases/kernel/syscalls/utils/newer_64.mk +++ b/testcases/kernel/syscalls/utils/newer_64.mk @@ -49,7 +49,9 @@ HAS_NEWER_64 := 0 endif %_64: CFLAGS += -D$(DEF_64)=1 -# XXX (garrcoop): End section of code in question.. %_64.o: %.c $(COMPILE.c) $(OUTPUT_OPTION) $< + +%_64: %_64.o + $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS)
On Tue, Mar 27, 2018 at 12:19:29PM +0200, Cyril Hrubis wrote: > Hi! > > > > +fcntl37: LDLIBS += -lpthread > > > > +fcntl37_64: LDLIBS += -lpthread > > > > > > This is wrong, we should use CFLAGS += -pthread instead, also the rest > > ^ This is not working. > > > > gcc -g -O2 -g -O2 -fno-strict-aliasing -pipe -Wall -W -Wold-style-definition -DTST_USE_NEWER64_SYSCALL=1 -pthread -D_FORTIFY_SOURCE=2 -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl -I/home/xzhou/ltp/testcases/kernel/syscalls/fcntl/../utils -D_GNU_SOURCE -I../../../../include -I../../../../include -I../../../../include/old/ -D_FILE_OFFSET_BITS=64 -c -o fcntl37_64.o fcntl37.c > > gcc -L../../../../lib fcntl37_64.o -lltp -o fcntl37_64 > > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_create': > > /home/xzhou/ltp/lib/safe_pthread.c:30: undefined reference to `pthread_create' > > ../../../../lib/libltp.a(safe_pthread.o): In function `safe_pthread_join': > > /home/xzhou/ltp/lib/safe_pthread.c:46: undefined reference to `pthread_join' > > collect2: error: ld returned 1 exit status > > Hmm, looks like something is wrong with the newer_64.mk. It picks up a > linker rule that does not include CFLAGS. Following patch fixes that, > but I'm not 100% sure that it's correct, I will look further into that. > > diff --git a/testcases/kernel/syscalls/utils/newer_64.mk b/testcases/kernel/syscalls/utils/newer_64.mk > index 8cd7e03c8..c70c2af53 100644 > --- a/testcases/kernel/syscalls/utils/newer_64.mk > +++ b/testcases/kernel/syscalls/utils/newer_64.mk > @@ -49,7 +49,9 @@ HAS_NEWER_64 := 0 > endif > > %_64: CFLAGS += -D$(DEF_64)=1 > -# XXX (garrcoop): End section of code in question.. > > %_64.o: %.c > $(COMPILE.c) $(OUTPUT_OPTION) $< > + > +%_64: %_64.o > + $(LINK.c) $(OUTPUT_OPTION) $< $(LDLIBS) This patch works fine. Thanks, Xiong > > > -- > Cyril Hrubis > chrubis@suse.cz
On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote: > Hi! > > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile > > index ae37214..7a06aa7 100644 > > --- a/testcases/kernel/syscalls/fcntl/Makefile > > +++ b/testcases/kernel/syscalls/fcntl/Makefile > > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread > > fcntl36: LDLIBS += -lpthread > > fcntl36_64: LDLIBS += -lpthread > > ...snip...... > > + char prog_name[PATH_MAX]; > > + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); > > + char * const newargv[] = { prog_name, NULL, NULL }; > > This is far to complicated for no reason. The path to LTP test binaries > is in $PATH so you can execute any LTP binary just by it's name. > Absolute path is absolutely unneeded. According to man 3 execve and my tests, i see absolute path is needed. The perror after execve reports "No such file or directory". Thanks, Xiong
On Mon, Mar 26, 2018 at 04:33:33PM +0200, Cyril Hrubis wrote: > Hi! > > diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile > > index ae37214..7a06aa7 100644 > > --- a/testcases/kernel/syscalls/fcntl/Makefile > > +++ b/testcases/kernel/syscalls/fcntl/Makefile > > @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread > > fcntl36: LDLIBS += -lpthread > > fcntl36_64: LDLIBS += -lpthread > > > > +fcntl37: LDLIBS += -lpthread > > +fcntl37_64: LDLIBS += -lpthread > > This is wrong, we should use CFLAGS += -pthread instead, also the rest > of the Makefile should be fixed as well. > > > include $(top_srcdir)/include/mk/testcases.mk > > include $(abs_srcdir)/../utils/newer_64.mk > > ... > > > +#include "lapi/fcntl.h" > > +#include "tst_safe_pthread.h" > > +#include "tst_test.h" > > + > > +static const char fname[] = "tst_lock_execve"; > > +static const char flag_fname[] = "/tmp/execved"; > ^ > You must not create files in random > places. Once you set the > .needs_tmpdir in the test structure > the test is executed with unique temporary > directory as working directory so > all you need to do for temporary > files is to work with relative filenames. > > > +static void cleanup(void); > > + > > +static void *thread_fn(void *arg) > > +{ > > + int fd = *(int *)arg; > ^ > This is broken, you have to use intptr_t > > > + tst_res(TINFO, "Thread running. fd %d", fd); > > + /* Just need to be alive when execve. */ > > + sleep(5); > > + SAFE_CLOSE(fd); > > + return NULL; > > +} > > + > > +static void checklock(int fd) > > +{ > > + pid_t pid = SAFE_FORK(); > > + if (pid == 0) { > > + struct flock flck = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_start = 0, > > + .l_len = 1, > > + }; > > + SAFE_FCNTL(fd, F_GETLK, &flck); > > + if (flck.l_type == F_UNLCK) > > + tst_res(TFAIL, "Record lock gets lost after execve"); > > + else > > + tst_res(TPASS, "Record lock survives execve"); > > + SAFE_CLOSE(fd); > > + _exit(0); > > Why _exit() ? > > > + } > > + waitpid(pid, NULL, 0); > > SAFE_WAITPID() > > > +} > > Also having the check in a separate function and calling it several > times makes it kind of hard for debugging. if one of the assertions > fails the error messages would be the same. Maybe we should pass a > string with describes assertion type to the function and use it > int the tst_res() messages. > > > +static void test01(void) > > +{ > > + int fd, ret; > > + struct stat stat_buf; > > + > > + /* > > + * If tmp/tst_lock_execve exists, execve to run checklock. > > + */ > > + ret = stat(flag_fname, &stat_buf); > > + if (ret == 0) { > > + checklock(fd); > > + cleanup(); > > You must not call the cleanup from the test. Cleanup function is called > from the test library. > > > + _exit(0); > > Again why _exit() ? > > > + } > > Don't do this. > > You must not reexecute the testcase and put file flags into random > places. If you need execve() a test helper, you have to write simple > binary helper. See testcases/kernel/syscalls/creat/creat07_child.c Finally get this helper working but does not reproduce the original bug we are trying to cover. I guess the testcase re-exec is necessary here. If this rule is unshakeable, I'll try upstreaming it to other test suite. Rule is rule, I respect rules. THanks, Xiong
Hi! > Finally get this helper working but does not reproduce the original > bug we are trying to cover. I guess the testcase re-exec is necessary > here. Can't we have the helper to re-exec itself?
diff --git a/testcases/kernel/syscalls/fcntl/Makefile b/testcases/kernel/syscalls/fcntl/Makefile index ae37214..7a06aa7 100644 --- a/testcases/kernel/syscalls/fcntl/Makefile +++ b/testcases/kernel/syscalls/fcntl/Makefile @@ -27,6 +27,9 @@ fcntl34_64: LDLIBS += -lpthread fcntl36: LDLIBS += -lpthread fcntl36_64: LDLIBS += -lpthread +fcntl37: LDLIBS += -lpthread +fcntl37_64: LDLIBS += -lpthread + include $(top_srcdir)/include/mk/testcases.mk include $(abs_srcdir)/../utils/newer_64.mk diff --git a/testcases/kernel/syscalls/fcntl/fcntl37.c b/testcases/kernel/syscalls/fcntl/fcntl37.c new file mode 100644 index 0000000..bac2168 --- /dev/null +++ b/testcases/kernel/syscalls/fcntl/fcntl37.c @@ -0,0 +1,146 @@ +/* + * Copyright (c) 2018 RedHat Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Author: Xiong Zhou <xzhou@redhat.com> + * + * This is testing for + * + * "Record locks are not inherited by a child created via fork(2), + * but are preserved across an execve(2)." + * + * from fcntl(2) man page. + * + */ + +#include <sys/types.h> +#include <sys/stat.h> +#include <sys/wait.h> +#include <stdio.h> +#include <stdlib.h> +#include <errno.h> +#include <unistd.h> +#include <fcntl.h> +#include <pthread.h> +#include <sched.h> +#include <limits.h> + +#include "lapi/fcntl.h" +#include "tst_safe_pthread.h" +#include "tst_test.h" + +static const char fname[] = "tst_lock_execve"; +static const char flag_fname[] = "/tmp/execved"; +static void cleanup(void); + +static void *thread_fn(void *arg) +{ + int fd = *(int *)arg; + tst_res(TINFO, "Thread running. fd %d", fd); + /* Just need to be alive when execve. */ + sleep(5); + SAFE_CLOSE(fd); + return NULL; +} + +static void checklock(int fd) +{ + pid_t pid = SAFE_FORK(); + if (pid == 0) { + struct flock flck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_GETLK, &flck); + if (flck.l_type == F_UNLCK) + tst_res(TFAIL, "Record lock gets lost after execve"); + else + tst_res(TPASS, "Record lock survives execve"); + SAFE_CLOSE(fd); + _exit(0); + } + waitpid(pid, NULL, 0); +} + +static void test01(void) +{ + int fd, ret; + struct stat stat_buf; + + /* + * If tmp/tst_lock_execve exists, execve to run checklock. + */ + ret = stat(flag_fname, &stat_buf); + if (ret == 0) { + checklock(fd); + cleanup(); + _exit(0); + } + + /* + * If tmp/tst_lock_execve does not exist, initialize it. + */ + SAFE_OPEN(flag_fname, O_RDWR|O_CREAT, 0755); + fd = SAFE_OPEN(fname, O_RDWR|O_CREAT, 0755); + struct flock64 flck = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 1, + }; + SAFE_FCNTL(fd, F_SETLK, &flck); + + /* + * Creat thread and keep it running after placing posix + * write lock. + */ + pthread_t th; + SAFE_PTHREAD_CREATE(&th, NULL, thread_fn, (void *)&fd); + sleep(1); + + /* + * Clear CLOEXEC + */ + int flags=SAFE_FCNTL(fd, F_GETFD); + flags &= ~FD_CLOEXEC; + SAFE_FCNTL(fd, F_SETFD, flags); + + /* + * Get full path name of running programm then execve. + */ + char prog_name[PATH_MAX]; + ret = readlink("/proc/self/exe", prog_name, PATH_MAX); + char * const newargv[] = { prog_name, NULL, NULL }; + tst_res(TINFO, "execve %s with %s locked", prog_name, fname); + execve(prog_name, newargv, NULL); + /* + * Failure info for debug + */ + perror("execve "); +} + +static void cleanup(void) +{ + SAFE_UNLINK(flag_fname); +} + +static struct tst_test test = { + .needs_tmpdir = 1, + .forks_child = 1, + .test_all = test01, + .cleanup = cleanup, +};
Signed-off-by: Xiong Zhou <xzhou@redhat.com> --- testcases/kernel/syscalls/fcntl/Makefile | 3 + testcases/kernel/syscalls/fcntl/fcntl37.c | 146 ++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 testcases/kernel/syscalls/fcntl/fcntl37.c