On Thu, Mar 30, 2017 at 09:06:43PM +0700, Nikolay Marchuk wrote:
[...]
> +int
> +nsfs_ioctl(struct tcb *tcp, unsigned int code, kernel_ulong_t arg)
> +{
> +     uid_t uid;
> +     switch (code) {
> +     case NS_GET_USERNS:
> +     case NS_GET_PARENT:
> +             return 1 + RVAL_FD + RVAL_DECODED;
> +     case NS_GET_NSTYPE:
> +             if (entering(tcp))
> +                     return 0;
> +             if (!syserror(tcp)) {
> +                     const char *outstr;
> +                     outstr = xlookup(setns_types, tcp->u_rval);
> +                     if (outstr) {
> +                             tcp->auxstr = outstr;
> +                             return 1 + RVAL_STR;
> +                     }
> +             }
> +             return 1;
> +     case NS_GET_OWNER_UID:
> +             if (entering(tcp))
> +                     return 0;
> +             tprints(", ");
> +             if (!umove_or_printaddr(tcp, arg, &uid)) {
> +                     printuid("[", uid);
> +                     tprints("]");
> +             }

printuid takes an unsigned int as uid and other parsers except those
defined in uid.c do not use uid_t.  As the libc's idea of uid_t may differ
from kernel's, let's use unsigned int so far.

[...]
> +#include "tests.h"
> +
> +#include <fcntl.h>
> +#include <linux/ioctl.h>
> +#include <sched.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <sys/wait.h>
> +#include <unistd.h>

Please include linux/ headers after libc headers unless necessary.

> +#include "nsfs.h"
> +
> +static void
> +test_no_namespace(void)
> +{
> +     ioctl(-1, NS_GET_USERNS);
> +     printf("ioctl(-1, NS_GET_USERNS) = -1 EBADF (%m)\n");
> +     ioctl(-1, NS_GET_PARENT);
> +     printf("ioctl(-1, NS_GET_PARENT) = -1 EBADF (%m)\n");
> +     ioctl(-1, NS_GET_NSTYPE);
> +     printf("ioctl(-1, NS_GET_NSTYPE) = -1 EBADF (%m)\n");
> +     ioctl(-1, NS_GET_OWNER_UID, NULL);
> +     printf("ioctl(-1, NS_GET_OWNER_UID, NULL) = -1 EBADF (%m)\n");
> +}
> +
> +static void
> +test_clone(pid_t pid)
> +{
> +     int ns_fd, userns_fd, parent_ns_fd, nstype, rc;
> +     /* Path length with terminator is less then 22 in any case. */
> +     char path[22];
> +     TAIL_ALLOC_OBJECT_CONST_PTR(uid_t, uid);
> +
> +     snprintf(path, sizeof(path), "/proc/%d/ns/user", pid);

Why 22?  Wouldn't it be better to use a sizeof based approach,
e.g. like in implementation of getfdproto()?

> +     ns_fd = open(path, O_RDONLY);
> +     if (ns_fd == -1)
> +             perror_msg_and_skip("open: %s", path);
> +
> +     userns_fd = ioctl(ns_fd, NS_GET_USERNS);
> +     printf("ioctl(%d, NS_GET_USERNS) = %s\n", ns_fd, sprintrc(userns_fd));
> +
> +     parent_ns_fd = ioctl(userns_fd, NS_GET_PARENT);
> +     printf("ioctl(%d, NS_GET_PARENT) = %s\n", userns_fd,
> +            sprintrc(parent_ns_fd));

When wrapping printf statements, please wrap the line after format
string, it's more readable.

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

The line is too long, please wrap.

> +     } else {
> +             printf("ioctl(%d, NS_GET_NSTYPE) = %d (CLONE_NEWUSER)\n", 
> userns_fd,
> +                    nstype);

The line is still too long, please wrap it earlier.

> +     }
> +
> +     rc = ioctl(userns_fd, NS_GET_OWNER_UID, uid);
> +     if (rc == -1) {
> +             printf("ioctl(%d, NS_GET_OWNER_UID, %p) = %s\n", userns_fd, uid,
> +                    sprintrc(rc));

When wrapping printf statements, please wrap the line after format
string, it's more readable.

> +     } else {
> +             printf("ioctl(%d, NS_GET_OWNER_UID, [%u]) = %d\n", userns_fd, 
> *uid, rc);

The line is too long, please wrap.

> +     }
> +}
> +
> +static int
> +child(void *arg)
> +{
> +     int *pipefd = (int *)arg;

Please add a space before arg.

> +     close(pipefd[1]);
> +     /* Wait for EOF from pipe. */
> +     while(read(pipefd[0], &pipefd[1], 1) != 0);
> +     return 0;
> +}
> +
> +#define STACK_SIZE (1024 * 1024)
> +
> +static void
> +test_user_namespace(void)
> +{
> +     char stack[STACK_SIZE];

Do you really need 1M of stack for child()?

> +     pid_t pid;
> +     int pipefd[2];
> +     int rc, status;
> +
> +     rc = pipe(pipefd);
> +     if (rc == -1)
> +             perror_msg_and_skip("pipe");
> +
> +     pid = clone(child, stack + STACK_SIZE, (CLONE_NEWUSER | CLONE_UNTRACED
> +                 | SIGCHLD), pipefd);
> +     close(pipefd[0]);

This potentially clobbers errno used by perror_msg_and_skip.

> +     if (pid == -1)
> +             perror_msg_and_skip("clone");

User namespaces are not universally available,
let's just skip this part of the test.


-- 
ldv

Attachment: signature.asc
Description: PGP signature

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Strace-devel mailing list
Strace-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to