On Tue, 15 Jan 2013 19:21:25 +0100, Daniel Vetter <daniel at 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 at 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 at chris-wilson.co.uk> > > Cc: Daniel Vetter <daniel.vetter at fwll.ch> > > Cc: stable at 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
