* [LTP] [PATCH v4] adjtimex02.c: Skipped EFAULT tests for libc variant"
2021-06-10 5:09 ` [LTP] [PATCH v4] adjtimex02.c: Skipped EFAULT tests for libc variant" Vinay Kumar
@ 2021-06-10 5:12 ` Vinay Kumar
2021-06-11 11:46 ` Cyril Hrubis
1 sibling, 0 replies; 5+ messages in thread
From: Vinay Kumar @ 2021-06-10 5:12 UTC (permalink / raw)
To: ltp
Hi Metan,
Patch is updated with commit log. Added the reason for EFAULT skipped
for libc variant as below,
"Tests for bad addresses in LTP cases trigger segment fault in libc on
a 32bit system."
Regards,
Vinay
On Thu, Jun 10, 2021 at 10:40 AM Vinay Kumar <vinay.m.engg@gmail.com> wrote:
>
> From: vinay kumar <vinay.m.engg@gmail.com>
>
> Tested EFAULT cases only for "__NR_adjtimex" syscall.
>
> Tests for bad addresses in LTP cases trigger segment
> fault in libc on a 32bit system.
>
> Signed-off-by: Vinay Kumar <vinay.m.engg@gmail.com>
> ---
> .../kernel/syscalls/adjtimex/adjtimex02.c | 226 ++++++++++++------
> 1 file changed, 152 insertions(+), 74 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/adjtimex/adjtimex02.c b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> index 19ee97158..5eaea6352 100644
> --- a/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> +++ b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> @@ -10,115 +10,193 @@
> #include <unistd.h>
> #include <pwd.h>
> #include "tst_test.h"
> +#include "lapi/syscalls.h"
>
> -#define SET_MODE ( ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> - ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK )
> +#define SET_MODE (ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> + ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK)
>
> -static int hz; /* HZ from sysconf */
> -
> -static struct timex *tim_save;
> -static struct timex *buf;
> +static int hz; /* HZ from sysconf */
>
> +static struct timex *tim_save, *buf;
> static struct passwd *ltpuser;
>
> -static void verify_adjtimex(unsigned int nr)
> +struct test_case {
> + unsigned int modes;
> + long lowlimit;
> + long highlimit;
> + long delta;
> + int exp_err;
> +};
> +
> +static int libc_adjtimex(void *timex)
> {
> - struct timex *bufp;
> - int expected_errno = 0;
> + return adjtimex(timex);
> +}
>
> - /*
> - * since Linux 2.6.26, if buf.offset value is outside
> - * the acceptable range, it is simply normalized instead
> - * of letting the syscall fail. so just skip this test
> - * case.
> - */
> - if (nr > 3 && (tst_kvercmp(2, 6, 25) > 0)) {
> - tst_res(TCONF, "this kernel normalizes buf."
> - "offset value if it is outside"
> - " the acceptable range.");
> - return;
> - }
> +static int sys_adjtimex(void *timex)
> +{
> + return tst_syscall(__NR_adjtimex, timex);
> +}
> +
> +static struct test_case tc[] = {
> + {
> + .modes = SET_MODE,
> + .exp_err = EFAULT,
> + },
> + {
> + .modes = ADJ_TICK,
> + .lowlimit = 900000,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_TICK,
> + .highlimit = 1100000,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_OFFSET,
> + .highlimit = 512000L,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_OFFSET,
> + .lowlimit = -512000L,
> + .delta = -1,
> + .exp_err = EINVAL,
> + },
> +};
> +
> +static struct test_variants
> +{
> + int (*adjtimex)(void *timex);
> + char *desc;
> +} variants[] = {
> + { .adjtimex = libc_adjtimex, .desc = "libc adjtimex()"},
> +
> +#if (__NR_adjtimex != __LTP__NR_INVALID_SYSCALL)
> + { .adjtimex = sys_adjtimex, .desc = "__NR_adjtimex syscall"},
> +#endif
> +};
> +
> +static void verify_adjtimex(unsigned int i)
> +{
> + struct timex *bufp;
> + struct test_variants *tv = &variants[tst_variant];
>
> *buf = *tim_save;
> buf->modes = SET_MODE;
> bufp = buf;
> - switch (nr) {
> - case 0:
> - bufp = (struct timex *)-1;
> - expected_errno = EFAULT;
> - break;
> - case 1:
> - buf->tick = 900000 / hz - 1;
> - expected_errno = EINVAL;
> - break;
> - case 2:
> - buf->tick = 1100000 / hz + 1;
> - expected_errno = EINVAL;
> - break;
> - case 3:
> - /* Switch to nobody user for correct error code collection */
> - ltpuser = SAFE_GETPWNAM("nobody");
> - SAFE_SETEUID(ltpuser->pw_uid);
> - expected_errno = EPERM;
> - break;
> - case 4:
> - buf->offset = 512000L + 1;
> - expected_errno = EINVAL;
> - break;
> - case 5:
> - buf->offset = (-1) * (512000L) - 1;
> - expected_errno = EINVAL;
> - break;
> - default:
> - tst_brk(TFAIL, "Invalid test case %u ", nr);
> +
> + if (tc[i].exp_err == EINVAL) {
> + if (tc[i].modes == ADJ_TICK) {
> + if (tc[i].lowlimit)
> + buf->tick = tc[i].lowlimit - tc[i].delta;
> +
> + if (tc[i].highlimit)
> + buf->tick = tc[i].highlimit + tc[i].delta;
> + }
> + if (tc[i].modes == ADJ_OFFSET) {
> + if (tc[i].lowlimit) {
> + tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> + return;
> + }
> + if (tc[i].highlimit) {
> + tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> + return;
> + }
> + }
> }
>
> - TEST(adjtimex(bufp));
> - if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> - tst_res(TPASS | TTERRNO,
> - "adjtimex() error %u ", expected_errno);
> - } else {
> - tst_res(TFAIL | TTERRNO,
> - "Test Failed, adjtimex() returned %ld",
> - TST_RET);
> + if (tc[i].exp_err == EFAULT) {
> + if (tv->adjtimex != libc_adjtimex) {
> + bufp = (struct timex *) -1;
> + } else {
> + tst_res(TCONF, "EFAULT is skipped for libc variant");
> + return;
> + }
> + }
> +
> + TEST(tv->adjtimex(bufp));
> +
> + if (tc[i].exp_err != TST_ERR) {
> + tst_res(TFAIL | TTERRNO, "adjtimex(): expected %s mode %x",
> + tst_strerrno(tc[i].exp_err), tc[i].modes);
> + return;
> }
>
> - /* clean up after ourselves */
> - if (nr == 3)
> - SAFE_SETEUID(0);
> + tst_res(TPASS, "adjtimex() error %s", tst_strerrno(tc[i].exp_err));
> +
> }
>
> static void setup(void)
> {
> + struct test_variants *tv = &variants[tst_variant];
> + size_t i;
> + int expected_errno = 0;
> +
> + tst_res(TINFO, "Testing variant: %s", tv->desc);
> +
> tim_save->modes = 0;
>
> + buf->modes = SET_MODE;
> +
> + /* Switch to nobody user for correct error code collection */
> + ltpuser = SAFE_GETPWNAM("nobody");
> + SAFE_SETEUID(ltpuser->pw_uid);
> + expected_errno = EPERM;
> +
> + TEST(tv->adjtimex(buf));
> +
> + if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> + tst_res(TPASS, "adjtimex() error %s ",
> + tst_strerrno(expected_errno));
> + } else {
> + tst_res(TFAIL | TTERRNO,
> + "adjtimex(): returned %ld", TST_RET);
> + }
> +
> + SAFE_SETEUID(0);
> +
> /* set the HZ from sysconf */
> hz = SAFE_SYSCONF(_SC_CLK_TCK);
>
> - /* Save current parameters */
> - if ((adjtimex(tim_save)) == -1)
> + for (i = 0; i < ARRAY_SIZE(tc); i++) {
> + if (tc[i].modes == ADJ_TICK) {
> + tc[i].highlimit /= hz;
> + tc[i].lowlimit /= hz;
> + }
> + }
> +
> + if ((adjtimex(tim_save)) == -1) {
> tst_brk(TBROK | TERRNO,
> - "adjtimex(): failed to save current params");
> + "adjtimex(): failed to save current params");
> + }
> }
>
> static void cleanup(void)
> {
> +
> tim_save->modes = SET_MODE;
>
> - /* Restore saved parameters */
> - if ((adjtimex(tim_save)) == -1)
> - tst_res(TWARN, "Failed to restore saved parameters");
> + if ((adjtimex(tim_save)) == -1) {
> + tst_brk(TBROK | TERRNO,
> + "adjtimex(): failed to save current params");
> + }
> }
>
> static struct tst_test test = {
> - .needs_root = 1,
> - .tcnt = 6,
> + .test = verify_adjtimex,
> .setup = setup,
> .cleanup = cleanup,
> - .test = verify_adjtimex,
> + .tcnt = ARRAY_SIZE(tc),
> + .test_variants = ARRAY_SIZE(variants),
> + .needs_root = 1,
> .bufs = (struct tst_buffers []) {
> - {&buf, .size = sizeof(*buf)},
> - {&tim_save, .size = sizeof(*tim_save)},
> - {},
> - }
> + {&buf, .size = sizeof(*buf)},
> + {&tim_save, .size = sizeof(*tim_save)},
> + {},
> + }
> };
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [LTP] [PATCH v4] adjtimex02.c: Skipped EFAULT tests for libc variant"
2021-06-10 5:09 ` [LTP] [PATCH v4] adjtimex02.c: Skipped EFAULT tests for libc variant" Vinay Kumar
2021-06-10 5:12 ` Vinay Kumar
@ 2021-06-11 11:46 ` Cyril Hrubis
2021-06-13 16:19 ` [LTP] [PATCH v5] adjtimex02.c: Skipped EFAULT tests for libc variant Vinay Kumar
1 sibling, 1 reply; 5+ messages in thread
From: Cyril Hrubis @ 2021-06-11 11:46 UTC (permalink / raw)
To: ltp
Hi!
> Tested EFAULT cases only for "__NR_adjtimex" syscall.
>
> Tests for bad addresses in LTP cases trigger segment
> fault in libc on a 32bit system.
>
> Signed-off-by: Vinay Kumar <vinay.m.engg@gmail.com>
> ---
> .../kernel/syscalls/adjtimex/adjtimex02.c | 226 ++++++++++++------
> 1 file changed, 152 insertions(+), 74 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/adjtimex/adjtimex02.c b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> index 19ee97158..5eaea6352 100644
> --- a/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> +++ b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> @@ -10,115 +10,193 @@
> #include <unistd.h>
> #include <pwd.h>
> #include "tst_test.h"
> +#include "lapi/syscalls.h"
>
> -#define SET_MODE ( ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> - ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK )
> +#define SET_MODE (ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> + ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK)
>
> -static int hz; /* HZ from sysconf */
> -
> -static struct timex *tim_save;
> -static struct timex *buf;
> +static int hz; /* HZ from sysconf */
>
> +static struct timex *tim_save, *buf;
> static struct passwd *ltpuser;
>
> -static void verify_adjtimex(unsigned int nr)
> +struct test_case {
> + unsigned int modes;
> + long lowlimit;
> + long highlimit;
> + long delta;
> + int exp_err;
> +};
> +
> +static int libc_adjtimex(void *timex)
> {
> - struct timex *bufp;
> - int expected_errno = 0;
> + return adjtimex(timex);
> +}
Do we need this indirection?
As long as we fix the prototype in the test_variants to have a proper
type for the function, i.e. the timex pointer is struct timex * instead
of void * we can store the libc function pointer directly to the variant
structure.
> - /*
> - * since Linux 2.6.26, if buf.offset value is outside
> - * the acceptable range, it is simply normalized instead
> - * of letting the syscall fail. so just skip this test
> - * case.
> - */
> - if (nr > 3 && (tst_kvercmp(2, 6, 25) > 0)) {
> - tst_res(TCONF, "this kernel normalizes buf."
> - "offset value if it is outside"
> - " the acceptable range.");
> - return;
> - }
> +static int sys_adjtimex(void *timex)
^
This should be struct timex *
> +{
> + return tst_syscall(__NR_adjtimex, timex);
> +}
> +
> +static struct test_case tc[] = {
> + {
> + .modes = SET_MODE,
> + .exp_err = EFAULT,
> + },
> + {
> + .modes = ADJ_TICK,
> + .lowlimit = 900000,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_TICK,
> + .highlimit = 1100000,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_OFFSET,
> + .highlimit = 512000L,
> + .delta = 1,
> + .exp_err = EINVAL,
> + },
> + {
> + .modes = ADJ_OFFSET,
> + .lowlimit = -512000L,
> + .delta = -1,
> + .exp_err = EINVAL,
> + },
> +};
> +
> +static struct test_variants
> +{
> + int (*adjtimex)(void *timex);
^
This should be struct timex * as well.
> + char *desc;
> +} variants[] = {
> + { .adjtimex = libc_adjtimex, .desc = "libc adjtimex()"},
> +
> +#if (__NR_adjtimex != __LTP__NR_INVALID_SYSCALL)
> + { .adjtimex = sys_adjtimex, .desc = "__NR_adjtimex syscall"},
> +#endif
> +};
> +
> +static void verify_adjtimex(unsigned int i)
> +{
> + struct timex *bufp;
> + struct test_variants *tv = &variants[tst_variant];
>
> *buf = *tim_save;
> buf->modes = SET_MODE;
> bufp = buf;
> - switch (nr) {
> - case 0:
> - bufp = (struct timex *)-1;
> - expected_errno = EFAULT;
> - break;
> - case 1:
> - buf->tick = 900000 / hz - 1;
> - expected_errno = EINVAL;
> - break;
> - case 2:
> - buf->tick = 1100000 / hz + 1;
> - expected_errno = EINVAL;
> - break;
> - case 3:
> - /* Switch to nobody user for correct error code collection */
> - ltpuser = SAFE_GETPWNAM("nobody");
> - SAFE_SETEUID(ltpuser->pw_uid);
> - expected_errno = EPERM;
> - break;
> - case 4:
> - buf->offset = 512000L + 1;
> - expected_errno = EINVAL;
> - break;
> - case 5:
> - buf->offset = (-1) * (512000L) - 1;
> - expected_errno = EINVAL;
> - break;
> - default:
> - tst_brk(TFAIL, "Invalid test case %u ", nr);
> +
> + if (tc[i].exp_err == EINVAL) {
> + if (tc[i].modes == ADJ_TICK) {
> + if (tc[i].lowlimit)
> + buf->tick = tc[i].lowlimit - tc[i].delta;
> +
> + if (tc[i].highlimit)
> + buf->tick = tc[i].highlimit + tc[i].delta;
> + }
> + if (tc[i].modes == ADJ_OFFSET) {
> + if (tc[i].lowlimit) {
> + tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> + return;
> + }
> + if (tc[i].highlimit) {
> + tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> + return;
> + }
> + }
> }
>
> - TEST(adjtimex(bufp));
> - if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> - tst_res(TPASS | TTERRNO,
> - "adjtimex() error %u ", expected_errno);
> - } else {
> - tst_res(TFAIL | TTERRNO,
> - "Test Failed, adjtimex() returned %ld",
> - TST_RET);
> + if (tc[i].exp_err == EFAULT) {
> + if (tv->adjtimex != libc_adjtimex) {
> + bufp = (struct timex *) -1;
> + } else {
> + tst_res(TCONF, "EFAULT is skipped for libc variant");
> + return;
> + }
> + }
> +
> + TEST(tv->adjtimex(bufp));
> +
> + if (tc[i].exp_err != TST_ERR) {
> + tst_res(TFAIL | TTERRNO, "adjtimex(): expected %s mode %x",
> + tst_strerrno(tc[i].exp_err), tc[i].modes);
> + return;
> }
We should really use TST_EXP_FAIL() here instead.
> - /* clean up after ourselves */
> - if (nr == 3)
> - SAFE_SETEUID(0);
> + tst_res(TPASS, "adjtimex() error %s", tst_strerrno(tc[i].exp_err));
> +
> }
>
> static void setup(void)
> {
> + struct test_variants *tv = &variants[tst_variant];
> + size_t i;
> + int expected_errno = 0;
> +
> + tst_res(TINFO, "Testing variant: %s", tv->desc);
> +
> tim_save->modes = 0;
>
> + buf->modes = SET_MODE;
> +
> + /* Switch to nobody user for correct error code collection */
> + ltpuser = SAFE_GETPWNAM("nobody");
> + SAFE_SETEUID(ltpuser->pw_uid);
> + expected_errno = EPERM;
> +
> + TEST(tv->adjtimex(buf));
> +
> + if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> + tst_res(TPASS, "adjtimex() error %s ",
> + tst_strerrno(expected_errno));
> + } else {
> + tst_res(TFAIL | TTERRNO,
> + "adjtimex(): returned %ld", TST_RET);
> + }
> +
> + SAFE_SETEUID(0);
What is this nonsense?
What is doing a test code in the test setup?
The only thing that should be done in the setup is to store the nobody
uid into a global variable.
The EPERM test should be done in the verify_adjtimex() function, we can
do something as:
if (tc[i].exp_err == EPERM)
SAFE_SETEUID(nobody_uid);
/* do the actuall test */
if (tc[i].exp_err == EPERM)
SAFE_SETEUID(0);
> /* set the HZ from sysconf */
> hz = SAFE_SYSCONF(_SC_CLK_TCK);
>
> - /* Save current parameters */
> - if ((adjtimex(tim_save)) == -1)
> + for (i = 0; i < ARRAY_SIZE(tc); i++) {
> + if (tc[i].modes == ADJ_TICK) {
> + tc[i].highlimit /= hz;
> + tc[i].lowlimit /= hz;
> + }
> + }
> +
> + if ((adjtimex(tim_save)) == -1) {
> tst_brk(TBROK | TERRNO,
> - "adjtimex(): failed to save current params");
> + "adjtimex(): failed to save current params");
> + }
> }
>
> static void cleanup(void)
> {
> +
> tim_save->modes = SET_MODE;
>
> - /* Restore saved parameters */
> - if ((adjtimex(tim_save)) == -1)
> - tst_res(TWARN, "Failed to restore saved parameters");
> + if ((adjtimex(tim_save)) == -1) {
> + tst_brk(TBROK | TERRNO,
> + "adjtimex(): failed to save current params");
^
This was correct before the change, we
change the modes to SET_MODE so we
really restore the value here.
> + }
> }
>
> static struct tst_test test = {
> - .needs_root = 1,
> - .tcnt = 6,
> + .test = verify_adjtimex,
> .setup = setup,
> .cleanup = cleanup,
> - .test = verify_adjtimex,
> + .tcnt = ARRAY_SIZE(tc),
> + .test_variants = ARRAY_SIZE(variants),
> + .needs_root = 1,
> .bufs = (struct tst_buffers []) {
> - {&buf, .size = sizeof(*buf)},
> - {&tim_save, .size = sizeof(*tim_save)},
> - {},
> - }
> + {&buf, .size = sizeof(*buf)},
> + {&tim_save, .size = sizeof(*tim_save)},
> + {},
> + }
> };
> --
> 2.17.1
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 5+ messages in thread