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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id E882EC7EE24 for ; Mon, 5 Jun 2023 20:27:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1D49810E033; Mon, 5 Jun 2023 20:27:26 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id B049210E033; Mon, 5 Jun 2023 20:27:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1685996844; x=1717532844; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=v6/gD3wPqPZqTdNTBUt1XC181r0nVOwiVrgdCqBtGJY=; b=Xvd73TygVyFV5qEqf/wnuVuR9g8Niw0a92SwAgtP30NnJ5iLQeolaFeG 8ODzG8YONGUPgKgsdK0pPklVj9DE1p2HwbHf73Jg583/78yeB9E9vnelk XRXGaz1fg26xk9hvOeotCUbs/4A93kzo477XhurtELTSAXY3L4vkeT/QG 733pGebZ4ZPgeI2dPr3CMXVvhdZilzDZsgUtpJVxLpCP+4ylX3WC5Xd4b j9SJ+Yv2t8CEf8GhPbdc1lu90O/OkSEDZXD9aaHJ0PWj4cqGszKQrBVK7 /QU7s7wUu3uwQhgvXnz4GMAd2/OaHCIf5BKvFSMnJcdiGCGISyQ2LHQZF g==; X-IronPort-AV: E=McAfee;i="6600,9927,10732"; a="442851000" X-IronPort-AV: E=Sophos;i="6.00,218,1681196400"; d="scan'208";a="442851000" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Jun 2023 13:27:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10732"; a="853111265" X-IronPort-AV: E=Sophos;i="6.00,218,1681196400"; d="scan'208";a="853111265" Received: from twgeistx-mobl.ger.corp.intel.com (HELO intel.com) ([10.249.42.176]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Jun 2023 13:27:21 -0700 Date: Mon, 5 Jun 2023 22:27:16 +0200 From: Andi Shyti To: Nirmoy Das Message-ID: References: <20230605201021.13928-1-nirmoy.das@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230605201021.13928-1-nirmoy.das@intel.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix a VMA UAF for multi-gt platform 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: Andrzej Hajda , Thomas =?iso-8859-15?Q?Hellstr=F6m?= , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Chris Wilson , Rodrigo Vivi Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hi Nirmoy, On Mon, Jun 05, 2023 at 10:10:21PM +0200, Nirmoy Das wrote: > Ensure correct handling of closed VMAs on multi-gt platforms to prevent > Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are > exclusively added to GT0's closed_vma link (gt->closed_vma) and > subsequently freed by i915_vma_parked(), which assumes the entire GPU is > idle. However, on platforms with multiple GTs, such as MTL, GT1 may > remain active while GT0 is idle. This causes GT0 to mistakenly consider > the closed VMAs in its closed_vma list as unnecessary, potentially > leading to Use-After-Free issues if a job for GT1 attempts to access a > freed VMA. > > Although we do take a wakeref for GT0 but it happens later, after > evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref > early. hooray! this is great, Nirmoy! I will give it a shot. > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tvrtko Ursulin > Cc: Thomas Hellström > Cc: Chris Wilson > Cc: Andi Shyti > Cc: Andrzej Hajda > Signed-off-by: Nirmoy Das > --- > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 5fb459ea4294..adcf8837dfe6 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -2692,6 +2692,7 @@ static int > eb_select_engine(struct i915_execbuffer *eb) > { > struct intel_context *ce, *child; > + struct intel_gt *gt; > unsigned int idx; > int err; > > @@ -2715,10 +2716,16 @@ eb_select_engine(struct i915_execbuffer *eb) > } > } > eb->num_batches = ce->parallel.number_children + 1; > + gt = ce->engine->gt; > > for_each_child(ce, child) > intel_context_get(child); > intel_gt_pm_get(ce->engine->gt); > + /* Keep GT0 active on MTL so that i915_vma_parked() doesn't > + * free VMAs while execbuf ioctl is validating VMAs. > + */ > + if (gt != to_gt(gt->i915)) you can use gt->info.id > + intel_gt_pm_get(to_gt(ce->engine->gt->i915)); > > if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) { > err = intel_context_alloc_state(ce); > @@ -2757,6 +2764,9 @@ eb_select_engine(struct i915_execbuffer *eb) > return err; > > err: > + if (ce->engine->gt != to_gt(ce->engine->gt->i915)) if (gt->info.id) gt is already ce->engine->gt > + intel_gt_pm_get(to_gt(ce->engine->gt->i915)); > + > intel_gt_pm_put(ce->engine->gt); > for_each_child(ce, child) > intel_context_put(child); > @@ -2770,6 +2780,8 @@ eb_put_engine(struct i915_execbuffer *eb) > struct intel_context *child; > > i915_vm_put(eb->context->vm); > + if (eb->gt != to_gt(eb->gt->i915)) > + intel_gt_pm_put(to_gt(eb->gt->i915)); this wakeref going up and down is a bit ugly... Perhaps we can add some flag about the GT type in the info structure. MTL is a weird multi-gt platform and, indeed, you can't shut down GT0 without affecting GT1. For now it's OK, though, as to test it. Andi > intel_gt_pm_put(eb->gt); > for_each_child(eb->context, child) > intel_context_put(child); > -- > 2.39.0