| Message ID | 1b6ddab9cd8e3620da9e37b1132e911280c22e32.1560410182.git.jstancek@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/3] lib: add ltp_alloc_stack() | expand |
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.
----- 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 >
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); }
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.
----- 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 >
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.
----- 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 >
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.
----- 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
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.
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;
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(-)