Re: [LTP] [PATCH v2 3/8] Rewrite userns03.c using new LTP API

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

generally LGTM, few notes below.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

>  testcases/kernel/containers/userns/userns03.c | 266 ++++++++----------
> +/*\
> + * [Description]
> + *
> + * Verify that /proc/PID/uid_map and /proc/PID/gid_map contains three values
> + * separated by white space:
>   * ID-inside-ns   ID-outside-ns   length
>   *
>   * ID-outside-ns is interpreted according to which process is opening the file.
> @@ -26,29 +21,23 @@
>   * The string "deny" would be written to /proc/self/setgroups before GID
>   * check if setgroups is allowed, see kernel commits:
>   *
> - *   commit 9cc46516ddf497ea16e8d7cb986ae03a0f6b92f8
> - *   Author: Eric W. Biederman <ebiederm@xmission.com>
> - *   Date:   Tue Dec 2 12:27:26 2014 -0600
> - *     userns: Add a knob to disable setgroups on a per user namespace basis
> - *
> - *   commit 66d2f338ee4c449396b6f99f5e75cd18eb6df272
> - *   Author: Eric W. Biederman <ebiederm@xmission.com>
> - *   Date:   Fri Dec 5 19:36:04 2014 -0600
> - *     userns: Allow setting gid_maps without privilege when setgroups is disabled

> + * commit 9cc46516ddf497ea16e8d7cb986ae03a0f6b92f8
> + * Author: Eric W. Biederman <ebiederm@xmission.com>
> + * Date:   Tue Dec 2 12:27:26 2014 -0600
> + * userns: Add a knob to disable setgroups on a per user namespace basis
>   *
> + * commit 66d2f338ee4c449396b6f99f5e75cd18eb6df272
> + * Author: Eric W. Biederman <ebiederm@xmission.com>
> + * Date:   Fri Dec 5 19:36:04 2014 -0600
> + * userns: Allow setting gid_maps without privilege when setgroups is disabled
Commits like these two will be very badly formatted in html/pdf output.
I'd also add some blank lines so that text will be split in paragraphs.
Thus I suggest:

/*\
 * [Description]
 *
 * Verify that /proc/PID/uid_map and /proc/PID/gid_map contains three values
 * separated by white space:
 *
 * ID-inside-ns   ID-outside-ns   length
 *
 * ID-outside-ns is interpreted according to which process is opening the file.
 *
 * If the process opening the file is in the same user namespace as the process
 * PID, then ID-outside-ns is defined with respect to the parent user namespace.
 *
 * If the process opening the file is in a different user namespace, then
 * ID-outside-ns is defined with respect to the user namespace of the process
 * opening the file.
 *
 * The string "deny" would be written to /proc/self/setgroups before GID
 * check if setgroups is allowed, see kernel commits:
 *
 * * 9cc46516ddf4 ("userns: Add a knob to disable setgroups on a per user namespace basis")
 * * 66d2f338ee4c ("userns: Allow setting gid_maps without privilege when setgroups is disabled")
 */


> @@ -75,161 +64,148 @@ static int child_fn1(void)
...
>  	/* map file format:ID-inside-ns   ID-outside-ns   length
> -	If the process opening the file is in the same user namespace as
> -	the process PID, then ID-outside-ns is defined with respect to the
> -	 parent user namespace.*/
> +	 * If the process opening the file is in the same user namespace as
> +	 * the process PID, then ID-outside-ns is defined with respect to the
> +	 * parent user namespace
> +	 */
>  	if (idinsidens != CHILD2UID || idoutsidens != parentuid) {
> -		printf("child_fn2 checks /proc/cpid2/uid_map:\n");
> -		printf("unexpected: idinsidens=%d idoutsidens=%d\n",
> -			idinsidens, idoutsidens);
> -		exit_val = 1;
> +		tst_res(TINFO, "child2 checks /proc/cpid2/uid_map");
nit: I'd put this TINFO before if, so that it's printed also for TPASS.
Also I'd wrote CPID2 to be obvious it's supposed to be a number, or even put
pid value.

> +		tst_res(TFAIL, "unexpected: namespace ID inside=%d outside=%d", idinsidens, idoutsidens);
> +	} else {
> +		tst_res(TPASS, "expected namespaces IDs");
>  	}

>  	sprintf(cpid1uidpath, "/proc/%d/uid_map", cpid1);
> -	SAFE_FILE_SCANF(NULL, cpid1uidpath, "%d %d %d", &idinsidens,
> -		&idoutsidens, &length);
> +	SAFE_FILE_SCANF(cpid1uidpath, "%d %d %d", &idinsidens, &idoutsidens, &length);

