[PATCH] drm/xe/userptr: properly setup pfn_flags_mask

[PATCH] drm/xe/userptr: properly setup pfn_flags_mask

[Matthew Auld]

Currently we just leave it uninitialised, which at first looks harmless,
however we also don't zero out the pfn array, and with pfn_flags_mask
the idea is to be able set individual flags for a given range of pfn or
completely ignore them, outside of default_flags. So here we end up with
pfn[i] & pfn_flags_mask, and if both are uninitialised we might get back
an unexpected flags value, like asking for read only with default_flags,
but getting back write on top, leading to potentially bogus behaviour.

To fix this ensure we zero the pfn_flags_mask, such that hmm only
considers the default_flags and not also the initial pfn[i] value.

Fixes: 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@intel.com>
Cc: <stable@vger.kernel.org> # v6.10+
---
 drivers/gpu/drm/xe/xe_hmm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/xe/xe_hmm.c b/drivers/gpu/drm/xe/xe_hmm.c
index 089834467880..8c3cd65fa4b3 100644
--- a/drivers/gpu/drm/xe/xe_hmm.c
+++ b/drivers/gpu/drm/xe/xe_hmm.c
@@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct xe_userptr_vma *uvma,
 		goto free_pfns;
 	}
 
+	hmm_range.pfn_flags_mask = 0;
 	hmm_range.default_flags = flags;
 	hmm_range.hmm_pfns = pfns;
 	hmm_range.notifier = &userptr->notifier;
-- 
2.48.1

Re: [PATCH] drm/xe/userptr: properly setup pfn_flags_mask

[Hellstrom, Thomas]

On Wed, 2025-02-26 at 17:17 +0000, Matthew Auld wrote:
> Currently we just leave it uninitialised, which at first looks
> harmless,
> however we also don't zero out the pfn array, and with pfn_flags_mask
> the idea is to be able set individual flags for a given range of pfn
> or
> completely ignore them, outside of default_flags. So here we end up
> with
> pfn[i] & pfn_flags_mask, and if both are uninitialised we might get
> back
> an unexpected flags value, like asking for read only with
> default_flags,
> but getting back write on top, leading to potentially bogus
> behaviour.
> 
> To fix this ensure we zero the pfn_flags_mask, such that hmm only
> considers the default_flags and not also the initial pfn[i] value.
> 
> Fixes: 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@intel.com>
> Cc: <stable@vger.kernel.org> # v6.10+
> ---
>  drivers/gpu/drm/xe/xe_hmm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hmm.c
> b/drivers/gpu/drm/xe/xe_hmm.c
> index 089834467880..8c3cd65fa4b3 100644
> --- a/drivers/gpu/drm/xe/xe_hmm.c
> +++ b/drivers/gpu/drm/xe/xe_hmm.c
> @@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct
> xe_userptr_vma *uvma,
>  		goto free_pfns;
>  	}
>  
> +	hmm_range.pfn_flags_mask = 0;
>  	hmm_range.default_flags = flags;
>  	hmm_range.hmm_pfns = pfns;
>  	hmm_range.notifier = &userptr->notifier;

Is there a chance we can modify the function to have an initializer for
hmm_range

struct hmm_range hmm_range = {...

That'd also make adding fields to struct hmm_range less fragile.

Either way,
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

RE: [PATCH] drm/xe/userptr: properly setup pfn_flags_mask

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> Matthew Auld
> Sent: Wednesday, February 26, 2025 10:47 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost@intel.com>; Hellstrom, Thomas
> <thomas.hellstrom@intel.com>; stable@vger.kernel.org
> Subject: [PATCH] drm/xe/userptr: properly setup pfn_flags_mask
> 
> Currently we just leave it uninitialised, which at first looks harmless, however
> we also don't zero out the pfn array, and with pfn_flags_mask the idea is to be
> able set individual flags for a given range of pfn or completely ignore them,
> outside of default_flags. So here we end up with pfn[i] & pfn_flags_mask, and
> if both are uninitialised we might get back an unexpected flags value, like
> asking for read only with default_flags, but getting back write on top, leading
> to potentially bogus behaviour.
> 
> To fix this ensure we zero the pfn_flags_mask, such that hmm only considers
> the default_flags and not also the initial pfn[i] value.
> 
> Fixes: 81e058a3e7fd ("drm/xe: Introduce helper to populate userptr")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@intel.com>
> Cc: <stable@vger.kernel.org> # v6.10+
> ---
>  drivers/gpu/drm/xe/xe_hmm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hmm.c b/drivers/gpu/drm/xe/xe_hmm.c
> index 089834467880..8c3cd65fa4b3 100644
> --- a/drivers/gpu/drm/xe/xe_hmm.c
> +++ b/drivers/gpu/drm/xe/xe_hmm.c
> @@ -206,6 +206,7 @@ int xe_hmm_userptr_populate_range(struct
> xe_userptr_vma *uvma,
>  		goto free_pfns;
>  	}
> 
> +	hmm_range.pfn_flags_mask = 0;

Looks correct to me,
Reviewed-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

Tejas
>  	hmm_range.default_flags = flags;
>  	hmm_range.hmm_pfns = pfns;
>  	hmm_range.notifier = &userptr->notifier;
> --
> 2.48.1