Gsoc Introduction.

Dmitry V. Levin ldv at altlinux.org
Thu Mar 16 02:01:48 UTC 2017


On Wed, Mar 15, 2017 at 03:22:36PM +0700, Nikolay Marchuk wrote:
> Hello,
> I have implemented NS_* ioctls. Should I use common ioctlent interface to
> print them with additional argument or should I print them without it?

Why do you think printing additional arguments makes any sense?

> From f03f6cea9ff2f1ca6fc38c0068844658180c5032 Mon Sep 17 00:00:00 2001
> From: Marchuk Nikolay <marchuk.nikolay.a at gmail.com>
> Date: Wed, 15 Mar 2017 11:59:26 +0700
> Subject: [PATCH 2/2] Add ioctl namespace entries from Linux 4.11
> 
> Signed-off-by: Nikolay Marchuk <marchuk.nikolay.a at gmail.com>
> ---
>  Makefile.am           |  1 +
>  configure.ac          |  1 +
>  defs.h                |  1 +
>  ioctl.c               |  4 +++
>  nsfs.c                | 38 ++++++++++++++++++++++++++++
>  tests/.gitignore      |  1 +
>  tests/Makefile.am     |  2 ++
>  tests/ioctl_nsfs.c    | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/ioctl_nsfs.test | 13 ++++++++++
>  xlat/nsfs_types.in    |  6 +++++
>  10 files changed, 137 insertions(+)
>  create mode 100644 nsfs.c
>  create mode 100644 tests/ioctl_nsfs.c
>  create mode 100755 tests/ioctl_nsfs.test
>  create mode 100644 xlat/nsfs_types.in
> 
> diff --git a/Makefile.am b/Makefile.am
> index c77f463..c673faa 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -172,6 +172,7 @@ strace_SOURCES =	\
>  	net.c		\
>  	netlink.c       \
>  	nsig.h		\
> +	nsfs.c      \

This doesn't look nice.

>  	numa.c		\
>  	oldstat.c	\
>  	open.c		\
> diff --git a/configure.ac b/configure.ac
> index 9e5087b..dc49fdc 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -366,6 +366,7 @@ AC_CHECK_HEADERS(m4_normalize([
>  	linux/ipc.h
>  	linux/mmtimer.h
>  	linux/msg.h
> +	linux/nsfs.h
>  	linux/perf_event.h
>  	linux/quota.h
>  	linux/seccomp.h
> diff --git a/defs.h b/defs.h
> index 793971e..0f3ec14 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -640,6 +640,7 @@ name ## _ioctl(struct tcb *, unsigned int request, kernel_ulong_t arg)
>  DECL_IOCTL(dm);
>  DECL_IOCTL(file);
>  DECL_IOCTL(fs_x);
> +DECL_IOCTL(nsfs);
>  DECL_IOCTL(ptp);
>  DECL_IOCTL(scsi);
>  DECL_IOCTL(term);
> diff --git a/ioctl.c b/ioctl.c
> index aa1880f..d972c38 100644
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -280,6 +280,10 @@ ioctl_decode(struct tcb *tcp)
>  	case 0x94:
>  		return btrfs_ioctl(tcp, code, arg);
>  #endif
> +#ifdef HAVE_LINUX_NSFS_H
> +	case 0xb7:
> +		return nsfs_ioctl(tcp, code, arg);
> +#endif
>  #ifdef HAVE_LINUX_DM_IOCTL_H
>  	case 0xfd:
>  		return dm_ioctl(tcp, code, arg);
> diff --git a/nsfs.c b/nsfs.c
> new file mode 100644
> index 0000000..de98ca9
> --- /dev/null
> +++ b/nsfs.c
> @@ -0,0 +1,38 @@
> +#include "defs.h"
> +
> +#ifdef HAVE_LINUX_NSFS_H
> +
> +#include "xlat/nsfs_types.h"
> +#include <linux/ioctl.h>
> +#include <linux/nsfs.h>
> +
> +/* Definitions for command which have been added later */
> +#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

Please indent.

> +
> +int
> +nsfs_ioctl(struct tcb * tcp, unsigned int code, kernel_ulong_t arg){
> +	char *outstr;
> +	if (entering(tcp) || syserror(tcp))
> +		return 0;

This way the 3rd argument of ioctl syscall is going to be printed.

> +	switch(code){
> +		case NS_GET_USERNS:
> +		case NS_GET_PARENT:
> +			return RVAL_DECODED | RVAL_FD;

This way the 3rd argument of ioctl syscall is going to be printed.

I'm not sure you've got the ioctl return code semantics right,
please read https://sourceforge.net/p/strace/mailman/message/34451172/

I think these 2 operation can (and should) be decoded on entering rather
than on exiting.

> +		case NS_GET_OWNER_UID:
> +			return RVAL_UDECIMAL | RVAL_DECODED;

The semantics of NS_GET_OWNER_UID is different: the uid is written
to the memory at address "arg", see linux fs/nsfs.c.

> +		case NS_GET_NSTYPE:
> +			outstr = xlookup(nsfs_types, tcp->u_rval);
> +			if(outstr){
> +				tcp->auxstr = outstr;
> +				return RVAL_STR | RVAL_DECODED;
> +			}

This way the 3rd argument of ioctl syscall is going to be printed.

> +		default:
> +			return 0;
> +	}
> +}
> +#endif /* HAVE_LINUX_NSFS_H */
> diff --git a/tests/.gitignore b/tests/.gitignore
> index a7754b6..060a391 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -121,6 +121,7 @@ ioctl_loop
>  ioctl_loop-nv
>  ioctl_loop-v
>  ioctl_mtd
> +ioctl_nsfs
>  ioctl_rtc
>  ioctl_rtc-v
>  ioctl_scsi
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c5c124c..bc0c621 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -184,6 +184,7 @@ check_PROGRAMS = \
>  	ioctl_loop-nv \
>  	ioctl_loop-v \
>  	ioctl_mtd \
> +	ioctl_nsfs \
>  	ioctl_rtc \
>  	ioctl_rtc-v \
>  	ioctl_scsi \
> @@ -593,6 +594,7 @@ DECODER_TESTS = \
>  	ioctl_loop-v.test \
>  	ioctl_loop.test \
>  	ioctl_mtd.test \
> +	ioctl_nsfs.test \
>  	ioctl_rtc-v.test \
>  	ioctl_rtc.test \
>  	ioctl_scsi.test \
> diff --git a/tests/ioctl_nsfs.c b/tests/ioctl_nsfs.c
> new file mode 100644
> index 0000000..beea729
> --- /dev/null
> +++ b/tests/ioctl_nsfs.c
> @@ -0,0 +1,70 @@
> +/*
> + * Check decoding of NS_* commands of ioctl syscall.
> + *
> + * Copyright (c) 2017 Nikolay Marchuk <marchuk.nikolay.a at gmail.com>
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. The name of the author may not be used to endorse or promote products
> + *    derived from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "tests.h" 
> +
> +#ifdef HAVE_LINUX_NSFS_H
> +
> +#include <stdio.h>
> +#include <sys/ioctl.h>
> +#include <linux/ioctl.h>
> +#include <linux/nsfs.h>
> +
> +/* Definitions for command which have been added later */
> +#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

Please indent.

> +
> +int
> +main(void)
> +{
> +	static kernel_ulong_t dummy_arg =
> +		(kernel_ulong_t) 0xbadc0dedda7a1057ULL;
> +	ioctl(-1, NS_GET_USERNS, dummy_arg);
> +	printf("ioctl(-1, NS_GET_USERNS, %#lx) = -1 EBADF (%m)\n", dummy_arg);
> +	ioctl(-1, NS_GET_PARENT, dummy_arg);
> +	printf("ioctl(-1, NS_GET_PARENT, %#lx) = -1 EBADF (%m)\n", dummy_arg);
> +	ioctl(-1, NS_GET_NSTYPE, dummy_arg);
> +	printf("ioctl(-1, NS_GET_NSTYPE, %#lx) = -1 EBADF (%m)\n", dummy_arg);
> +	ioctl(-1, NS_GET_OWNER_UID, dummy_arg);
> +	printf("ioctl(-1, NS_GET_OWNER_UID, %#lx) = -1 EBADF (%m)\n", dummy_arg);
> +
> +	puts("+++ exited with 0 +++");
> +	return 0;
> +}

This way you test the generic parser, the most part of your parser (the
switch) is not going to be covered.  Please add some calls with valid ns
descriptors, too.

> +
> +
> +#else /* !HAVE_LINUX_DM_IOCTL_H */
> +
> +SKIP_MAIN_UNDEFINED("HAVE_LINUX_NSFS_H")
> +
> +#endif /* HAVE_LINUX_DM_IOCTL_H */
> \ No newline at end of file
> diff --git a/tests/ioctl_nsfs.test b/tests/ioctl_nsfs.test
> new file mode 100755
> index 0000000..b4604ed
> --- /dev/null
> +++ b/tests/ioctl_nsfs.test
> @@ -0,0 +1,13 @@
> +#!/bin/sh
> +
> +# Check decoding of NS_* ioctls.
> +
> +. "${srcdir=.}/init.sh"
> +
> +run_prog > /dev/null
> +run_strace -a16 -eioctl $args > "$EXP"
> +check_prog grep
> +grep -v '^ioctl([012],' < "$LOG" > "$OUT"
> +match_diff "$OUT" "$EXP"
> +
> +rm -f "$EXP" "$OUT"
> \ No newline at end of file

Please add a newline.

> diff --git a/xlat/nsfs_types.in b/xlat/nsfs_types.in
> new file mode 100644
> index 0000000..249935e
> --- /dev/null
> +++ b/xlat/nsfs_types.in
> @@ -0,0 +1,6 @@
> +CLONE_NEWCGROUP	0x02000000
> +CLONE_NEWUTS	0x04000000
> +CLONE_NEWIPC	0x08000000
> +CLONE_NEWUSER	0x10000000
> +CLONE_NEWPID	0x20000000
> +CLONE_NEWNET	0x40000000 

xlat/setns_types.in looks very similar to this file, do you really need it?


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


More information about the Strace-devel mailing list