Re: [PATCH] drm/xe: Drop HW fence pointer to HW fence ctx

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 15 Aug 2024 12:35:22 -0700, Matthew Brost wrote:
>

Just a couple of nits below. Otherwise this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

> The HW fence ctx objects are not ref counted rather tied to the life of
> an LRC object. HW fences reference the HW fence ctx, HW fences can
> outlive LRCs thus resulting in UAF. Drop the  HW fence pointer to HW
> fence ctx rather just store was is needed directly in HW fence.

s/was/what/

>
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_fence.c       | 9 +++++----
>  drivers/gpu/drm/xe/xe_hw_fence_types.h | 7 +++++--
>  drivers/gpu/drm/xe/xe_trace.h          | 2 +-
>  3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> index 45a9789cf501..25c20eac9e3a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> @@ -148,20 +148,20 @@ static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence)
>  {
>	struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence);
>
> -	return dev_name(gt_to_xe(fence->ctx->gt)->drm.dev);
> +	return dev_name(fence->xe->drm.dev);
>  }
>
>  static const char *xe_hw_fence_get_timeline_name(struct dma_fence *dma_fence)
>  {
>	struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence);
>
> -	return fence->ctx->name;
> +	return fence->name;
>  }
>
>  static bool xe_hw_fence_signaled(struct dma_fence *dma_fence)
>  {
>	struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence);
> -	struct xe_device *xe = gt_to_xe(fence->ctx->gt);
> +	struct xe_device *xe = fence->xe;
>	u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32);
>
>	return dma_fence->error ||
> @@ -253,7 +253,8 @@ void xe_hw_fence_init(struct dma_fence *fence, struct xe_hw_fence_ctx *ctx,
>	struct  xe_hw_fence *hw_fence =
>		container_of(fence, typeof(*hw_fence), dma);
>
> -	hw_fence->ctx = ctx;
> +	hw_fence->xe = gt_to_xe(ctx->gt);
> +	sprintf(hw_fence->name, "%s", ctx->name);

snprintf? Limit to hw_fence->name size.

>	hw_fence->seqno_map = seqno_map;
>	INIT_LIST_HEAD(&hw_fence->irq_link);
>
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence_types.h b/drivers/gpu/drm/xe/xe_hw_fence_types.h
> index b33c4956e8ea..364a61f4bfda 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence_types.h
> +++ b/drivers/gpu/drm/xe/xe_hw_fence_types.h
> @@ -12,6 +12,7 @@
>  #include <linux/list.h>
>  #include <linux/spinlock.h>
>
> +struct xe_device;
>  struct xe_gt;
>
>  /**
> @@ -61,8 +62,10 @@ struct xe_hw_fence_ctx {
>  struct xe_hw_fence {
>	/** @dma: base dma fence for hardware fence context */
>	struct dma_fence dma;
> -	/** @ctx: hardware fence context */
> -	struct xe_hw_fence_ctx *ctx;
> +	/** @xe: Xe device for hw fence driver name */
> +	struct xe_device *xe;
> +	/** @name: name of hardware fence context */
> +	char name[MAX_FENCE_NAME_LEN];
>	/** @seqno_map: I/O map for seqno */
>	struct iosys_map seqno_map;
>	/** @irq_link: Link in struct xe_hw_fence_irq.pending */
> diff --git a/drivers/gpu/drm/xe/xe_trace.h b/drivers/gpu/drm/xe/xe_trace.h
> index 1abdb30cb7ad..8573d7a87d84 100644
> --- a/drivers/gpu/drm/xe/xe_trace.h
> +++ b/drivers/gpu/drm/xe/xe_trace.h
> @@ -309,7 +309,7 @@ DECLARE_EVENT_CLASS(xe_hw_fence,
>		    TP_ARGS(fence),
>
>		    TP_STRUCT__entry(
> -			     __string(dev, __dev_name_gt(fence->ctx->gt))
> +			     __string(dev, __dev_name_xe(fence->xe))
>			     __field(u64, ctx)
>			     __field(u32, seqno)
>			     __field(struct xe_hw_fence *, fence)
> --
> 2.34.1
>