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.
Besides it's really wrong to have it in the setup (mount is dole later in
run()), it also does not work (again, randomly ENOENT).

Kind regards,
Petr

> 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