Re: [PATCH v1 3/7] drm/xe/vram: Make VRAM pools the allocation handle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xe: NULL pointer dereference of tile->mem.vram on integrated graphics
--

commit c154aad6efc21416255e0f4ca071bf1270bc02d1
Author: Piotr Piórkowski <piotr.piorkowski@intel.com>
drm/xe/vram: Make VRAM pools the allocation handle

This commit changes allocation paths to use a struct xe_vram_pool rather
than a struct xe_vram_region. This allows regions to be split into several
pools without affecting every consumer. BO creation, migration, and page
fault paths are updated to interact with the new pool abstractions.

> diff --git a/drivers/gpu/drm/xe/xe_assert.h b/drivers/gpu/drm/xe/xe_assert.h
> --- a/drivers/gpu/drm/xe/xe_assert.h
> +++ b/drivers/gpu/drm/xe/xe_assert.h
[ ... ]
> @@ -147,7 +147,8 @@
>  	char __buf[10] __maybe_unused;								\
>  	xe_assert_msg(tile_to_xe(__tile), condition, "tile: %u VRAM %s\n" msg,			\
>  		      __tile->id, ({ string_get_size(						\
> -				     xe_vram_region_actual_physical_size(__tile->mem.vram), 1,	\
> +				     xe_vram_region_actual_physical_size(			\
> +					__tile->mem.vram->region), 1,				\
>  				     STRING_UNITS_2, __buf, sizeof(__buf)); __buf; }), ## arg);	\
>  })

[Severity: High]
Is it possible for __tile->mem.vram to be NULL here on integrated graphics?

On systems without dedicated graphics, xe_tile_alloc_vram() leaves 
tile->mem.vram as NULL. If an assertion triggers, evaluating 
__tile->mem.vram->region while formatting the message might cause a kernel
panic.

> diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> --- a/drivers/gpu/drm/xe/xe_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_debugfs.c
[ ... ]
> @@ -99,7 +99,7 @@ static ssize_t inject_mempage_offline_trigger(struct file *f,
>  {
>  	struct xe_device *xe = file_inode(f)->i_private;
>  	struct xe_tile *tile = xe_device_get_root_tile(xe);
> -	struct xe_vram_region *vr = tile->mem.vram;
> +	struct xe_vram_region *vr = tile->mem.vram->region;
>  	u64 pfn;
>  	int ret;

[Severity: High]
Could this early dereference cause a panic on integrated graphics?

If tile->mem.vram is NULL, evaluating tile->mem.vram->region before the
subsequent !vr check below will fault when accessing this debugfs node.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
[ ... ]
> @@ -201,7 +201,7 @@ static int xe_pagefault_handle_vma(struct xe_gt *gt, struct xe_vma *vma,
>  		xe_validation_ctx_init(&ctx, &vm->xe->val, &exec,
>  				       (struct xe_val_flags) {});
>  		drm_exec_until_all_locked(&exec) {
> -			err = xe_pagefault_begin(&exec, vma, tile->mem.vram,
> +			err = xe_pagefault_begin(&exec, vma, tile->mem.vram->region,
>  						 needs_vram == 1);
>  			drm_exec_retry_on_contention(&exec);
>  			xe_validation_retry_on_oom(&ctx, &err);

[Severity: High]
Does tile->mem.vram need a NULL check before being dereferenced?

During a GPU page fault on an integrated graphics system, tile->mem.vram is
expected to be NULL. Evaluating tile->mem.vram->region directly here to pass
into xe_pagefault_begin() could lead to a NULL pointer dereference.

> diff --git a/drivers/gpu/drm/xe/xe_tile.h b/drivers/gpu/drm/xe/xe_tile.h
> --- a/drivers/gpu/drm/xe/xe_tile.h
> +++ b/drivers/gpu/drm/xe/xe_tile.h
[ ... ]
> @@ -29,11 +30,12 @@ static inline bool xe_tile_is_root(struct xe_tile *tile)
>   * struct xe_tile pointer
>   * @tile: Pointer to the struct xe_tile.
>   *
> - * Return: Pointer to the struct xe_vram_region embedded in *@tile.
> + * Return: Pointer to the struct xe_vram_region backing *@tile's general
> + * purpose VRAM pool.
>   */
>  static inline struct xe_vram_region *xe_tile_to_vr(struct xe_tile *tile)
>  {
> -	return tile->mem.vram;
> +	return tile->mem.vram->region;
>  }

[Severity: High]
Will this unconditionally dereference tile->mem.vram?

Previously this function just returned the pointer, which could safely be
NULL if the device had no VRAM pool. Extracting the region from it might now
panic on integrated graphics if called.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002104018.3648425-1-piotr.piorkowski@intel.com?part=3