[v3,4/5] drm: Add decoding of i915 ioctls

Submitted by Patrik Jakobsson on July 1, 2015, 12:52 p.m.

Details

Message ID 1435755168-16207-5-git-send-email-patrik.jakobsson@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Patrik Jakobsson July 1, 2015, 12:52 p.m.
There are more ioctls to add but the ones in this patch are most
commonly used.

* Makefile.am: Add compilation of drm_i915.c
* drm.c: Dispatch i915 ioctls
* drm_i915.c: Decode DRM_IOCTL_I915_GETPARAM
* drm_i915.c: Decode DRM_IOCTL_I915_SETPARAM
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_EXECBUFFER2
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_BUSY
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_CREATE
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_PREAD
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_PWRITE
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_MMAP
* drm_i915.c: Decode DRM_IOCTL_I915_GEM_MMAP_GTT
* drm_i915.c: Decode DRM_IOCTL_I915_SET_DOMAIN
* drm_i915.c: Decode DRM_IOCTL_I915_MADVISE
* drm_i915.c: Decode DRM_IOCTL_I915_GET_TILING
* drm_i915.c: Decode DRM_IOCTL_I915_SET_TILING
* drm_i915.c: Decode DRM_IOCTL_I915_USERPTR
* xlat/drm_i915_getparams.in: List GETPARAM parameters
* xlat/drm_i915_setparams.in: List SETPARAM parameters
* xlat/drm_i915_ioctls.in: List i915 ioctls

Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
---
 Makefile.am                |   1 +
 drm.c                      |  18 ++-
 drm_i915.c                 | 332 +++++++++++++++++++++++++++++++++++++++++++++
 xlat/drm_i915_getparams.in |  28 ++++
 xlat/drm_i915_ioctls.in    |  51 +++++++
 xlat/drm_i915_setparams.in |   4 +
 6 files changed, 433 insertions(+), 1 deletion(-)
 create mode 100644 drm_i915.c
 create mode 100644 xlat/drm_i915_getparams.in
 create mode 100644 xlat/drm_i915_ioctls.in
 create mode 100644 xlat/drm_i915_setparams.in

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 99382bd..aa27e0c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -42,6 +42,7 @@  strace_SOURCES =	\
 	desc.c		\
 	dirent.c	\
 	drm.c		\
+	drm_i915.c	\
 	evdev.c		\
 	execve.c	\
 	exit.c		\
diff --git a/drm.c b/drm.c
index 61df09f..846ea4d 100644
--- a/drm.c
+++ b/drm.c
@@ -35,6 +35,9 @@ 
 
 #define DRM_MAX_NAME_LEN 128
 
+extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
+extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);
+
 struct drm_ioctl_priv {
 	char name[DRM_MAX_NAME_LEN];
 };
@@ -93,16 +96,29 @@  int drm_is_driver(struct tcb *tcp, const char *name)
 
 int drm_decode_number(struct tcb *tcp, unsigned int arg)
 {
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			return drm_i915_decode_number(tcp, arg);
+	}
+
 	return 0;
 }
 
 int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
 {
+	int ret = 0;
+
+	/* Check for device specific ioctls */
+	if (drm_is_priv(tcp->u_arg[1])) {
+		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
+			ret = drm_i915_ioctl(tcp, code, arg);
+	}
+
 	/* Free any allocated private data */
 	if (exiting(tcp) && tcp->priv_data != NULL) {
 		free(tcp->priv_data);
 		tcp->priv_data = NULL;
 	}
 
-	return 0;
+	return ret;
 }
