Date: Mon, 23 Sep 2024 16:08:08 +0000
From: Matthew Brost <matthew.brost@intel.com>
To: Matthew Auld <matthew.auld@intel.com>
CC: <intel-xe@lists.freedesktop.org>, <stable@vger.kernel.org>
Subject: Re: [PATCH v2] drm/xe: fix UAF around queue destruction
Message-ID: <ZvGSaJACPAOFHrF5@DUT025-TGLU.fm.intel.com>
References: <20240923145647.77707-2-matthew.auld@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240923145647.77707-2-matthew.auld@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6wNY4r/kqOv4m4IDqT2LXsfCWGA+5tS2q+c4gpA723DimXuCUuvQkVJoTuGy?=
 =?us-ascii?Q?p8vWV6JdayQoC9YwbM0V7ReDOsODNeM8NwwfWC/NDjMJtLcpnjzA96wpgGkd?=
 =?us-ascii?Q?xqVNde+rrQuvW0e4WYlQPEFesZZ7MO0L6/XRwmMXCc0r8CLONbKymJRMscBB?=
 =?us-ascii?Q?0SzYpSSxgbVbd/gY6Dcl/guAx4Q0mTQKugKBReoVZPG5TSfFyhj6vEK9Gk/7?=
 =?us-ascii?Q?Tno5ZCwWOQSWaxIRWlz6HvOApH95nIIlrB+qk8rBBXoPm6Tnkyczx34N1WFA?=
 =?us-ascii?Q?RITC1zRmn2M0wQDOsY3QzXSsaRAQdfOI+5WjyBmQwkvM1NqBkBWSqbMPEpXX?=
 =?us-ascii?Q?xljrWO6xIGB4TyLPckDUJYP2XWkYVSVxJi1OztP3l6v+eu6xssUVu4qPhbgO?=
 =?us-ascii?Q?ys7houy0X5dhQX0+6QrZsNYB6iws5LD/seCglSyvGxY1VuYeQn+u9TqaJiTW?=
 =?us-ascii?Q?MQ6mngjaBPTXWh4cRsBcXksc8rE92yidqXIs1JFa1a7y+JLEUuGtVK+0TwwA?=
 =?us-ascii?Q?iOT4f0G+Ct7smOxb5zQmy8Z7H/g3tP0F8Tsu7L5F6JmHNAEsZXpED12Pr814?=
 =?us-ascii?Q?6Ud5N3vuk1FoWvS+ev56wH55g+m6qWrPHSVlfQJBYKIxL2aPA+Nq2SICGi7E?=
 =?us-ascii?Q?GNyjv1b+bFT6eladm0ck3kLBjdyczaeD6zPpFkMR3d/eCzL4VyjDEVI2hU4n?=
 =?us-ascii?Q?NBK48AWk6QJG0dgI9yRSZQjR1aW8eYAJ09zfZC8SRE2Y5PUWcCvCg3PNVaUc?=
 =?us-ascii?Q?1+68msX5stQEQD6yuuh7arvldXxwE4AE66uYNyI/lrFGHXzLS5ADNGTVLMzF?=
 =?us-ascii?Q?gxkTzT5yhQQQbLV5ZuCvGmXqe8e4GGjExQw62bZnI5kzJaTptGABWJ3KqcdV?=
 =?us-ascii?Q?R+0XMswATiLqaCEbFt/2biGxn75U1YkZnm4QauZ42sFIIMbfA9RSRdjjzR+l?=
 =?us-ascii?Q?6EKDT7k97s6b3L3mO4Ybw6XLqQhgJkKFXDLXFqMBVUpglfGNOaQkvjAaU7uT?=
 =?us-ascii?Q?9Bjf5gWvqhsB2pio7dyINWFByKfdaCiWucXGVpbclEQPAY8+UEkihUeFhmyt?=
 =?us-ascii?Q?laihxJchOy7RayucFNwMGODflIZ0k8R9zBWdNtjaQHRow/SXnH2Joe/5z8uB?=
 =?us-ascii?Q?Yy+9v4pMpis5PotZ91NonTodZSKyoYA0/jFpyts8GrMrkSEa4Jb8NprKTUpB?=
 =?us-ascii?Q?y6JL2yuI+4OxYdEjPJJ0uVOQmiM8sMhIkDXecFl7irVDTbpOE3XBufJgassF?=
 =?us-ascii?Q?0o5ow/KZw/OS6s75d+4VO4C3ezwBvTTG7Of7/imX6/TmTg34NLkFi20J8JBj?=
 =?us-ascii?Q?glfBIgMz37BvpUSk5UqMLxxp+Um0qHGnIKAKTQ9XU4oxkS4Q5L4hDWo+R/82?=
 =?us-ascii?Q?KaAafZAcI1oMEyY6NDHCLPPElmempiEy7/5GDDFYNvpyO1iL7q+BiYnzxnin?=
 =?us-ascii?Q?hsAsuopm2iqcRyQFdPHDQeIRwxkcpnUc5SAg2MJjoV450vCLwmlRasjTg/Ag?=
 =?us-ascii?Q?z9Mdj7lk8kXvpNGz9I5jDWq4pO1y5TPo1aR8fEoDPsuBvEdJxfEamffZWbxL?=
 =?us-ascii?Q?lQm7SKP5mhaqK5yNrqdRLy4pblW4J8+iwjKSfg38GitGP/hf69k5K91nhWb+?=
 =?us-ascii?Q?2w=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3e0ebb1c-1126-4dd6-2c39-08dcdbea2fdc
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB6522.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2024 16:10:05.2781
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LGeLBLuxbmKn0o8SCyKJkarq5CVTfuo6Yum7ieEbIc6SLGS2nIKmAbiEE9FOstTg6opPymgapz4T80i1ZrMWkQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6861
X-OriginatorOrg: intel.com

On Mon, Sep 23, 2024 at 03:56:48PM +0100, Matthew Auld wrote:
> We currently do stuff like queuing the final destruction step on a
> random system wq, which will outlive the driver instance. With bad
> timing we can teardown the driver with one or more work workqueue still
> being alive leading to various UAF splats. Add a fini step to ensure
> user queues are properly torn down. At this point GuC should already be
> nuked so queue itself should no longer be referenced from hw pov.
> 
> v2 (Matt B)
>  - Looks much safer to use a waitqueue and then just wait for the
>    xa_array to become empty before triggering the drain.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2317
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: <stable@vger.kernel.org> # v6.8+
> ---
>  drivers/gpu/drm/xe/xe_device.c       |  6 +++++-
>  drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>  drivers/gpu/drm/xe/xe_guc_submit.c   | 26 +++++++++++++++++++++++++-
>  drivers/gpu/drm/xe/xe_guc_types.h    |  2 ++
>  4 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index cb5a9fd820cf..90b3478ed7cd 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -297,6 +297,9 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
>  	if (xe->unordered_wq)
>  		destroy_workqueue(xe->unordered_wq);
>  
> +	if (xe->destroy_wq)
> +		destroy_workqueue(xe->destroy_wq);
> +
>  	ttm_device_fini(&xe->ttm);
>  }
>  
> @@ -360,8 +363,9 @@ struct xe_device *xe_device_create(struct pci_dev *pdev,
>  	xe->preempt_fence_wq = alloc_ordered_workqueue("xe-preempt-fence-wq", 0);
>  	xe->ordered_wq = alloc_ordered_workqueue("xe-ordered-wq", 0);
>  	xe->unordered_wq = alloc_workqueue("xe-unordered-wq", 0, 0);
> +	xe->destroy_wq = alloc_workqueue("xe-destroy-wq", 0, 0);
>  	if (!xe->ordered_wq || !xe->unordered_wq ||
> -	    !xe->preempt_fence_wq) {
> +	    !xe->preempt_fence_wq || !xe->destroy_wq) {
>  		/*
>  		 * Cleanup done in xe_device_destroy via
>  		 * drmm_add_action_or_reset register above
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 5ad96d283a71..515385b916cc 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -422,6 +422,9 @@ struct xe_device {
>  	/** @unordered_wq: used to serialize unordered work, mostly display */
>  	struct workqueue_struct *unordered_wq;
>  
> +	/** @destroy_wq: used to serialize user destroy work, like queue */
> +	struct workqueue_struct *destroy_wq;
> +
>  	/** @tiles: device tiles */
>  	struct xe_tile tiles[XE_MAX_TILES_PER_DEVICE];
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index fbbe6a487bbb..ae2f85cc2d08 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -276,10 +276,26 @@ static struct workqueue_struct *get_submit_wq(struct xe_guc *guc)
>  }
>  #endif
>  
> +static void xe_guc_submit_fini(struct xe_guc *guc)
> +{
> +	struct xe_device *xe = guc_to_xe(guc);
> +	struct xe_gt *gt = guc_to_gt(guc);
> +	int ret;
> +
> +	ret = wait_event_timeout(
> +		guc->submission_state.fini_wq,
> +		xa_empty(&guc->submission_state.exec_queue_lookup), HZ * 5);
> +
> +	drain_workqueue(xe->destroy_wq);
> +
> +	xe_gt_assert(gt, ret);
> +}
> +
>  static void guc_submit_fini(struct drm_device *drm, void *arg)
>  {
>  	struct xe_guc *guc = arg;
>  
> +	xe_guc_submit_fini(guc);
>  	xa_destroy(&guc->submission_state.exec_queue_lookup);
>  	free_submit_wq(guc);
>  }
> @@ -345,6 +361,8 @@ int xe_guc_submit_init(struct xe_guc *guc, unsigned int num_ids)
>  
>  	xa_init(&guc->submission_state.exec_queue_lookup);
>  
> +	init_waitqueue_head(&guc->submission_state.fini_wq);
> +
>  	primelockdep(guc);
>  
>  	return drmm_add_action_or_reset(&xe->drm, guc_submit_fini, guc);
> @@ -361,6 +379,9 @@ static void __release_guc_id(struct xe_guc *guc, struct xe_exec_queue *q, u32 xa
>  
>  	xe_guc_id_mgr_release_locked(&guc->submission_state.idm,
>  				     q->guc->id, q->width);
> +
> +	if (xa_empty(&guc->submission_state.exec_queue_lookup))
> +		wake_up(&guc->submission_state.fini_wq);
>  }
>  
>  static int alloc_guc_id(struct xe_guc *guc, struct xe_exec_queue *q)
> @@ -1268,13 +1289,16 @@ static void __guc_exec_queue_fini_async(struct work_struct *w)
>  
>  static void guc_exec_queue_fini_async(struct xe_exec_queue *q)
>  {
> +	struct xe_guc *guc = exec_queue_to_guc(q);
> +	struct xe_device *xe = guc_to_xe(guc);
> +
>  	INIT_WORK(&q->guc->fini_async, __guc_exec_queue_fini_async);
>  
>  	/* We must block on kernel engines so slabs are empty on driver unload */
>  	if (q->flags & EXEC_QUEUE_FLAG_PERMANENT || exec_queue_wedged(q))
>  		__guc_exec_queue_fini_async(&q->guc->fini_async);
>  	else
> -		queue_work(system_wq, &q->guc->fini_async);
> +		queue_work(xe->destroy_wq, &q->guc->fini_async);
>  }
>  
>  static void __guc_exec_queue_fini(struct xe_guc *guc, struct xe_exec_queue *q)
> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
> index 546ac6350a31..69046f698271 100644
> --- a/drivers/gpu/drm/xe/xe_guc_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
> @@ -81,6 +81,8 @@ struct xe_guc {
>  #endif
>  		/** @submission_state.enabled: submission is enabled */
>  		bool enabled;
> +		/** @submission_state.fini_wq: submit fini wait queue */
> +		wait_queue_head_t fini_wq;
>  	} submission_state;
>  	/** @hwconfig: Hardware config state */
>  	struct {
> -- 
> 2.46.1
>