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

[xuyang2018.jy@fujitsu.com]

Hi Petr

> Hi 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.
> 
> Ah thx, good idea. I guess the point of the setup was to run copy only once, but
> your points are obviously valid.
> 
> I didn't notice it before because I overlooked SAFE_EXECLP() in test_nosuid() it
> had parameter TESTBIN, thus not being run from mountpoint.
> 
> nit: I suggest to move to SAFE_EXECL() as it expect path, not filename as it's
> not using PATH. Similarly we could change execlp() to execl() in test_noexec(),
> but I'd prefer to keep execlp(), so that we test two different libc wrappers.

Sound reasonable.
> 
>> 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
> +1 for avoid the need of snprintf when there are 2 constants.
> NOTE: we can separate 3 items with spaces:
> #define BIN_PATH	MNTPOINT "/" TESTBIN
> But I'd rename it to TESTBIN_PATH.
> Or maybe even better to use just "TEST":
> #define TEST        "mount03_setuid_test"
> #define TEST_PATH	MNTPOINT "/" TEST

Looks well.
> 
>>    #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);
> this is not needed when we have BIN_PATH
Yes.
>> +       SAFE_CP(TESTBIN, file);
> SAFE_CP(TESTBIN, BIN_PATH);
>> +       SAFE_STAT(file, &st);
>> +       if (st.st_mode != SUID_MODE)
>> +               SAFE_CHMOD(file, SUID_MODE);
> 	SAFE_CHMOD(BIN_PATH, 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)
> 
> Final diff is below, but for readability it's temporarily also on my fork:
> https://github.com/pevik/ltp/blob/57ba1ba47987a201c39764b4259a15aa39db9d2e/testcases/kernel/syscalls/mount/mount03.c

OK.
> 
> Kind regards,
> Petr
> 
>> Best Regards
>> Yang Xu
> 
> diff --git testcases/kernel/syscalls/mount/mount03.c testcases/kernel/syscalls/mount/mount03.c
> index 9c58783d7..eef2a65c6 100644
> --- testcases/kernel/syscalls/mount/mount03.c
> +++ testcases/kernel/syscalls/mount/mount03.c
> @@ -18,8 +18,9 @@
>   #include "tst_test.h"
>   #include "lapi/mount.h"
>   
> -#define MNTPOINT        "mntpoint"
> +#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

Here SUID_MODE miss S_ISUID, that is why this case also pass if we 
excute a program on filesystem that not mounted with nosuid option.

Also, it seems make check code has problem when detecting S_ISUID

Symbolic permissions 'S_IRUSR|S_IXUSR|S_IXGRP|S_IXOTH' are not 
preferred. Consider using octal permissions '0511'.

Since we have converted mount03 into new api, mount03_setuid_test.c 
should also be converted into new spdx.

Best Regards
Yang Xu

> @@ -74,12 +75,18 @@ static void test_nosuid(void)
>   {
>   	pid_t pid;
>   	int status;
> +	struct stat st;
> +
> +	SAFE_CP(TESTBIN, BIN_PATH);
> +	SAFE_STAT(BIN_PATH, &st);
> +	if (st.st_mode != SUID_MODE)
> +		SAFE_CHMOD(BIN_PATH, SUID_MODE);
>   
>   	pid = SAFE_FORK();
>   	if (!pid) {
>   		SAFE_SETGID(nobody_gid);
>   		SAFE_SETREUID(-1, nobody_uid);
> -		SAFE_EXECLP(TESTBIN, TESTBIN, NULL);
> +		SAFE_EXECL(BIN_PATH, TESTBIN, NULL);
>   	}
>   
>   	SAFE_WAITPID(pid, &status, 0);
> @@ -137,18 +144,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);
> -	SAFE_CP(TESTBIN, file);
> -
> -	SAFE_STAT(file, &st);
> -	if (st.st_mode != SUID_MODE)
> -	    SAFE_CHMOD(file, SUID_MODE);
>   }
>   
>   static void cleanup(void)