diff --git a/drm_i915.c b/drm_i915.c
new file mode 100644
index 0000000..e4705b6
--- /dev/null
+++ b/drm_i915.c
@@ -0,0 +1,332 @@ 
+/*
+ * Copyright (c) 2015 Intel Corporation
+ * 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.
+ *
+ * Authors:
+ *    Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
+ */
+
+#include "defs.h"
+
+#include <drm.h>
+#include <i915_drm.h>
+#include "xlat/drm_i915_ioctls.h"
+#include "xlat/drm_i915_getparams.h"
+#include "xlat/drm_i915_setparams.h"
+
+static int i915_getparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_getparam param;
+	int value;
+
+	if (umove(tcp, arg, &param))
+		return 0;
+
+	if (entering(tcp)) {
+		tprints(", {param=");
+		printxval(drm_i915_getparams, param.param, "I915_PARAM_???");
+	} else if (exiting(tcp)) {
+		if (umove(tcp, (long)param.value, &value))
+			return 0;
+
+		tprints(", value=");
+		switch (param.param) {
+		case I915_PARAM_CHIPSET_ID:
+			tprintf("0x%04x", value);
+			break;
+		default:
+			tprintf("%d", value);
+		}
+		tprints("}");
+	}
+
+	return 1;
+}
+
+static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_setparam param;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &param))
+			return 0;
+
+		tprints(", {param=");
+		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
+		tprintf(", value=%d}", param.value);
+	}
+
+	return 1;
+}
+
+static int i915_gem_execbuffer2(struct tcb *tcp, const unsigned int code,
+				long arg)
+{
+	struct drm_i915_gem_execbuffer2 eb;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &eb))
+			return 0;
+
+		tprintf(", {buffers_ptr=%p, buffer_count=%u, "
+			"batch_start_offset=%x, batch_len=%u, DR1=%u, DR4=%u, "
+			"num_cliprects=%u, cliprects_ptr=%p, flags=0x%Lx}",
+			(void *)eb.buffers_ptr, eb.buffer_count,
+			eb.batch_start_offset, eb.batch_len, eb.DR1, eb.DR4,
+			eb.num_cliprects, (void *)eb.cliprects_ptr, eb.flags);
+	}
+
+	return 1;
+}
+
+static int i915_gem_busy(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_busy busy;
+
+	if (umove(tcp, arg, &busy))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", busy.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", busy=%c, ring=%u}",
+			(busy.busy & 0x1) ? 'Y' : 'N', (busy.busy >> 16));
+	}
+
+	return 1;
+}
+
+static int i915_gem_create(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_create create;
+
+	if (umove(tcp, arg, &create))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {size=%Lu", create.size);
+	} else if (exiting(tcp)) {
+		tprintf(", handle=%u}", create.handle);
+	}
+
+	return 1;
+}
+
+static int i915_gem_pread(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pread pr;
+
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pr))
+			return 0;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pr.handle, pr.offset, pr.size, (void *)pr.data_ptr);
+	}
+
+	return 1;
+}
+
+static int i915_gem_pwrite(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_pwrite pw;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &pw))
+			return 0;
+
+		tprintf(", {handle=%u, offset=%Lu, size=%Lu, data_ptr=%p}",
+			pw.handle, pw.offset, pw.size, (void *)pw.data_ptr);
+	}
+
+	return 1;
+}
+
+static int i915_gem_mmap(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, size=%Lu", mmap.handle, mmap.size);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu, addr_ptr=%p}",
+			mmap.offset, (void *)mmap.addr_ptr);
+	}
+
+	return 1;
+}
+
+static int i915_gem_mmap_gtt(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_mmap_gtt mmap;
+
+	if (umove(tcp, arg, &mmap))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", mmap.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", offset=%Lu}", mmap.offset);
+	}
+
+	return 1;
+}
+
+static int i915_gem_set_domain(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_domain dom;
+
+	if (entering(tcp)) {
+		if (umove(tcp, arg, &dom))
+			return 0;
+
+		tprintf(", {handle=%u, read_domains=%x, write_domain=%x}",
+			dom.handle, dom.read_domains, dom.write_domain);
+	}
+
+	return 1;
+}
+
+static int i915_gem_madvise(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_madvise madv;
+
+	if (umove(tcp, arg, &madv))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, madv=%u", madv.handle, madv.madv);
+	} else if (exiting(tcp)) {
+		tprintf(", retained=%u}", madv.retained);
+	}
+
+	return 1;
+}
+
+static int i915_gem_get_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_get_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u", tiling.handle);
+	} else if (exiting(tcp)) {
+		tprintf(", tiling_mode=%u, swizzle_mode=%u}", tiling.tiling_mode,
+			tiling.swizzle_mode);
+	}
+
+	return 1;
+}
+
+static int i915_gem_set_tiling(struct tcb *tcp, const unsigned int code,
+			       long arg)
+{
+	struct drm_i915_gem_set_tiling tiling;
+
+	if (umove(tcp, arg, &tiling))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {handle=%u, tiling_mode=%u, stride=%u",
+			tiling.handle, tiling.tiling_mode, tiling.stride);
+	} else if (exiting(tcp)) {
+		tprintf(", swizzle_mode=%u}", tiling.swizzle_mode);
+	}
+
+	return 1;
+}
+
+static int i915_gem_userptr(struct tcb *tcp, const unsigned int code, long arg)
+{
+	struct drm_i915_gem_userptr uptr;
+
+	if (umove(tcp, arg, &uptr))
+		return 0;
+
+	if (entering(tcp)) {
+		tprintf(", {user_ptr=%p, user_size=%Lu", (void *)uptr.user_ptr,
+			uptr.user_size);
+	} else if (exiting(tcp)) {
+		tprintf(", flags=0x%x, handle=%u}", uptr.flags, uptr.handle);
+	}
+
+	return 1;
+}
+
+int drm_i915_decode_number(struct tcb *tcp, unsigned int arg)
+{
+	const char *str = xlookup(drm_i915_ioctls, arg);
+
+	if (str) {
+		tprintf("%s", str);
+		return 1;
+	}
+
+	return 0;
+}
+
+int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg)
+{
+	switch (code) {
+	case DRM_IOCTL_I915_GETPARAM:
+		return i915_getparam(tcp, code, arg);
+	case DRM_IOCTL_I915_SETPARAM:
+		return i915_setparam(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_EXECBUFFER2:
+		return i915_gem_execbuffer2(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_BUSY:
+		return i915_gem_busy(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_CREATE:
+		return i915_gem_create(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PREAD:
+		return i915_gem_pread(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_PWRITE:
+		return i915_gem_pwrite(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP:
+		return i915_gem_mmap(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MMAP_GTT:
+		return i915_gem_mmap_gtt(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_DOMAIN:
+		return i915_gem_set_domain(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_MADVISE:
+		return i915_gem_madvise(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_GET_TILING:
+		return i915_gem_get_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_SET_TILING:
+		return i915_gem_set_tiling(tcp, code, arg);
+	case DRM_IOCTL_I915_GEM_USERPTR:
+		return i915_gem_userptr(tcp, code, arg);
+	}
+
+	return 0;
+}
diff --git a/xlat/drm_i915_getparams.in b/xlat/drm_i915_getparams.in
new file mode 100644
index 0000000..15275cb
--- /dev/null
+++ b/xlat/drm_i915_getparams.in
@@ -0,0 +1,28 @@ 
+I915_PARAM_IRQ_ACTIVE
+I915_PARAM_ALLOW_BATCHBUFFER
+I915_PARAM_LAST_DISPATCH
+I915_PARAM_CHIPSET_ID
+I915_PARAM_HAS_GEM
+I915_PARAM_NUM_FENCES_AVAIL
+I915_PARAM_HAS_OVERLAY
+I915_PARAM_HAS_PAGEFLIPPING
+I915_PARAM_HAS_EXECBUF2
+I915_PARAM_HAS_BSD
+I915_PARAM_HAS_BLT
+I915_PARAM_HAS_RELAXED_FENCING
+I915_PARAM_HAS_COHERENT_RINGS
+I915_PARAM_HAS_EXEC_CONSTANTS
+I915_PARAM_HAS_RELAXED_DELTA
+I915_PARAM_HAS_GEN7_SOL_RESET
+I915_PARAM_HAS_LLC
+I915_PARAM_HAS_ALIASING_PPGTT
+I915_PARAM_HAS_WAIT_TIMEOUT
+I915_PARAM_HAS_SEMAPHORES
+I915_PARAM_HAS_PRIME_VMAP_FLUSH
+I915_PARAM_HAS_VEBOX
+I915_PARAM_HAS_SECURE_BATCHES
+I915_PARAM_HAS_PINNED_BATCHES
+I915_PARAM_HAS_EXEC_NO_RELOC
+I915_PARAM_HAS_EXEC_HANDLE_LUT
+I915_PARAM_HAS_WT
+I915_PARAM_CMD_PARSER_VERSION
diff --git a/xlat/drm_i915_ioctls.in b/xlat/drm_i915_ioctls.in
new file mode 100644
index 0000000..21c3397
--- /dev/null
+++ b/xlat/drm_i915_ioctls.in
@@ -0,0 +1,51 @@ 
+/* Unfortunately i915 collides with other DRM drivers so we must specify these */
+DRM_IOCTL_I915_INIT
+DRM_IOCTL_I915_FLUSH
+DRM_IOCTL_I915_FLIP
+DRM_IOCTL_I915_BATCHBUFFER
+DRM_IOCTL_I915_IRQ_EMIT
+DRM_IOCTL_I915_IRQ_WAIT
+DRM_IOCTL_I915_GETPARAM
+DRM_IOCTL_I915_SETPARAM
+DRM_IOCTL_I915_ALLOC
+DRM_IOCTL_I915_FREE
+DRM_IOCTL_I915_INIT_HEAP
+DRM_IOCTL_I915_CMDBUFFER
+DRM_IOCTL_I915_DESTROY_HEAP
+DRM_IOCTL_I915_SET_VBLANK_PIPE
+DRM_IOCTL_I915_GET_VBLANK_PIPE
+DRM_IOCTL_I915_VBLANK_SWAP
+DRM_IOCTL_I915_HWS_ADDR
+DRM_IOCTL_I915_GEM_INIT
+DRM_IOCTL_I915_GEM_EXECBUFFER
+DRM_IOCTL_I915_GEM_EXECBUFFER2
+DRM_IOCTL_I915_GEM_PIN
+DRM_IOCTL_I915_GEM_UNPIN
+DRM_IOCTL_I915_GEM_BUSY
+DRM_IOCTL_I915_GEM_SET_CACHING
+DRM_IOCTL_I915_GEM_GET_CACHING
+DRM_IOCTL_I915_GEM_THROTTLE
+DRM_IOCTL_I915_GEM_ENTERVT
+DRM_IOCTL_I915_GEM_LEAVEVT
+DRM_IOCTL_I915_GEM_CREATE
+DRM_IOCTL_I915_GEM_PREAD
+DRM_IOCTL_I915_GEM_PWRITE
+DRM_IOCTL_I915_GEM_MMAP
+DRM_IOCTL_I915_GEM_MMAP_GTT
+DRM_IOCTL_I915_GEM_SET_DOMAIN
+DRM_IOCTL_I915_GEM_SW_FINISH
+DRM_IOCTL_I915_GEM_SET_TILING
+DRM_IOCTL_I915_GEM_GET_TILING
+DRM_IOCTL_I915_GEM_GET_APERTURE
+DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID
+DRM_IOCTL_I915_GEM_MADVISE
+DRM_IOCTL_I915_OVERLAY_PUT_IMAGE
+DRM_IOCTL_I915_OVERLAY_ATTRS
+DRM_IOCTL_I915_SET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GET_SPRITE_COLORKEY
+DRM_IOCTL_I915_GEM_WAIT
+DRM_IOCTL_I915_GEM_CONTEXT_CREATE
+DRM_IOCTL_I915_GEM_CONTEXT_DESTROY
+DRM_IOCTL_I915_REG_READ
+DRM_IOCTL_I915_GET_RESET_STATS
+DRM_IOCTL_I915_GEM_USERPTR
diff --git a/xlat/drm_i915_setparams.in b/xlat/drm_i915_setparams.in
new file mode 100644
index 0000000..d93d2ea
--- /dev/null
+++ b/xlat/drm_i915_setparams.in
@@ -0,0 +1,4 @@ 
+I915_SETPARAM_USE_MI_BATCHBUFFER_START
+I915_SETPARAM_TEX_LRU_LOG_GRANULARITY
+I915_SETPARAM_ALLOW_BATCHBUFFER
+I915_SETPARAM_NUM_USED_FENCES

Comments

On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
[...]
> --- a/drm.c
> +++ b/drm.c
> @@ -35,6 +35,9 @@
>  
>  #define DRM_MAX_NAME_LEN 128
>  
> +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);

Please rename "arg" to "code", and ...

> +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);

... move both declarations to defs.h to make them visible also
in the file where these functions are defined.

[...]
> +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct drm_i915_setparam param;
> +
> +	if (entering(tcp)) {
> +		if (umove(tcp, arg, &param))
> +			return 0;
> +
> +		tprints(", {param=");
> +		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
> +		tprintf(", value=%d}", param.value);
> +	}
> +
> +	return 1;
> +}

In this and most of other parsers of _IOC_WRITE ioctls added by this and
the next patches, any error in parser that leads to "return 0" will result
to disabled "arg" decoding, including the fallback decoding performed by
sys_ioctl.

Maybe it's time to deal with this issue in a more generic way.
On Fri, Jul 03, 2015 at 03:36:09AM +0300, Dmitry V. Levin wrote:
> On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
> [...]
> > --- a/drm.c
> > +++ b/drm.c
> > @@ -35,6 +35,9 @@
> >  
> >  #define DRM_MAX_NAME_LEN 128
> >  
> > +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
> 
> Please rename "arg" to "code", and ...
> 
> > +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);
> 
> ... move both declarations to defs.h to make them visible also
> in the file where these functions are defined.
> 
> [...]
> > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_i915_setparam param;
> > +
> > +	if (entering(tcp)) {
> > +		if (umove(tcp, arg, &param))
> > +			return 0;
> > +
> > +		tprints(", {param=");
> > +		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
> > +		tprintf(", value=%d}", param.value);
> > +	}
> > +
> > +	return 1;
> > +}
> 
> In this and most of other parsers of _IOC_WRITE ioctls added by this and
> the next patches, any error in parser that leads to "return 0" will result
> to disabled "arg" decoding, including the fallback decoding performed by
> sys_ioctl.
> 
> Maybe it's time to deal with this issue in a more generic way.
> 

Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other hand how
likely is it that we fail in umove and what chance do we have to recover from
that anyway? All I can think of is OOM.

> 
> -- 
> ldv
On Mon, 6 Jul 2015 12:35:52 +0200
Patrik Jakobsson <patrik.jakobsson@linux.intel.com> wrote:

> On Fri, Jul 03, 2015 at 03:36:09AM +0300, Dmitry V. Levin wrote:
> > On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
> > [...]
> > > --- a/drm.c
> > > +++ b/drm.c
> > > @@ -35,6 +35,9 @@
> > >  
> > >  #define DRM_MAX_NAME_LEN 128
> > >  
> > > +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
> > 
> > Please rename "arg" to "code", and ...
> > 
> > > +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);
> > 
> > ... move both declarations to defs.h to make them visible also
> > in the file where these functions are defined.
> > 
> > [...]
> > > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > > +{
> > > +	struct drm_i915_setparam param;
> > > +
> > > +	if (entering(tcp)) {
> > > +		if (umove(tcp, arg, &param))
> > > +			return 0;
> > > +
> > > +		tprints(", {param=");
> > > +		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
> > > +		tprintf(", value=%d}", param.value);
> > > +	}
> > > +
> > > +	return 1;
> > > +}
> > 
> > In this and most of other parsers of _IOC_WRITE ioctls added by this and
> > the next patches, any error in parser that leads to "return 0" will result
> > to disabled "arg" decoding, including the fallback decoding performed by
> > sys_ioctl.
> > 
> > Maybe it's time to deal with this issue in a more generic way.
> > 
> 
> Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other hand how
> likely is it that we fail in umove and what chance do we have to recover from
> that anyway? All I can think of is OOM.

umove() can fail in multiple ways. For example, if the memory is not
valid in the tracee, umove() will fail.

Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
fallbacks on failure should be more generic.
On Mon, Jul 06, 2015 at 04:40:24PM +0200, Gabriel Laskar wrote:
> On Mon, 6 Jul 2015 12:35:52 +0200, Patrik Jakobsson wrote:
> > On Fri, Jul 03, 2015 at 03:36:09AM +0300, Dmitry V. Levin wrote:
> > > On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
> > > [...]
> > > > --- a/drm.c
> > > > +++ b/drm.c
> > > > @@ -35,6 +35,9 @@
> > > >  
> > > >  #define DRM_MAX_NAME_LEN 128
> > > >  
> > > > +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
> > > 
> > > Please rename "arg" to "code", and ...
> > > 
> > > > +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);
> > > 
> > > ... move both declarations to defs.h to make them visible also
> > > in the file where these functions are defined.
> > > 
> > > [...]
> > > > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > > > +{
> > > > +	struct drm_i915_setparam param;
> > > > +
> > > > +	if (entering(tcp)) {
> > > > +		if (umove(tcp, arg, &param))
> > > > +			return 0;
> > > > +
> > > > +		tprints(", {param=");
> > > > +		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
> > > > +		tprintf(", value=%d}", param.value);
> > > > +	}
> > > > +
> > > > +	return 1;
> > > > +}
> > > 
> > > In this and most of other parsers of _IOC_WRITE ioctls added by this and
> > > the next patches, any error in parser that leads to "return 0" will result
> > > to disabled "arg" decoding, including the fallback decoding performed by
> > > sys_ioctl.
> > > 
> > > Maybe it's time to deal with this issue in a more generic way.
> > > 
> > 
> > Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other hand how
> > likely is it that we fail in umove and what chance do we have to recover from
> > that anyway? All I can think of is OOM.
> 
> umove() can fail in multiple ways. For example, if the memory is not
> valid in the tracee, umove() will fail.

Yes, this is the most likely cause for umove() to fail,
and the most easily reproducible one, e.g.
ioctl(-1, DRM_IOCTL_VERSION, 42);

> Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
> fallbacks on failure should be more generic.

What would be useful is a way for "on entering" parsers to return
"done with decoding" information to their callers.

This could be implemented by or'ing return value in the current semantics
with a flag with "done with decoding" meaning, e.g. RVAL_DONE.

If an ioctl parser returned RVAL_DONE, this would tell SYS_FUNC(ioctl)
that the decoding is finished but fallback decoding is needed, while
RVAL_DONE+1 would mean that the decoding is finished and no fallback
decoding is needed.
On Wed, Jul 08, 2015 at 03:11:36AM +0300, Dmitry V. Levin wrote:
> On Mon, Jul 06, 2015 at 04:40:24PM +0200, Gabriel Laskar wrote:
> > On Mon, 6 Jul 2015 12:35:52 +0200, Patrik Jakobsson wrote:
> > > On Fri, Jul 03, 2015 at 03:36:09AM +0300, Dmitry V. Levin wrote:
> > > > On Wed, Jul 01, 2015 at 02:52:47PM +0200, Patrik Jakobsson wrote:
> > > > [...]
> > > > > --- a/drm.c
> > > > > +++ b/drm.c
> > > > > @@ -35,6 +35,9 @@
> > > > >  
> > > > >  #define DRM_MAX_NAME_LEN 128
> > > > >  
> > > > > +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int arg);
> > > > 
> > > > Please rename "arg" to "code", and ...
> > > > 
> > > > > +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code, long arg);
> > > > 
> > > > ... move both declarations to defs.h to make them visible also
> > > > in the file where these functions are defined.
> > > > 
> > > > [...]
> > > > > +static int i915_setparam(struct tcb *tcp, const unsigned int code, long arg)
> > > > > +{
> > > > > +	struct drm_i915_setparam param;
> > > > > +
> > > > > +	if (entering(tcp)) {
> > > > > +		if (umove(tcp, arg, &param))
> > > > > +			return 0;
> > > > > +
> > > > > +		tprints(", {param=");
> > > > > +		printxval(drm_i915_setparams, param.param, "I915_PARAM_???");
> > > > > +		tprintf(", value=%d}", param.value);
> > > > > +	}
> > > > > +
> > > > > +	return 1;
> > > > > +}
> > > > 
> > > > In this and most of other parsers of _IOC_WRITE ioctls added by this and
> > > > the next patches, any error in parser that leads to "return 0" will result
> > > > to disabled "arg" decoding, including the fallback decoding performed by
> > > > sys_ioctl.
> > > > 
> > > > Maybe it's time to deal with this issue in a more generic way.
> > > > 
> > > 
> > > Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other hand how
> > > likely is it that we fail in umove and what chance do we have to recover from
> > > that anyway? All I can think of is OOM.
> > 
> > umove() can fail in multiple ways. For example, if the memory is not
> > valid in the tracee, umove() will fail.
> 
> Yes, this is the most likely cause for umove() to fail,
> and the most easily reproducible one, e.g.
> ioctl(-1, DRM_IOCTL_VERSION, 42);

Yes, then we definitely need to handle those fails better

> 
> > Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
> > fallbacks on failure should be more generic.
> 
> What would be useful is a way for "on entering" parsers to return
> "done with decoding" information to their callers.
> 
> This could be implemented by or'ing return value in the current semantics
> with a flag with "done with decoding" meaning, e.g. RVAL_DONE.
> 
> If an ioctl parser returned RVAL_DONE, this would tell SYS_FUNC(ioctl)
> that the decoding is finished but fallback decoding is needed, while
> RVAL_DONE+1 would mean that the decoding is finished and no fallback
> decoding is needed.

I like that idea but isn't the current return semantics already good enough
for that? The problem right now is that we ignore the return value from
ioctl_decode() "on entering". What we could do is:
1. Call ioctl_decode_number()
2. Call ioctl_decode() if above didn't fail
3. If any of 1 and/or 2 failed we do fallback

> 
> 
> -- 
> ldv
On Fri, Jul 10, 2015 at 02:36:38PM +0200, Patrik Jakobsson wrote:
> On Wed, Jul 08, 2015 at 03:11:36AM +0300, Dmitry V. Levin wrote:
> > On Mon, Jul 06, 2015 at 04:40:24PM +0200, Gabriel Laskar wrote:
[...]
> > > Anyway, SYS_FUNC(ioctl) is a bit complicated, and the handling of the
> > > fallbacks on failure should be more generic.
> > 
> > What would be useful is a way for "on entering" parsers to return
> > "done with decoding" information to their callers.
> > 
> > This could be implemented by or'ing return value in the current semantics
> > with a flag with "done with decoding" meaning, e.g. RVAL_DONE.
> > 
> > If an ioctl parser returned RVAL_DONE, this would tell SYS_FUNC(ioctl)
> > that the decoding is finished but fallback decoding is needed, while
> > RVAL_DONE+1 would mean that the decoding is finished and no fallback
> > decoding is needed.
> 
> I like that idea but isn't the current return semantics already good enough
> for that? The problem right now is that we ignore the return value from
> ioctl_decode() "on entering".

After commit v4.10-104-g204c2bc we no longer ignore the return value from
ioctl_decode() "on entering".