From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 969D5C433E0 for ; Mon, 18 Jan 2021 12:35:33 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 204DE2225C for ; Mon, 18 Jan 2021 12:35:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 204DE2225C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5EFFC89F89; Mon, 18 Jan 2021 12:35:32 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3D15289F89 for ; Mon, 18 Jan 2021 12:35:31 +0000 (UTC) IronPort-SDR: dfKCIA5fJaAeZY58I+im+/JfYyYV6su9pxp3N+1U4xYD2zwFbHy+Vhj3x1unnuC1Ln2p3BoPef Kmj+n4lHFLTQ== X-IronPort-AV: E=McAfee;i="6000,8403,9867"; a="240338906" X-IronPort-AV: E=Sophos;i="5.79,356,1602572400"; d="scan'208";a="240338906" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jan 2021 04:35:30 -0800 IronPort-SDR: 62yfiw62APF2ha73gCbBTwFAYh1U1HaVaAZAXKEXTNJm9C0ZwdNXOynbqvdwBpat/FT79MtlF2 cHi8y4Xs0Kbw== X-IronPort-AV: E=Sophos;i="5.79,356,1602572400"; d="scan'208";a="383554617" Received: from ynetzer-mobl.ger.corp.intel.com (HELO localhost) ([10.252.42.241]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jan 2021 04:35:28 -0800 From: Jani Nikula To: Chris Wilson , intel-gfx@lists.freedesktop.org In-Reply-To: <20210118101755.476744-1-chris@chris-wilson.co.uk> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <87lfcqobpl.fsf@intel.com> <20210118101755.476744-1-chris@chris-wilson.co.uk> Date: Mon, 18 Jan 2021 14:35:25 +0200 Message-ID: <87czy2o22a.fsf@intel.com> MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH] drm/i915: Check for rq->hwsp validity after acquiring RCU lock X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: stable@vger.kernel.org, Chris Wilson Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, 18 Jan 2021, Chris Wilson wrote: > Since we allow removing the timeline map at runtime, there is a risk > that rq->hwsp points into a stale page. To control that risk, we hold > the RCU read lock while reading *rq->hwsp, but we missed a couple of > important barriers. First, the unpinning / removal of the timeline map > must be after all RCU readers into that map are complete, i.e. after an > rcu barrier (in this case courtesy of call_rcu()). Secondly, we must > make sure that the rq->hwsp we are about to dereference under the RCU > lock is valid. In this case, we make the rq->hwsp pointer safe during > i915_request_retire() and so we know that rq->hwsp may become invalid > only after the request has been signaled. Therefore is the request is > not yet signaled when we acquire rq->hwsp under the RCU, we know that > rq->hwsp will remain valid for the duration of the RCU read lock. > > This is a very small window that may lead to either considering the > request not completed (causing a delay until the request is checked > again, any wait for the request is not affected) or dereferencing an > invalid pointer. > > Fixes: 3adac4689f58 ("drm/i915: Introduce concept of per-timeline (context) HWSP") > Signed-off-by: Chris Wilson > Cc: Tvrtko Ursulin > Cc: # v5.1+ > Reviewed-by: Tvrtko Ursulin > Link: https://patchwork.freedesktop.org/patch/msgid/20201218122421.18344-1-chris@chris-wilson.co.uk > (cherry picked from commit 9bb36cf66091ddf2d8840e5aa705ad3c93a6279b) Thanks for the backports, all three pushed to drm-intel-fixes. BR, Jani. > --- > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 9 ++--- > drivers/gpu/drm/i915/gt/intel_timeline.c | 10 +++--- > drivers/gpu/drm/i915/i915_request.h | 37 ++++++++++++++++++--- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c > index a24cc1ff08a0..0625cbb3b431 100644 > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c > @@ -134,11 +134,6 @@ static bool remove_signaling_context(struct intel_breadcrumbs *b, > return true; > } > > -static inline bool __request_completed(const struct i915_request *rq) > -{ > - return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno); > -} > - > __maybe_unused static bool > check_signal_order(struct intel_context *ce, struct i915_request *rq) > { > @@ -257,7 +252,7 @@ static void signal_irq_work(struct irq_work *work) > list_for_each_entry_rcu(rq, &ce->signals, signal_link) { > bool release; > > - if (!__request_completed(rq)) > + if (!__i915_request_is_complete(rq)) > break; > > if (!test_and_clear_bit(I915_FENCE_FLAG_SIGNAL, > @@ -379,7 +374,7 @@ static void insert_breadcrumb(struct i915_request *rq) > * straight onto a signaled list, and queue the irq worker for > * its signal completion. > */ > - if (__request_completed(rq)) { > + if (__i915_request_is_complete(rq)) { > if (__signal_request(rq) && > llist_add(&rq->signal_node, &b->signaled_requests)) > irq_work_queue(&b->irq_work); > diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c > index 7ea94d201fe6..8015964043eb 100644 > --- a/drivers/gpu/drm/i915/gt/intel_timeline.c > +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c > @@ -126,6 +126,10 @@ static void __rcu_cacheline_free(struct rcu_head *rcu) > struct intel_timeline_cacheline *cl = > container_of(rcu, typeof(*cl), rcu); > > + /* Must wait until after all *rq->hwsp are complete before removing */ > + i915_gem_object_unpin_map(cl->hwsp->vma->obj); > + __idle_hwsp_free(cl->hwsp, ptr_unmask_bits(cl->vaddr, CACHELINE_BITS)); > + > i915_active_fini(&cl->active); > kfree(cl); > } > @@ -133,11 +137,6 @@ static void __rcu_cacheline_free(struct rcu_head *rcu) > static void __idle_cacheline_free(struct intel_timeline_cacheline *cl) > { > GEM_BUG_ON(!i915_active_is_idle(&cl->active)); > - > - i915_gem_object_unpin_map(cl->hwsp->vma->obj); > - i915_vma_put(cl->hwsp->vma); > - __idle_hwsp_free(cl->hwsp, ptr_unmask_bits(cl->vaddr, CACHELINE_BITS)); > - > call_rcu(&cl->rcu, __rcu_cacheline_free); > } > > @@ -179,7 +178,6 @@ cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline) > return ERR_CAST(vaddr); > } > > - i915_vma_get(hwsp->vma); > cl->hwsp = hwsp; > cl->vaddr = page_pack_bits(vaddr, cacheline); > > diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h > index 620b6fab2c5c..92adfee30c7c 100644 > --- a/drivers/gpu/drm/i915/i915_request.h > +++ b/drivers/gpu/drm/i915/i915_request.h > @@ -434,7 +434,7 @@ static inline u32 hwsp_seqno(const struct i915_request *rq) > > static inline bool __i915_request_has_started(const struct i915_request *rq) > { > - return i915_seqno_passed(hwsp_seqno(rq), rq->fence.seqno - 1); > + return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno - 1); > } > > /** > @@ -465,11 +465,19 @@ static inline bool __i915_request_has_started(const struct i915_request *rq) > */ > static inline bool i915_request_started(const struct i915_request *rq) > { > + bool result; > + > if (i915_request_signaled(rq)) > return true; > > - /* Remember: started but may have since been preempted! */ > - return __i915_request_has_started(rq); > + result = true; > + rcu_read_lock(); /* the HWSP may be freed at runtime */ > + if (likely(!i915_request_signaled(rq))) > + /* Remember: started but may have since been preempted! */ > + result = __i915_request_has_started(rq); > + rcu_read_unlock(); > + > + return result; > } > > /** > @@ -482,10 +490,16 @@ static inline bool i915_request_started(const struct i915_request *rq) > */ > static inline bool i915_request_is_running(const struct i915_request *rq) > { > + bool result; > + > if (!i915_request_is_active(rq)) > return false; > > - return __i915_request_has_started(rq); > + rcu_read_lock(); > + result = __i915_request_has_started(rq) && i915_request_is_active(rq); > + rcu_read_unlock(); > + > + return result; > } > > /** > @@ -509,12 +523,25 @@ static inline bool i915_request_is_ready(const struct i915_request *rq) > return !list_empty(&rq->sched.link); > } > > +static inline bool __i915_request_is_complete(const struct i915_request *rq) > +{ > + return i915_seqno_passed(__hwsp_seqno(rq), rq->fence.seqno); > +} > + > static inline bool i915_request_completed(const struct i915_request *rq) > { > + bool result; > + > if (i915_request_signaled(rq)) > return true; > > - return i915_seqno_passed(hwsp_seqno(rq), rq->fence.seqno); > + result = true; > + rcu_read_lock(); /* the HWSP may be freed at runtime */ > + if (likely(!i915_request_signaled(rq))) > + result = __i915_request_is_complete(rq); > + rcu_read_unlock(); > + > + return result; > } > > static inline void i915_request_mark_complete(struct i915_request *rq) -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx