[PATCH 2/2] Implemented parser for NS_* ioctl commands.

[Dmitry V. Levin]

On Wed, Mar 22, 2017 at 06:08:01PM +0700, Nikolay Marchuk wrote:
> Test was rewritten, now it is based on example from https://lists.gt.net/linux/kernel/2606267.

Please note that if it's code is based on that example, it has to be
reflected in the copyright header.

> New test passed on 4.11-rc2 kernel.

Nice.

[...]
> diff --git a/nsfs.c b/nsfs.c
> new file mode 100644
> index 0000000..bf12248
> --- /dev/null
> +++ b/nsfs.c
> @@ -0,0 +1,94 @@
> +/*
> + * Support for decoding of NS_* ioctl commands.
[...]
> +#include "defs.h"
> +
> +#include <linux/ioctl.h>
> +
> +/* Definitions for commands */
> +#ifdef HAVE_LINUX_NSFS_H
> +# include <linux/nsfs.h>
> +#else
> +# ifndef NSIO
> +#  define NSIO		0xb7
> +# endif
> +# ifndef NS_GET_USERNS
> +#  define NS_GET_USERNS		_IO(NSIO, 0x1)
> +# endif
> +# ifndef NS_GET_PARENT
> +#  define NS_GET_PARENT		_IO(NSIO, 0x2)
> +# endif
> +#endif
> +
> +#ifndef NS_GET_NSTYPE
> +# define NS_GET_NSTYPE		_IO(NSIO, 0x3)
> +#endif
> +#ifndef NS_GET_OWNER_UID
> +# define NS_GET_OWNER_UID	_IO(NSIO, 0x4)
> +#endif

The same code is repeated verbatim in the test, which means that
it has to be moved into a separate header file.

> +#include <sys/types.h>

This is already included by "defs.h".

> +#include "xlat/setns_types.h"
> +
> +int
> +nsfs_ioctl(struct tcb *tcp, unsigned int code, kernel_ulong_t arg)
> +{
> +	const char *outstr;
> +	uid_t uid;
> +	if (entering(tcp)) {
> +		switch (code) {
> +		case NS_GET_USERNS:
> +		case NS_GET_PARENT:
> +			return RVAL_DECODED | (1 + RVAL_FD);

A simple "return 1 + RVAL_DECODED + RVAL_FD;" would suffice.

> +		default:
> +			return 0;
> +		}
> +	} else {
> +		switch (code) {
> +		case NS_GET_NSTYPE:
> +			if (!syserror(tcp)) {
> +				outstr = xlookup(setns_types, tcp->u_rval);
> +				if (outstr) {
> +					tcp->auxstr = outstr;
> +					return 1 + RVAL_STR;
> +				}
> +			}
> +			return 1 + RVAL_DECIMAL;

A simple "return 1;" would suffice.

> +		case NS_GET_OWNER_UID:
> +			tprints(", ");
> +			if (!umove_or_printaddr(tcp, arg, &uid)) {
> +				printuid("[", uid);
> +				tprints("]");
> +			}
> +			return 1 + RVAL_DECIMAL;

Likewise.

> +		default:
> +			return 0;
> +		}

I think this parser would look simpler if you moved the "entering" check
inside the switch statement, e.g.

	switch (code) {
		case NS_GET_USERNS:
		case NS_GET_PARENT:
			return 1 + RVAL_DECODED + RVAL_FD;
		case NS_GET_NSTYPE:
			if (entering)
				return 0;
			...
		case NS_GET_OWNER_UID:
			if (entering)
				return 0;
			...
		default:
			return RVAL_DECODED;
	}

[...]
> +static void
> +test_clone(pid_t pid)
> +{
> +	int ns_fd, userns_fd, parent_ns_fd, nstype, rc;
> +	const char *errstr;
> +	char path[PATH_MAX];

Why have you chosen PATH_MAX here?

> +	uid_t uid;
> +
> +	snprintf(path, sizeof(path), "/proc/%d/ns/user", pid);
> +	ns_fd = open(path, O_RDONLY);
> +	if (ns_fd == -1)
> +		perror_msg_and_skip("Open user namespace file error");

We prefer the following format:

		perror_msg_and_skip("open: %s", path);

> +	userns_fd = ioctl(ns_fd, NS_GET_USERNS);
> +	errstr = sprintrc(userns_fd);
> +	printf("ioctl(%d, NS_GET_USERNS) = %s\n", ns_fd, errstr);

No need to save sprintrc return code to errstr if it's not necessary.

The same statement could be written as
	printf("ioctl(%d, NS_GET_USERNS) = %s\n", ns_fd, sprintrc(userns_fd);

> +	parent_ns_fd = ioctl(userns_fd, NS_GET_PARENT);
> +	errstr = sprintrc(parent_ns_fd);
> +	printf("ioctl(%d, NS_GET_PARENT) = %s\n", userns_fd, errstr);

Likewise.

> +	nstype = ioctl(userns_fd, NS_GET_NSTYPE);
> +	errstr = sprintrc(nstype);
> +	if (nstype == CLONE_NEWUSER) {
> +		printf("ioctl(%d, NS_GET_NSTYPE) = %d (CLONE_NEWUSER)\n", userns_fd,
> +			   nstype);

This is not the best choice of wrapping.

> +	} else {
> +		printf("ioctl(%d, NS_GET_NSTYPE) = %s\n", userns_fd, errstr);
> +	}
> +
> +	rc = ioctl(userns_fd, NS_GET_OWNER_UID, &uid);

Use uid obtained using TAIL_ALLOC_OBJECT_CONST_PTR instead of an object
placed on the stack.

> +	errstr = sprintrc(rc);
> +	if (rc == -1) {
> +		printf("ioctl(%d, NS_GET_OWNER_UID, %p) = %s\n", userns_fd, &uid,
> +			   errstr);
> +	} else {
> +		printf("ioctl(%d, NS_GET_OWNER_UID, [", userns_fd);
> +		if ((uid_t) -1U == (uid_t) uid)
> +			printf("-1]) = %s\n", errstr);
> +		else
> +			printf("%u]) = %s\n", uid, errstr);
> +	}

Do we ever expect an uid == -1?

> +}
> +
> +static int
> +childFunc(void *arg)

Just call it child.

> +{
> +		sleep(5);

Why 5?  Why sleep?  It's not the best way of synchronization.

> +		return 0;
> +}
> +
> +#define STACK_SIZE (1024 * 1024)
> +
> +static void
> +test_user_namespace(void)
> +{
> +	char *stack;
> +	pid_t pid;
> +
> +	stack = malloc(STACK_SIZE);
> +	if (stack == NULL) {
> +		perror_msg_and_skip("Malloc error");
> +	}

Why malloc instead of a static chunk of memory?

> +	pid = clone(childFunc, stack + STACK_SIZE, CLONE_NEWUSER | CLONE_UNTRACED,
> +				NULL);
> +	if (pid == -1)
> +		perror_msg_and_skip("Clone error");

No need to append "error" to every error message.
A simple "clone" would suffice.

> +	test_clone(pid);
> +	kill(pid, SIGKILL);

sleep+kill is a very rough synchronisation method, could you think
of something more robust, e.g. pipe/read/close/wait?


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170324/c72b0668/attachment.bin>