>  	/* If the process opening the file is in a different user namespace,
> -	then ID-outside-ns is defined with respect to the user namespace
> -	of the process opening the file.*/
> +	 * then ID-outside-ns is defined with respect to the user namespace
> +	 * of the process opening the file
> +	 */
>  	if (idinsidens != CHILD1UID || idoutsidens != CHILD2UID) {
> -		printf("child_fn2 checks /proc/cpid1/uid_map:\n");
> -		printf("unexpected: idinsidens=%d idoutsidens=%d\n",
> -			idinsidens, idoutsidens);
> -		exit_val = 1;
> +		tst_res(TINFO, "child2 checks /proc/cpid1/uid_map");
And here as well.

> +		tst_res(TFAIL, "unexpected: namespace ID inside=%d outside=%d", idinsidens, idoutsidens);
> +	} else {
> +		tst_res(TPASS, "expected namespaces IDs");
>  	}

>  	sprintf(cpid1gidpath, "/proc/%d/gid_map", cpid1);
> -	SAFE_FILE_SCANF(NULL, "/proc/self/gid_map", "%d %d %d",
> -		 &idinsidens, &idoutsidens, &length);
> +	SAFE_FILE_SCANF("/proc/self/gid_map", "%d %d %d", &idinsidens, &idoutsidens, &length);

>  	if (idinsidens != CHILD2GID || idoutsidens != parentgid) {
> -		printf("child_fn2 checks /proc/cpid2/gid_map:\n");
> -		printf("unexpected: idinsidens=%d idoutsidens=%d\n",
> -			idinsidens, idoutsidens);
> -		exit_val = 1;
> +		tst_res(TINFO, "child2 checks /proc/cpid2/gid_map");
And here.
> +		tst_res(TFAIL, "unexpected: namespace ID inside=%d outside=%d", idinsidens, idoutsidens);
> +	} else {
> +		tst_res(TPASS, "expected namespaces IDs");
>  	}

> -	SAFE_FILE_SCANF(NULL, cpid1gidpath, "%d %d %d", &idinsidens,
> -		&idoutsidens, &length);
> +	SAFE_FILE_SCANF(cpid1gidpath, "%d %d %d", &idinsidens, &idoutsidens, &length);

>  	if (idinsidens != CHILD1GID || idoutsidens != CHILD2GID) {
> -		printf("child_fn1 checks /proc/cpid1/gid_map:\n");
> -		printf("unexpected: idinsidens=%d idoutsidens=%d\n",
> -			idinsidens, idoutsidens);
> -		exit_val = 1;
> +		tst_res(TINFO, "child1 checks /proc/cpid1/gid_map");
And here.
> +		tst_res(TFAIL, "unexpected: namespace ID inside=%d outside=%d", idinsidens, idoutsidens);
> +	} else {
> +		tst_res(TPASS, "expected namespaces IDs");
>  	}

> -int main(int argc, char *argv[])
> +static void run(void)
>  {
> +	parentuid = geteuid();
> +	parentgid = getegid();
> +
> +	cpid1 = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, (void *)child_fn1, NULL);
> +	if (cpid1 < 0)
> +		tst_brk(TBROK | TTERRNO, "cpid1 clone failed");
> +
> +	cpid2 = ltp_clone_quick(CLONE_NEWUSER | SIGCHLD, (void *)child_fn2, NULL);
> +	if (cpid2 < 0)
> +		tst_brk(TBROK | TTERRNO, "cpid2 clone failed");
> +
> +	if (access("/proc/self/setgroups", F_OK) == 0) {
> +		sprintf(path, "/proc/%d/setgroups", cpid1);
> +
> +		fd = SAFE_OPEN(path, O_WRONLY, 0644);
> +		SAFE_WRITE(1, fd, "deny", 4);
> +		SAFE_CLOSE(fd);
> +
> +		/* If the setgroups file has the value "deny",
> +		 * then the setgroups(2) system call can't
> +		 * subsequently be reenabled (by writing "allow" to
> +		 * the file) in this user namespace.  (Attempts to
> +		 * do so will fail with the error EPERM.)
> +		 */
> +
> +		/* test that setgroups can't be re-enabled */
> +		fd = SAFE_OPEN(path, O_WRONLY, 0644);
> +		ret = write(fd, "allow", 5);
> +
> +		if (ret >= 0)
nit: Maybe keep ret != -1 (as was in original?)
> +			tst_brk(TBROK, "write action should fail");
> +		else if (errno != EPERM)
> +			tst_brk(TBROK | TTERRNO, "unexpected error");
> +
> +		SAFE_CLOSE(fd);
> +
> +		tst_res(TPASS, "setgroups can't be re-enabled");
> +
> +		sprintf(path, "/proc/%d/setgroups", cpid2);
> +
> +		fd = SAFE_OPEN(path, O_WRONLY, 0644);
> +		SAFE_WRITE(1, fd, "deny", 4);
> +		SAFE_CLOSE(fd);
...

Kind regards,
Petr

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