Re: [Intel-gfx] [PATCH] drm/i915: Invalidate the relocation presumed_offsets along the slow path

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, 15 Jan 2013 19:21:25 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Jan 15, 2013 at 04:17:54PM +0000, Chris Wilson wrote:
> > In the slow path, we are forced to copy the relocations prior to
> > acquiring the struct mutex in order to handle pagefaults. We forgo
> > copying the new offsets back into the relocation entries in order to
> > prevent a recursive locking bug should we trigger a pagefault whilst
> > holding the mutex for the reservations of the execbuffer. Therefore, we
> > need to reset the presumed_offsets just in case the objects are rebound
> > back into their old locations after relocating for this exexbuffer - if
> > that were to happen we would assume the relocations were valid and leave
> > the actual pointers to the kernels dangling, instant hang.
> > 
> > Fixes regression from commit bcf50e2775bbc3101932d8e4ab8c7902aa4163b4
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Sun Nov 21 22:07:12 2010 +0000
> > 
> >     drm/i915: Handle pagefaults in execbuffer user relocations
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=55984
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@fwll.ch>
> > Cc: stable@vger.kernel.org
> 
> Awesome piece of debugging!
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 4532757..40c062d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -767,6 +767,8 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
> >  	total = 0;
> >  	for (i = 0; i < count; i++) {
> >  		struct drm_i915_gem_relocation_entry __user *user_relocs;
> > +		u64 invalid_offset = (u64)-1;
> 
> I'm a bit uneasy with the semantics here, fearing that a random piece of
> userspace ORs in a few flags instead of adding them. Could we align this
> to 4096 bytes? Or maybe enshrine 0 as our official invalid reloc ...

0 is a valid offset, the location of the stolen fbcon. For the sake of
argument, use (u64)-4096. Even if userspace were suddenly to readback
reloc->presumed_offset (instead of exec->offset) and start or'ing in
flags, it would never match the obj->gtt_offset forcing the relocation
to be recomputed by the kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre