* [LTP] [PATCH v3 1/3] lib: add ltp_alloc_stack() @ 2019-06-13 7:23 Jan Stancek 2019-06-13 7:24 ` [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc Jan Stancek 2019-06-13 7:24 ` [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child Jan Stancek 0 siblings, 2 replies; 17+ messages in thread From: Jan Stancek @ 2019-06-13 7:23 UTC (permalink / raw) To: ltp Meant to allocate stack with sufficient alignment for all arches. Use it in lib/cloner.c instead of malloc(). Signed-off-by: Jan Stancek <jstancek@redhat.com> --- include/tst_clone.h | 1 + lib/cloner.c | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/include/tst_clone.h b/include/tst_clone.h index 72145fabe7cd..786cee5d1209 100644 --- a/include/tst_clone.h +++ b/include/tst_clone.h @@ -33,6 +33,7 @@ int ltp_clone_malloc(unsigned long clone_flags, int (*fn)(void *arg), void *arg, size_t stacksize); int ltp_clone_quick(unsigned long clone_flags, int (*fn)(void *arg), void *arg); +void *ltp_alloc_stack(size_t size); #define clone(...) (use_the_ltp_clone_functions__do_not_use_clone) diff --git a/lib/cloner.c b/lib/cloner.c index 63f223d2a150..cf37184aa22a 100644 --- a/lib/cloner.c +++ b/lib/cloner.c @@ -28,8 +28,7 @@ #include <stdlib.h> #include <sched.h> #include <stdarg.h> -#include "test.h" -#include "config.h" +#include "tst_clone.h" #undef clone /* we want to use clone() */ @@ -118,6 +117,24 @@ int ltp_clone7(unsigned long flags, int (*fn)(void *arg), void *arg, } /* + * ltp_alloc_stack: allocate stack of size 'size', that is sufficiently + * aligned for all arches. User is responsible for freeing allocated + * memory. + * Returns pointer to new stack. On error, returns NULL with errno set. + */ +void *ltp_alloc_stack(size_t size) +{ + void *ret = NULL; + int err; + + err = posix_memalign(&ret, 64, size); + if (err) + errno = err; + + return ret; +} + +/* * ltp_clone_malloc: also does the memory allocation for clone with a * caller-specified size. */ @@ -129,7 +146,7 @@ ltp_clone_malloc(unsigned long clone_flags, int (*fn) (void *arg), void *arg, int ret; int saved_errno; - stack = malloc(stack_size); + stack = ltp_alloc_stack(stack_size); if (stack == NULL) return -1; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc 2019-06-13 7:23 [LTP] [PATCH v3 1/3] lib: add ltp_alloc_stack() Jan Stancek @ 2019-06-13 7:24 ` Jan Stancek 2019-06-13 8:55 ` Li Wang 2019-06-13 7:24 ` [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child Jan Stancek 1 sibling, 1 reply; 17+ messages in thread From: Jan Stancek @ 2019-06-13 7:24 UTC (permalink / raw) To: ltp There are no users outside of lib. Signed-off-by: Jan Stancek <jstancek@redhat.com> --- include/tst_clone.h | 2 +- lib/cloner.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/tst_clone.h b/include/tst_clone.h index 786cee5d1209..fd52097e2072 100644 --- a/include/tst_clone.h +++ b/include/tst_clone.h @@ -29,7 +29,7 @@ int ltp_clone(unsigned long flags, int (*fn)(void *arg), void *arg, size_t stack_size, void *stack); int ltp_clone7(unsigned long flags, int (*fn)(void *arg), void *arg, size_t stack_size, void *stack, ...); -int ltp_clone_malloc(unsigned long clone_flags, int (*fn)(void *arg), +int ltp_clone_alloc(unsigned long clone_flags, int (*fn)(void *arg), void *arg, size_t stacksize); int ltp_clone_quick(unsigned long clone_flags, int (*fn)(void *arg), void *arg); diff --git a/lib/cloner.c b/lib/cloner.c index cf37184aa22a..8469745d004b 100644 --- a/lib/cloner.c +++ b/lib/cloner.c @@ -135,11 +135,11 @@ void *ltp_alloc_stack(size_t size) } /* - * ltp_clone_malloc: also does the memory allocation for clone with a + * ltp_clone_alloc: also does the memory allocation for clone with a * caller-specified size. */ int -ltp_clone_malloc(unsigned long clone_flags, int (*fn) (void *arg), void *arg, +ltp_clone_alloc(unsigned long clone_flags, int (*fn) (void *arg), void *arg, size_t stack_size) { void *stack; @@ -162,7 +162,7 @@ ltp_clone_malloc(unsigned long clone_flags, int (*fn) (void *arg), void *arg, } /* - * ltp_clone_quick: calls ltp_clone_malloc with predetermined stack size. + * ltp_clone_quick: calls ltp_clone_alloc with predetermined stack size. * Experience thus far suggests that one page is often insufficient, * while 6*getpagesize() seems adequate. */ @@ -170,5 +170,5 @@ int ltp_clone_quick(unsigned long clone_flags, int (*fn) (void *arg), void *arg) { size_t stack_size = getpagesize() * 6; - return ltp_clone_malloc(clone_flags, fn, arg, stack_size); + return ltp_clone_alloc(clone_flags, fn, arg, stack_size); } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc 2019-06-13 7:24 ` [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc Jan Stancek @ 2019-06-13 8:55 ` Li Wang 2019-06-13 13:57 ` Cyril Hrubis 0 siblings, 1 reply; 17+ messages in thread From: Li Wang @ 2019-06-13 8:55 UTC (permalink / raw) To: ltp On Thu, Jun 13, 2019 at 3:25 PM Jan Stancek <jstancek@redhat.com> wrote: > There are no users outside of lib. > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > --- > include/tst_clone.h | 2 +- > lib/cloner.c | 8 ++++---- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/tst_clone.h b/include/tst_clone.h > index 786cee5d1209..fd52097e2072 100644 > --- a/include/tst_clone.h > +++ b/include/tst_clone.h > @@ -29,7 +29,7 @@ int ltp_clone(unsigned long flags, int (*fn)(void *arg), > void *arg, > size_t stack_size, void *stack); > int ltp_clone7(unsigned long flags, int (*fn)(void *arg), void *arg, > size_t stack_size, void *stack, ...); > -int ltp_clone_malloc(unsigned long clone_flags, int (*fn)(void *arg), > +int ltp_clone_alloc(unsigned long clone_flags, int (*fn)(void *arg), > void *arg, size_t stacksize); > int ltp_clone_quick(unsigned long clone_flags, int (*fn)(void *arg), > void *arg); > diff --git a/lib/cloner.c b/lib/cloner.c > index cf37184aa22a..8469745d004b 100644 > --- a/lib/cloner.c > +++ b/lib/cloner.c > @@ -135,11 +135,11 @@ void *ltp_alloc_stack(size_t size) > } > > /* > - * ltp_clone_malloc: also does the memory allocation for clone with a > + * ltp_clone_alloc: also does the memory allocation for clone with a > * caller-specified size. > */ > int > -ltp_clone_malloc(unsigned long clone_flags, int (*fn) (void *arg), void > *arg, > +ltp_clone_alloc(unsigned long clone_flags, int (*fn) (void *arg), void > *arg, > size_t stack_size) > { > void *stack; > @@ -162,7 +162,7 @@ ltp_clone_malloc(unsigned long clone_flags, int (*fn) > (void *arg), void *arg, > } > > /* > - * ltp_clone_quick: calls ltp_clone_malloc with predetermined stack size. > + * ltp_clone_quick: calls ltp_clone_alloc with predetermined stack size. > * Experience thus far suggests that one page is often insufficient, > * while 6*getpagesize() seems adequate. > */ > @@ -170,5 +170,5 @@ int ltp_clone_quick(unsigned long clone_flags, int > (*fn) (void *arg), void *arg) > { > size_t stack_size = getpagesize() * 6; > > - return ltp_clone_malloc(clone_flags, fn, arg, stack_size); > + return ltp_clone_alloc(clone_flags, fn, arg, stack_size); > } > There is another legacy problem maybe we need take care. Any thought about how releasing the stack memory[1] after calling ltp_clone_quick() in a test? [1] which allocated memory in ltp_clone_alloc(). -- Regards, Li Wang -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20190613/56fd0c52/attachment-0001.html> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc 2019-06-13 8:55 ` Li Wang @ 2019-06-13 13:57 ` Cyril Hrubis 2019-06-14 2:46 ` Li Wang 0 siblings, 1 reply; 17+ messages in thread From: Cyril Hrubis @ 2019-06-13 13:57 UTC (permalink / raw) To: ltp Hi! > > @@ -170,5 +170,5 @@ int ltp_clone_quick(unsigned long clone_flags, int > > (*fn) (void *arg), void *arg) > > { > > size_t stack_size = getpagesize() * 6; > > > > - return ltp_clone_malloc(clone_flags, fn, arg, stack_size); > > + return ltp_clone_alloc(clone_flags, fn, arg, stack_size); > > } > > > > There is another legacy problem maybe we need take care. > > Any thought about how releasing the stack memory[1] after calling > ltp_clone_quick() in a test? > > [1] which allocated memory in ltp_clone_alloc(). Well, we can free the memory in ltp_clone_alloc() if the child runs in a separate memory space, but if CLONE_VM was passed in flags there is no way how to free the memory... -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc 2019-06-13 13:57 ` Cyril Hrubis @ 2019-06-14 2:46 ` Li Wang 2019-06-14 15:24 ` Cyril Hrubis 0 siblings, 1 reply; 17+ messages in thread From: Li Wang @ 2019-06-14 2:46 UTC (permalink / raw) To: ltp On Thu, Jun 13, 2019 at 9:58 PM Cyril Hrubis <chrubis@suse.cz> wrote: > Hi! > > > @@ -170,5 +170,5 @@ int ltp_clone_quick(unsigned long clone_flags, int > > > (*fn) (void *arg), void *arg) > > > { > > > size_t stack_size = getpagesize() * 6; > > > > > > - return ltp_clone_malloc(clone_flags, fn, arg, stack_size); > > > + return ltp_clone_alloc(clone_flags, fn, arg, stack_size); > > > } > > > > > > > There is another legacy problem maybe we need take care. > > > > Any thought about how releasing the stack memory[1] after calling > > ltp_clone_quick() in a test? > > > > [1] which allocated memory in ltp_clone_alloc(). > > Well, we can free the memory in ltp_clone_alloc() if the child runs in a > separate memory space, but if CLONE_VM was passed in flags there is no > way how to free the memory... > A stupid way come up to my mind is just to export the stack as global pointer and releasing in do_cleanup() if possible. diff --git a/include/tst_clone.h b/include/tst_clone.h index fd52097..c98eb44 100644 --- a/include/tst_clone.h +++ b/include/tst_clone.h diff --git a/include/tst_clone.h b/include/tst_clone.h index fd52097..c98eb44 100644 --- a/include/tst_clone.h +++ b/include/tst_clone.h @@ -24,6 +24,8 @@ #ifndef TST_CLONE_H__ #define TST_CLONE_H__ +void *tst_clone_stack; + /* Functions from lib/cloner.c */ int ltp_clone(unsigned long flags, int (*fn)(void *arg), void *arg, size_t stack_size, void *stack); diff --git a/lib/cloner.c b/lib/cloner.c index 8469745..4534982 100644 --- a/lib/cloner.c +++ b/lib/cloner.c @@ -142,19 +142,17 @@ int ltp_clone_alloc(unsigned long clone_flags, int (*fn) (void *arg), void *arg, size_t stack_size) { - void *stack; int ret; int saved_errno; - - stack = ltp_alloc_stack(stack_size); - if (stack == NULL) + tst_clone_stack = ltp_alloc_stack(stack_size); + if (tst_clone_stack == NULL) return -1; - ret = ltp_clone(clone_flags, fn, arg, stack_size, stack); + ret = ltp_clone(clone_flags, fn, arg, stack_size, tst_clone_stack); if (ret == -1) { saved_errno = errno; - free(stack); + free(tst_clone_stack); errno = saved_errno; } diff --git a/lib/tst_test.c b/lib/tst_test.c index 95f389d..e96a9c7 100644 --- a/lib/tst_test.c +++ b/lib/tst_test.c @@ -909,6 +909,9 @@ static void do_cleanup(void) if (mntpoint_mounted) tst_umount(tst_test->mntpoint); + if (tst_clone_stack) + free(tst_clone_stack); + if (tst_test->needs_device && tdev.dev) tst_release_device(tdev.dev); -- Regards, Li Wang -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20190614/8e97e3c0/attachment-0001.html> ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc 2019-06-14 2:46 ` Li Wang @ 2019-06-14 15:24 ` Cyril Hrubis 0 siblings, 0 replies; 17+ messages in thread From: Cyril Hrubis @ 2019-06-14 15:24 UTC (permalink / raw) To: ltp Hi! > > Well, we can free the memory in ltp_clone_alloc() if the child runs in a > > separate memory space, but if CLONE_VM was passed in flags there is no > > way how to free the memory... > > > > A stupid way come up to my mind is just to export the stack as global > pointer and releasing in do_cleanup() if possible. This only works if you call the function only once, if you clone two or more you end up in the very same situation as before. Also we will have to figure out similar problem as a part of https://github.com/linux-test-project/ltp/issues/531 anyways, so I wouldn't bother at this point. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 7:23 [LTP] [PATCH v3 1/3] lib: add ltp_alloc_stack() Jan Stancek 2019-06-13 7:24 ` [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc Jan Stancek @ 2019-06-13 7:24 ` Jan Stancek 2019-06-13 8:25 ` Li Wang 1 sibling, 1 reply; 17+ messages in thread From: Jan Stancek @ 2019-06-13 7:24 UTC (permalink / raw) To: ltp Test crashes (SIGBUS) when using child stack have been observed for ioctl_ns01. This is because stack isn't aligned. Use ltp_alloc_stack() for stack allocation. Add SIGCHLD to clone flags, so that LTP library can reap all children and check their return code. Also check ltp_clone() return value. Suppress warning for unused *arg in child(). Signed-off-by: Jan Stancek <jstancek@redhat.com> --- testcases/kernel/syscalls/ioctl/ioctl_ns01.c | 13 +++++++++---- testcases/kernel/syscalls/ioctl/ioctl_ns05.c | 12 +++++++++--- testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 15 +++++++++++---- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c index dfde4da6c5d6..d241a5d0fa53 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c @@ -23,7 +23,7 @@ #define STACK_SIZE (1024 * 1024) -static char child_stack[STACK_SIZE]; +static char *child_stack; static void setup(void) { @@ -31,6 +31,10 @@ static void setup(void) if (exists < 0) tst_res(TCONF, "namespace not available"); + + child_stack = ltp_alloc_stack(STACK_SIZE); + if (!child_stack) + tst_brk(TBROK|TERRNO, "stack alloc"); } static void test_ns_get_parent(void) @@ -53,7 +57,7 @@ static void test_ns_get_parent(void) } } -static int child(void *arg) +static int child(void *arg LTP_ATTRIBUTE_UNUSED) { test_ns_get_parent(); return 0; @@ -63,8 +67,9 @@ static void run(void) { test_ns_get_parent(); - ltp_clone(CLONE_NEWPID, &child, 0, - STACK_SIZE, child_stack); + if (ltp_clone(CLONE_NEWPID | SIGCHLD, &child, 0, + STACK_SIZE, child_stack) == -1) + tst_brk(TBROK | TERRNO, "ltp_clone failed"); } static struct tst_test test = { diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns05.c b/testcases/kernel/syscalls/ioctl/ioctl_ns05.c index a8dee07a1154..7455ff17caf3 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_ns05.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns05.c @@ -22,7 +22,7 @@ #define STACK_SIZE (1024 * 1024) -static char child_stack[STACK_SIZE]; +static char *child_stack; static void setup(void) { @@ -30,9 +30,13 @@ static void setup(void) if (exists < 0) tst_res(TCONF, "namespace not available"); + + child_stack = ltp_alloc_stack(STACK_SIZE); + if (!child_stack) + tst_brk(TBROK|TERRNO, "stack alloc"); } -static int child(void *arg) +static int child(void *arg LTP_ATTRIBUTE_UNUSED) { if (getpid() != 1) tst_res(TFAIL, "child should think its pid is 1"); @@ -44,8 +48,10 @@ static int child(void *arg) static void run(void) { - pid_t pid = ltp_clone(CLONE_NEWPID, &child, 0, + pid_t pid = ltp_clone(CLONE_NEWPID | SIGCHLD, &child, 0, STACK_SIZE, child_stack); + if (pid == -1) + tst_brk(TBROK | TERRNO, "ltp_clone failed"); char child_namespace[20]; int my_fd, child_fd, parent_fd; diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns06.c b/testcases/kernel/syscalls/ioctl/ioctl_ns06.c index 805a0a072e2f..6b137e64ff25 100644 --- a/testcases/kernel/syscalls/ioctl/ioctl_ns06.c +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns06.c @@ -23,7 +23,7 @@ #define STACK_SIZE (1024 * 1024) -static char child_stack[STACK_SIZE]; +static char *child_stack; static void setup(void) { @@ -31,9 +31,13 @@ static void setup(void) if (exists < 0) tst_res(TCONF, "namespace not available"); + + child_stack = ltp_alloc_stack(STACK_SIZE); + if (!child_stack) + tst_brk(TBROK|TERRNO, "stack alloc"); } -static int child(void *arg) +static int child(void *arg LTP_ATTRIBUTE_UNUSED) { TST_CHECKPOINT_WAIT(0); return 0; @@ -41,10 +45,13 @@ static int child(void *arg) static void run(void) { - pid_t pid = ltp_clone(CLONE_NEWUSER, &child, 0, - STACK_SIZE, child_stack); char child_namespace[20]; + pid_t pid = ltp_clone(CLONE_NEWUSER | SIGCHLD, &child, 0, + STACK_SIZE, child_stack); + if (pid == -1) + tst_brk(TBROK | TERRNO, "ltp_clone failed"); + sprintf(child_namespace, "/proc/%i/ns/user", pid); int my_fd, child_fd, parent_fd; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 7:24 ` [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child Jan Stancek @ 2019-06-13 8:25 ` Li Wang 2019-06-13 10:16 ` Jan Stancek 0 siblings, 1 reply; 17+ messages in thread From: Li Wang @ 2019-06-13 8:25 UTC (permalink / raw) To: ltp On Thu, Jun 13, 2019 at 3:25 PM Jan Stancek <jstancek@redhat.com> wrote: > Test crashes (SIGBUS) when using child stack have been observed for > ioctl_ns01. This is because stack isn't aligned. Use ltp_alloc_stack() > for stack allocation. > > Add SIGCHLD to clone flags, so that LTP library can reap all children > and check their return code. Also check ltp_clone() return value. > > Suppress warning for unused *arg in child(). > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > --- > testcases/kernel/syscalls/ioctl/ioctl_ns01.c | 13 +++++++++---- > testcases/kernel/syscalls/ioctl/ioctl_ns05.c | 12 +++++++++--- > testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 15 +++++++++++---- > 3 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > index dfde4da6c5d6..d241a5d0fa53 100644 > --- a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > @@ -23,7 +23,7 @@ > > #define STACK_SIZE (1024 * 1024) > > -static char child_stack[STACK_SIZE]; > +static char *child_stack; > > static void setup(void) > { > @@ -31,6 +31,10 @@ static void setup(void) > > if (exists < 0) > tst_res(TCONF, "namespace not available"); > + > + child_stack = ltp_alloc_stack(STACK_SIZE); > As you commented that "User is responsible for freeing allocated memory", but you forget to do that in each of these test cases :). We need free(child_stack) in the cleanup function. -- Regards, Li Wang -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20190613/898bdd4e/attachment.html> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 8:25 ` Li Wang @ 2019-06-13 10:16 ` Jan Stancek 2019-06-13 11:26 ` Li Wang 2019-06-13 14:17 ` Cyril Hrubis 0 siblings, 2 replies; 17+ messages in thread From: Jan Stancek @ 2019-06-13 10:16 UTC (permalink / raw) To: ltp ----- Original Message ----- > On Thu, Jun 13, 2019 at 3:25 PM Jan Stancek <jstancek@redhat.com> wrote: > > > Test crashes (SIGBUS) when using child stack have been observed for > > ioctl_ns01. This is because stack isn't aligned. Use ltp_alloc_stack() > > for stack allocation. > > > > Add SIGCHLD to clone flags, so that LTP library can reap all children > > and check their return code. Also check ltp_clone() return value. > > > > Suppress warning for unused *arg in child(). > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > --- > > testcases/kernel/syscalls/ioctl/ioctl_ns01.c | 13 +++++++++---- > > testcases/kernel/syscalls/ioctl/ioctl_ns05.c | 12 +++++++++--- > > testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 15 +++++++++++---- > > 3 files changed, 29 insertions(+), 11 deletions(-) > > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > index dfde4da6c5d6..d241a5d0fa53 100644 > > --- a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > @@ -23,7 +23,7 @@ > > > > #define STACK_SIZE (1024 * 1024) > > > > -static char child_stack[STACK_SIZE]; > > +static char *child_stack; > > > > static void setup(void) > > { > > @@ -31,6 +31,10 @@ static void setup(void) > > > > if (exists < 0) > > tst_res(TCONF, "namespace not available"); > > + > > + child_stack = ltp_alloc_stack(STACK_SIZE); > > > > As you commented that "User is responsible for freeing allocated memory", > but you forget to do that in each of these test cases :). I omitted it on purpose. OS will clean it up on exit, since it's called only in setup() it's not going to keep leaking more memory. > > We need free(child_stack) in the cleanup function. Can you elaborate? > > -- > Regards, > Li Wang > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 10:16 ` Jan Stancek @ 2019-06-13 11:26 ` Li Wang 2019-06-13 14:17 ` Cyril Hrubis 1 sibling, 0 replies; 17+ messages in thread From: Li Wang @ 2019-06-13 11:26 UTC (permalink / raw) To: ltp On Thu, Jun 13, 2019 at 6:16 PM Jan Stancek <jstancek@redhat.com> wrote: > > > ----- Original Message ----- > > On Thu, Jun 13, 2019 at 3:25 PM Jan Stancek <jstancek@redhat.com> wrote: > > > > > Test crashes (SIGBUS) when using child stack have been observed for > > > ioctl_ns01. This is because stack isn't aligned. Use ltp_alloc_stack() > > > for stack allocation. > > > > > > Add SIGCHLD to clone flags, so that LTP library can reap all children > > > and check their return code. Also check ltp_clone() return value. > > > > > > Suppress warning for unused *arg in child(). > > > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > > --- > > > testcases/kernel/syscalls/ioctl/ioctl_ns01.c | 13 +++++++++---- > > > testcases/kernel/syscalls/ioctl/ioctl_ns05.c | 12 +++++++++--- > > > testcases/kernel/syscalls/ioctl/ioctl_ns06.c | 15 +++++++++++---- > > > 3 files changed, 29 insertions(+), 11 deletions(-) > > > > > > diff --git a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > > b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > > index dfde4da6c5d6..d241a5d0fa53 100644 > > > --- a/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > > +++ b/testcases/kernel/syscalls/ioctl/ioctl_ns01.c > > > @@ -23,7 +23,7 @@ > > > > > > #define STACK_SIZE (1024 * 1024) > > > > > > -static char child_stack[STACK_SIZE]; > > > +static char *child_stack; > > > > > > static void setup(void) > > > { > > > @@ -31,6 +31,10 @@ static void setup(void) > > > > > > if (exists < 0) > > > tst_res(TCONF, "namespace not available"); > > > + > > > + child_stack = ltp_alloc_stack(STACK_SIZE); > > > > > > > As you commented that "User is responsible for freeing allocated memory", > > but you forget to do that in each of these test cases :). > > I omitted it on purpose. OS will clean it up on exit, since it's called > only in setup() it's not going to keep leaking more memory. > Okay. I believe that, but it is not a recommended coding habit which I had been told ;-). > > > > > We need free(child_stack) in the cleanup function. > > Can you elaborate? > Nothing else. It is just a supplementary as above comment. If we do memory release, then add: static void cleanup(viod) { if (child_stack) free(child_stack); } -- Regards, Li Wang -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20190613/cb918bd1/attachment.html> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 10:16 ` Jan Stancek 2019-06-13 11:26 ` Li Wang @ 2019-06-13 14:17 ` Cyril Hrubis 2019-06-13 14:57 ` Jan Stancek 1 sibling, 1 reply; 17+ messages in thread From: Cyril Hrubis @ 2019-06-13 14:17 UTC (permalink / raw) To: ltp Hi! > > We need free(child_stack) in the cleanup function. > > Can you elaborate? If I remember correctly at some point we decided to clean up after tests properly so that we don't upset various debugging tools, i.e. coverity, valgrind, etc. and I think that you were part of that discussion. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 14:17 ` Cyril Hrubis @ 2019-06-13 14:57 ` Jan Stancek 2019-06-13 15:14 ` Cyril Hrubis 0 siblings, 1 reply; 17+ messages in thread From: Jan Stancek @ 2019-06-13 14:57 UTC (permalink / raw) To: ltp ----- Original Message ----- > Hi! > > > We need free(child_stack) in the cleanup function. > > > > Can you elaborate? > > If I remember correctly at some point we decided to clean up after tests > properly so that we don't upset various debugging tools, i.e. coverity, > valgrind, etc. and I think that you were part of that discussion. I recall I started with that position (free all), and I thought you turned me around after this many years :-). Do we have anything about this in style guide? I only found brief mention in "don't call cleanup from setup" section, which isn't even possible with newlib. ... You don't need to clean up the following: * +malloc(3)+'ed memory. * Read-only file descriptors in persistent paths (i.e. not temporary directories). > > -- > Cyril Hrubis > chrubis@suse.cz > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 14:57 ` Jan Stancek @ 2019-06-13 15:14 ` Cyril Hrubis 2019-06-16 7:34 ` Jan Stancek 0 siblings, 1 reply; 17+ messages in thread From: Cyril Hrubis @ 2019-06-13 15:14 UTC (permalink / raw) To: ltp Hi! > > If I remember correctly at some point we decided to clean up after tests > > properly so that we don't upset various debugging tools, i.e. coverity, > > valgrind, etc. and I think that you were part of that discussion. > > I recall I started with that position (free all), and I thought you > turned me around after this many years :-). Well I didn't care that much, but I guess that I lean slightly to free the memory :-). > Do we have anything about this in style guide? I only found brief mention > in "don't call cleanup from setup" section, which isn't even possible with newlib. I don't think so. I guess that we should write something down, once we decide what is the prefered option. > ... > You don't need to clean up the following: > > * +malloc(3)+'ed memory. > * Read-only file descriptors in persistent paths (i.e. not > temporary directories). Looks like this is terribly outdated, at least I tend to tell people to close all filedescriptors to make things simpler. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-13 15:14 ` Cyril Hrubis @ 2019-06-16 7:34 ` Jan Stancek 2019-06-17 8:50 ` Cyril Hrubis 0 siblings, 1 reply; 17+ messages in thread From: Jan Stancek @ 2019-06-16 7:34 UTC (permalink / raw) To: ltp ----- Original Message ----- > Hi! > > > If I remember correctly at some point we decided to clean up after tests > > > properly so that we don't upset various debugging tools, i.e. coverity, > > > valgrind, etc. and I think that you were part of that discussion. > > > > I recall I started with that position (free all), and I thought you > > turned me around after this many years :-). > > Well I didn't care that much, but I guess that I lean slightly to free > the memory :-). OK, so should I repost or is this good to go with free added to cleanup? > > > Do we have anything about this in style guide? I only found brief mention > > in "don't call cleanup from setup" section, which isn't even possible with > > newlib. > > I don't think so. I guess that we should write something down, once we > decide what is the prefered option. > > > ... > > You don't need to clean up the following: > > > > * +malloc(3)+'ed memory. > > * Read-only file descriptors in persistent paths (i.e. not > > temporary directories). > > Looks like this is terribly outdated, at least I tend to tell people to > close all filedescriptors to make things simpler. > > -- > Cyril Hrubis > chrubis@suse.cz > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-16 7:34 ` Jan Stancek @ 2019-06-17 8:50 ` Cyril Hrubis 2019-06-17 14:38 ` Jan Stancek 0 siblings, 1 reply; 17+ messages in thread From: Cyril Hrubis @ 2019-06-17 8:50 UTC (permalink / raw) To: ltp Hi! > > > I recall I started with that position (free all), and I thought you > > > turned me around after this many years :-). > > > > Well I didn't care that much, but I guess that I lean slightly to free > > the memory :-). > > OK, so should I repost or is this good to go with free added to cleanup? Let's do that. And I will write an RFC patch for the test-writing-guidelines. -- Cyril Hrubis chrubis@suse.cz ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-17 8:50 ` Cyril Hrubis @ 2019-06-17 14:38 ` Jan Stancek 2019-06-18 16:00 ` Amir Goldstein 0 siblings, 1 reply; 17+ messages in thread From: Jan Stancek @ 2019-06-17 14:38 UTC (permalink / raw) To: ltp ----- Original Message ----- > Hi! > > > > I recall I started with that position (free all), and I thought you > > > > turned me around after this many years :-). > > > > > > Well I didn't care that much, but I guess that I lean slightly to free > > > the memory :-). > > > > OK, so should I repost or is this good to go with free added to cleanup? > > Let's do that. Pushed. (Li, I added your Acked-by to it, since your comment has been addressed). Regards, Jan ^ permalink raw reply [flat|nested] 17+ messages in thread
* [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child 2019-06-17 14:38 ` Jan Stancek @ 2019-06-18 16:00 ` Amir Goldstein 0 siblings, 0 replies; 17+ messages in thread From: Amir Goldstein @ 2019-06-18 16:00 UTC (permalink / raw) To: ltp On Mon, Jun 17, 2019 at 5:38 PM Jan Stancek <jstancek@redhat.com> wrote: > > > ----- Original Message ----- > > Hi! > > > > > I recall I started with that position (free all), and I thought you > > > > > turned me around after this many years :-). > > > > > > > > Well I didn't care that much, but I guess that I lean slightly to free > > > > the memory :-). > > > > > > OK, so should I repost or is this good to go with free added to cleanup? > > > > Let's do that. > > Pushed. > (Li, I added your Acked-by to it, since your comment has been addressed). > FYI, not related to these fixes, but tests ioctl_ns0[46] have failures on kernel without CONFIG_USER_NS: tst_test.c:1112: INFO: Timeout per run is 0h 05m 00s ioctl_ns04.c:24: CONF: namespace not available safe_macros.c:225: BROK: ioctl_ns04.c:31: open(/proc/self/ns/user,0,01606327050) failed: ENOENT And so does test getxattr05: getxattr05.c:85: PASS: Got same data when acquiring the value of system.posix_acl_access twice getxattr05.c:95: FAIL: unshare(CLONE_NEWUSER) failed: EINVAL tst_test.c:362: BROK: Invalid child (29014) exit value 1 Thanks, Amir. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-06-18 16:00 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-13 7:23 [LTP] [PATCH v3 1/3] lib: add ltp_alloc_stack() Jan Stancek 2019-06-13 7:24 ` [LTP] [PATCH v3 2/3] lib: rename ltp_clone_malloc to ltp_clone_alloc Jan Stancek 2019-06-13 8:55 ` Li Wang 2019-06-13 13:57 ` Cyril Hrubis 2019-06-14 2:46 ` Li Wang 2019-06-14 15:24 ` Cyril Hrubis 2019-06-13 7:24 ` [LTP] [PATCH v3 3/3] syscalls/ioctl_ns0[156]: align stack and wait for child Jan Stancek 2019-06-13 8:25 ` Li Wang 2019-06-13 10:16 ` Jan Stancek 2019-06-13 11:26 ` Li Wang 2019-06-13 14:17 ` Cyril Hrubis 2019-06-13 14:57 ` Jan Stancek 2019-06-13 15:14 ` Cyril Hrubis 2019-06-16 7:34 ` Jan Stancek 2019-06-17 8:50 ` Cyril Hrubis 2019-06-17 14:38 ` Jan Stancek 2019-06-18 16:00 ` Amir Goldstein
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.