[Intel-gfx] [PATCH v3 4/5] drm: Add decoding of i915 ioctls

[Patrik Jakobsson]

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