linux-kernel - Re: [PATCH v1 1/2] nouveau/svm: fix missing folio unlock + put after make_device_exclusive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <oy3q6xbgqfzk53bitgtefaycsya4wwcxt5gp46mhnwh4qpl7fh@74yluy6whpb3>
Date: Wed, 12 Feb 2025 12:27:24 +1100
From: Alistair Popple <apopple@...dia.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	nouveau@...ts.freedesktop.org, Karol Herbst <kherbst@...hat.com>, Lyude Paul <lyude@...hat.com>, 
	Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>
Subject: Re: [PATCH v1 1/2] nouveau/svm: fix missing folio unlock + put after
 make_device_exclusive_range()

On Fri, Jan 24, 2025 at 07:15:23PM +0100, David Hildenbrand wrote:
> In case we have to retry the loop, we are missing to unlock+put the
> folio. In that case, we will keep failing make_device_exclusive_range()
> because we cannot grab the folio lock, and even return from the function
> with the folio locked and referenced, effectively never succeeding the
> make_device_exclusive_range().
> 
> While at it, convert the other unlock+put to use a folio as well.
> 
> This was found by code inspection.

I couldn't (easily at least) trigger this condition from userspace, probably
because the window is pretty tight between mmu_interval_read_begin() and
mmu_interval_read_retry(). But I tested it by manually forcing at least one
retry and it fixes the issue observed via inspection. There's no reason to think
it would be entirely impossible to hit either, and the change looks good so
please add:

Reviewed-by: Alistair Popple <apopple@...dia.com>
Tested-by: Alistair Popple <apopple@...dia.com>

> Fixes: 8f187163eb89 ("nouveau/svm: implement atomic SVM access")
> Signed-off-by: David Hildenbrand <david@...hat.com>
> ---
>  drivers/gpu/drm/nouveau/nouveau_svm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index b4da82ddbb6b2..8ea98f06d39af 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -590,6 +590,7 @@ static int nouveau_atomic_range_fault(struct nouveau_svmm *svmm,
>  	unsigned long timeout =
>  		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>  	struct mm_struct *mm = svmm->notifier.mm;
> +	struct folio *folio;
>  	struct page *page;
>  	unsigned long start = args->p.addr;
>  	unsigned long notifier_seq;
> @@ -616,12 +617,16 @@ static int nouveau_atomic_range_fault(struct nouveau_svmm *svmm,
>  			ret = -EINVAL;
>  			goto out;
>  		}
> +		folio = page_folio(page);
>  
>  		mutex_lock(&svmm->mutex);
>  		if (!mmu_interval_read_retry(&notifier->notifier,
>  					     notifier_seq))
>  			break;
>  		mutex_unlock(&svmm->mutex);
> +
> +		folio_unlock(folio);
> +		folio_put(folio);
>  	}
>  
>  	/* Map the page on the GPU. */
> @@ -637,8 +642,8 @@ static int nouveau_atomic_range_fault(struct nouveau_svmm *svmm,
>  	ret = nvif_object_ioctl(&svmm->vmm->vmm.object, args, size, NULL);
>  	mutex_unlock(&svmm->mutex);
>  
> -	unlock_page(page);
> -	put_page(page);
> +	folio_unlock(folio);
> +	folio_put(folio);
>  
>  out:
>  	mmu_interval_notifier_remove(&notifier->notifier);
> -- 
> 2.47.1
>