From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH 2/2] drm/i915: Initialise userptr mmu_notifier serial to 1 Date: Fri, 11 Jul 2014 12:31:12 +0100 Message-ID: <53BFCB00.1020902@linux.intel.com> References: <1405074481-4742-1-git-send-email-chris@chris-wilson.co.uk> <1405074481-4742-2-git-send-email-chris@chris-wilson.co.uk> <53BFC3CA.3010703@linux.intel.com> <20140711110602.GB12165@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 034B66E1DC for ; Fri, 11 Jul 2014 04:31:14 -0700 (PDT) In-Reply-To: <20140711110602.GB12165@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 07/11/2014 12:06 PM, Chris Wilson wrote: > On Fri, Jul 11, 2014 at 12:00:26PM +0100, Tvrtko Ursulin wrote: >> >> On 07/11/2014 11:28 AM, Chris Wilson wrote: >>> During the range invalidate, we walk the list of buffers associated with >>> the mmu_notifer and find the ones that overlap the range. An >>> optimisation is made to speed up the iteration by assuming the previous >>> iter is still valid whilst the tree is unmodified. This exposes a bug >>> when a range invalidate is triggered after we have just created the >>> mmu_notifier, but before attaching any buffers. In that case, we presume >>> we have an unmodified list and start walking from the last iter which is >>> NULL. Oops. >>> >>> The easiest fix is then to initialise the serial of the tree to 1. >>> >>> Signed-off-by: Chris Wilson >>> Cc: Tvrtko Ursulin >>> --- >>> drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c >>> index 7c38f50014db..8e9e91029aed 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c >>> @@ -197,7 +197,7 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm) >>> mmu->mm = mm; >>> mmu->objects = RB_ROOT; >>> mmu->count = 0; >>> - mmu->serial = 0; >>> + mmu->serial = 1; >>> INIT_LIST_HEAD(&mmu->linear); >>> mmu->is_linear = false; >>> >> >> Looks good to me and I think safe to merge in any case, so: >> >> Reviewed-by: Tvrtko Ursulin >> >> But it will be interesting to know what code managed to trigger this >> race, because as we discussed on IRC it would indicate some pretty >> wild userspace behaviour. Or lack of imagination on our part? > > A threaded client. One thread using userptr, the other doing munmap or > free. Given enough embarrassment, it will happen every time. Yes fine, but I struggle to imagine what would be the intention of such code or how did it manage to fail in such way. I hope the only difference is not that userptr "upgraded" the failure mode for heap corruption or memory management races in general. Tvrtko