From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dmitry V. Levin" Subject: Re: [PATCH v3 4/5] drm: Add decoding of i915 ioctls Date: Wed, 8 Jul 2015 03:11:36 +0300 Message-ID: <20150708001136.GA8326@altlinux.org> References: <1435755168-16207-1-git-send-email-patrik.jakobsson@linux.intel.com> <1435755168-16207-5-git-send-email-patrik.jakobsson@linux.intel.com> <20150703003609.GB29080@altlinux.org> <20150706103552.GB2459@patrik-dev-mach> <20150706164024.6fbf994c@guiness.lab.lse.epita.fr> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2145905685==" Return-path: Received: from pegasus3.altlinux.org (pegasus3.altlinux.org [194.107.17.103]) by gabe.freedesktop.org (Postfix) with ESMTP id 36A586E78C for ; Tue, 7 Jul 2015 17:11:38 -0700 (PDT) In-Reply-To: <20150706164024.6fbf994c@guiness.lab.lse.epita.fr> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Gabriel Laskar Cc: intel-gfx@lists.freedesktop.org, strace-devel@lists.sourceforge.net List-Id: intel-gfx@lists.freedesktop.org --===============2145905685== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NzB8fVQJ5HfG6fxh" Content-Disposition: inline --NzB8fVQJ5HfG6fxh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 @@ > > > > =20 > > > > #define DRM_MAX_NAME_LEN 128 > > > > =20 > > > > +extern int drm_i915_decode_number(struct tcb *tcp, unsigned int ar= g); > > >=20 > > > Please rename "arg" to "code", and ... > > >=20 > > > > +extern int drm_i915_ioctl(struct tcb *tcp, const unsigned int code= , long arg); > > >=20 > > > ... move both declarations to defs.h to make them visible also > > > in the file where these functions are defined. > > >=20 > > > [...] > > > > +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, ¶m)) > > > > + return 0; > > > > + > > > > + tprints(", {param=3D"); > > > > + printxval(drm_i915_setparams, param.param, "I915_PARAM_???"); > > > > + tprintf(", value=3D%d}", param.value); > > > > + } > > > > + > > > > + return 1; > > > > +} > > >=20 > > > 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 r= esult > > > to disabled "arg" decoding, including the fallback decoding performed= by > > > sys_ioctl. > > >=20 > > > Maybe it's time to deal with this issue in a more generic way. > > >=20 > >=20 > > Yes, I'm thinking SYS_FUNC(ioctl) could be improved. But on the other h= and how > > likely is it that we fail in umove and what chance do we have to recove= r from > > that anyway? All I can think of is OOM. >=20 > 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. --=20 ldv --NzB8fVQJ5HfG6fxh Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlWcargACgkQfKvmrJ41Nh5/rQCeKeXqjvuV3pXhJfLhmaC5jhlh Ze8AoIzG23zF6rrg7Gi3VPAQOAnFTMD0 =bU7b -----END PGP SIGNATURE----- --NzB8fVQJ5HfG6fxh-- --===============2145905685== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============2145905685==--