Re: [LTP] [PATCH v3 0/2] mount03: Convert to new API

[Petr Vorel <pvorel@suse.cz>]

> Hi Petr

> > Hi Xu,

> > ...
> >>>>> -	SAFE_READ(0, otfd, readbuf, sizeof(readbuf));
> >>>>> +	nobody_uid = ltpuser->pw_uid;
> >>>>> +	nobody_gid = ltpuser->pw_gid;

> >>>>> -	SAFE_FSTAT(otfd, &file_stat);
> >>>>> +	snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> >>>>> +	TST_RESOURCE_COPY(NULL, TESTBIN, file);

> >>>> In fact, old test case copy resource file when mount fileystem, but now,
> >>>> you change this.  So in test_nosuid function, you test nosuid behaviour
> >>>> in tmpdir instead of different filesystems.

> >>> old code in setup:
> >>>       fs_type = tst_dev_fs_type();
> >>>       device = tst_acquire_device(cleanup);

> >>>       if (!device)
> >>>           tst_brkm(TCONF, cleanup, "Failed to obtain block device");

> >>>       tst_mkfs(cleanup, device, fs_type, NULL, NULL);

> >>>       SAFE_MKDIR(cleanup, mntpoint, DIR_MODE);

> >>>       SAFE_MOUNT(cleanup, device, mntpoint, fs_type, 0, NULL);
> >>>       TST_RESOURCE_COPY(cleanup, "mount03_setuid_test", path_name);

> >>> new code:
> >>>       snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> >>>       SAFE_CP(TESTBIN, file);

> >>> Well, Li in his v2 removed the code because there is .mntpoint = MNTPOINT, in
> >>> struct tst_test, therefore MNTPOINT is mounted in the filesystem, right?

> >>> But he also did SAFE_STAT and SAFE_CHMOD on MNTPOINT, which is IMHO wrong
> >>> (or at least different from the old code).

> >> Yes, it is wrong. I guess Chen misundertand mntpoint usage(it just
> >> create mntpoint instead mount dev to a moutpoint).

> >> So do you will fix this？

> > Yes, see the diff below. I'm waiting little longer if anybody else has some
> > comments before merging it.

> > Thanks for your review.

> >> Best Regards
> >> Yang Xu

> > diff --git testcases/kernel/syscalls/mount/mount03.c testcases/kernel/syscalls/mount/mount03.c
> > index 74b018d78..9c58783d7 100644
> > --- testcases/kernel/syscalls/mount/mount03.c
> > +++ testcases/kernel/syscalls/mount/mount03.c
> > @@ -15,7 +15,6 @@
> >   #include <sys/types.h>
> >   #include <sys/wait.h>
> >   #include <pwd.h>
> > -#include "old_resource.h"
> >   #include "tst_test.h"
> >   #include "lapi/mount.h"

> > @@ -145,7 +144,7 @@ static void setup(void)
> >   	nobody_gid = ltpuser->pw_gid;

> >   	snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> > -	TST_RESOURCE_COPY(NULL, TESTBIN, file);
> > +	SAFE_CP(TESTBIN, file);

> I still think we should test nosuid behaviour on different filesystem 
> like other test function because we have expand it to all filesystems.

> Also include tmpfs, so SAFE_CP should be in test_nosuid function 
> otherwise may hit ENOENT problem.

Actually, I randomly hit ENOENT, when SAFE_CP *is* in test_nosuid().
Not sure what happen (filesystem not synced? sync() does not help).
It works when kept in the setup.

Kind regards,
Petr

> different code as below:

> [root@localhost mount]# git diff .
> diff --git a/testcases/kernel/syscalls/mount/mount03.c 
> b/testcases/kernel/syscalls/mount/mount03.c
> index 74b018d78..b0582c76b 100644
> --- a/testcases/kernel/syscalls/mount/mount03.c
> +++ b/testcases/kernel/syscalls/mount/mount03.c
> @@ -21,6 +21,7 @@

>   #define MNTPOINT        "mntpoint"
>   #define TESTBIN        "mount03_setuid_test"
> +#define BIN_PATH           MNTPOINT"/"TESTBIN
>   #define TEST_STR "abcdefghijklmnopqrstuvwxyz"
>   #define FILE_MODE      0644
>   #define SUID_MODE      0511
> @@ -75,12 +76,19 @@ static void test_nosuid(void)
>   {
>          pid_t pid;
>          int status;
> +       struct stat st;
> +
> +       snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> +       SAFE_CP(TESTBIN, file);
> +       SAFE_STAT(file, &st);
> +       if (st.st_mode != SUID_MODE)
> +               SAFE_CHMOD(file, SUID_MODE);

>          pid = SAFE_FORK();
>          if (!pid) {
>                  SAFE_SETGID(nobody_gid);
>                  SAFE_SETREUID(-1, nobody_uid);
> -               SAFE_EXECLP(TESTBIN, TESTBIN, NULL);
> +               SAFE_EXECLP(BIN_PATH, TESTBIN, NULL);
>          }

>          SAFE_WAITPID(pid, &status, 0);
> @@ -138,18 +146,10 @@ static struct tcase {

>   static void setup(void)
>   {
> -       struct stat st;
>          struct passwd *ltpuser = SAFE_GETPWNAM("nobody");

>          nobody_uid = ltpuser->pw_uid;
>          nobody_gid = ltpuser->pw_gid;
> -
> -       snprintf(file, PATH_MAX, "%s/%s", MNTPOINT, TESTBIN);
> -       TST_RESOURCE_COPY(NULL, TESTBIN, file);
> -
> -       SAFE_STAT(file, &st);
> -       if (st.st_mode != SUID_MODE)
> -           SAFE_CHMOD(file, SUID_MODE);
>   }

>   static void cleanup(void)
> [root@localhost mount]#


> Best Regards
> Yang Xu


> >   	SAFE_STAT(file, &st);
> >   	if (st.st_mode != SUID_MODE)

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp