Re: [PATCH v2] drm/nouveau/svm: drain fault handler before freeing svmm

Reviewed-by: Lyude Paul <[email protected]>

Will push to drm-misc-next in just a moment

On Fri, 2026-08-14 at 18:00 +0800, Zhenhao Wan wrote:
> The SVM fault handler nouveau_svm_fault() looks up each fault's
> nouveau_svmm under svm->mutex, caches it in the fault array, then
> drops
> svm->mutex and dereferences the svmm across blocking faults
> (mmu_interval_notifier_insert(), hmm_range_fault()).
> 
> nouveau_svmm is not refcounted. On DRM file close nouveau_svmm_fini()
> ->
> mmu_notifier_put() frees it asynchronously via call_srcu() without
> waiting for the handler, and mmget_not_zero() pins the mm, not the
> svmm.
> flush_work() on the fault buffer otherwise runs only at device
> removal
> and suspend, never on close, so a fault handled concurrently with a
> close
> can dereference a freed svmm.
> 
> The instance is already unlinked by nouveau_svmm_part() earlier in
> the
> same close, so no new fault can resolve to it. Drain the handler in
> nouveau_svmm_fini() before the free, guarding the flush with a NULL
> check
> on drm->svm: on device teardown nouveau_svm_fini() runs first and
> frees
> drm->svm (NULL) after blocking the notify and flushing the buffer, so
> the
> handler is already drained; on per-client close drm->svm is alive and
> the
> flush runs.
> 
> Fixes: eeaf06ac1a55 ("drm/nouveau/svm: initial support for shared
> virtual memory")
> Reported-by: Yuhao Jiang <[email protected]>
> Assisted-by: Claude:claude-opus-5
> Cc: [email protected]
> Signed-off-by: Zhenhao Wan <[email protected]>
> ---
> Changes in v2:
> - Fix a NULL deref introduced in v1: guard flush_work() with a NULL
>   check on drm->svm.  On device teardown nouveau_svm_fini() frees
>   drm->svm before the per-client svmm teardown, so
> nouveau_svmm_fini()
>   would deref NULL (reported by Sashiko AI review); the handler is
>   already drained there, so there is nothing left to flush.
> - Tighten the commit message and the code comment; no functional
> change.
> - Link to v1:
> https://patch.msgid.link/[email protected]
> 
> To: Lyude Paul <[email protected]>
> To: Danilo Krummrich <[email protected]>
> To: Maarten Lankhorst <[email protected]>
> To: Maxime Ripard <[email protected]>
> To: Thomas Zimmermann <[email protected]>
> To: David Airlie <[email protected]>
> To: Simona Vetter <[email protected]>
> To: Ben Skeggs <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
>  drivers/gpu/drm/nouveau/nouveau_svm.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
> b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index dcc92131488e..a87406d3da30 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -304,6 +304,19 @@ nouveau_svmm_fini(struct nouveau_svmm **psvmm)
>  {
>  	struct nouveau_svmm *svmm = *psvmm;
>  	if (svmm) {
> +		struct nouveau_svm *svm = svmm->vmm->cli->drm->svm;
> +
> +		/* The fault handler caches svmm pointers under svm-
> >mutex and
> +		 * dereferences them after dropping it, across
> blocking faults.
> +		 * The instance is already unlinked
> (nouveau_svmm_part), so drain
> +		 * the handler before the free to release any in-
> flight reference.
> +		 *
> +		 * On device teardown nouveau_svm_fini() ran first
> and freed
> +		 * drm->svm (NULL) after draining the buffer;
> nothing to flush.
> +		 */
> +		if (svm)
> +			flush_work(&svm->buffer[0].work);
> +
>  		mutex_lock(&svmm->mutex);
>  		svmm->vmm = NULL;
>  		mutex_unlock(&svmm->mutex);
> 
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> change-id: 20260813-nouveau-svm-svmm-uaf-bbcf36265aaa
> 
> Best regards,
> --  
> Zhenhao Wan <[email protected]>