Re: [PATCH 5/5] drm: Also check unused fields for addfb2

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 5/5] drm: Also check unused fields for addfb2
Date: Tue, 10 Feb 2015 11:51:51 +0000	[thread overview]
Message-ID: <20150210115151.GO9837@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20150210113651.GK24485@phenom.ffwll.local>

On Tue, Feb 10, 2015 at 12:36:51PM +0100, Daniel Vetter wrote:
> On Tue, Feb 10, 2015 at 11:01:56AM +0000, Chris Wilson wrote:
> > On Mon, Feb 09, 2015 at 07:03:28PM +0100, Daniel Vetter wrote:
> > > Just the usual paranoia ...
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index b15d720eda4c..a12d7e8a0ca0 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -3322,6 +3322,23 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> > >  		}
> > >  	}
> > >  
> > > +	for (; i < 4; i++) {
> > > +		if (r->handles[i]) {
> > > +			DRM_DEBUG_KMS("buffer object handle for unused plane %d\n", i);
> > 
> > Printing the invalid value is also useful. We tended to put user
> > debugging messages as DRM_DEBUG(); would probably be useful to add
> > DRM_DEBUG_USER() and clean up all the EINVAL reporting.
> 
> Generally I agree, but here I couldn't come up with a case where it would
> be useful:
> - Missing memset is just that.
> - Memset is there, but userspace filled in too many buffers - the i it
>   prints should be enough.

The comment was more towards the future, when I expect the validity
checking to be more stringent. For consistency we should use the same
debug level as for other EINVAL logging, and when we come to cut and
paste the error message, having the value in there should remind us to
be verbose.

Even here I expect the value to be useful diagnostic, e.g. to help
narrow down the circumstances under which the invalid ioctl was called.
Maybe it contains poison, maybe it is valid but stale etc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx