Re: [PATCH] drm/xe/pf: Fix potential GGTT allocation leak

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Nov 04, 2024 at 03:49:01PM +0100, Michal Wajdeczko wrote:
> In unlikely event that we fail during sending the new VF GGTT
> configuration to the GuC, we will free only the GGTT node data
> struct but will miss to release the actual GGTT allocation.
> 
> This will later lead to list corruption, GGTT space leak and
> finally risking crash when unloading the driver:
> 
>  [ ] ... [drm] GT0: PF: Failed to provision VF1 with 1073741824 (1.00 GiB) GGTT (-EIO)
>  [ ] ... [drm] GT0: PF: VF1 provisioning remains at 0 (0 B) GGTT
> 
>  [ ] list_add corruption. next->prev should be prev (ffff88813cfcd628), but was 0000000000000000. (next=ffff88813cfe2028).
>  [ ] RIP: 0010:__list_add_valid_or_report+0x6b/0xb0
>  [ ] Call Trace:
>  [ ]  drm_mm_insert_node_in_range+0x2c0/0x4e0
>  [ ]  xe_ggtt_node_insert+0x46/0x70 [xe]
>  [ ]  pf_provision_vf_ggtt+0x7f5/0xa70 [xe]
>  [ ]  xe_gt_sriov_pf_config_set_ggtt+0x5e/0x770 [xe]
>  [ ]  ggtt_set+0x4b/0x70 [xe]
>  [ ]  simple_attr_write_xsigned.constprop.0.isra.0+0xb0/0x110
> 
>  [ ] ... [drm] GT0: PF: Failed to provision VF1 with 1073741824 (1.00 GiB) GGTT (-ENOSPC)
>  [ ] ... [drm] GT0: PF: VF1 provisioning remains at 0 (0 B) GGTT
> 
>  [ ] Oops: general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b7b: 0000 [#1] PREEMPT SMP NOPTI
>  [ ] RIP: 0010:drm_mm_remove_node+0x1b7/0x390
>  [ ] Call Trace:
>  [ ]  <TASK>
>  [ ]  ? die_addr+0x2e/0x80
>  [ ]  ? exc_general_protection+0x1a1/0x3e0
>  [ ]  ? asm_exc_general_protection+0x22/0x30
>  [ ]  ? drm_mm_remove_node+0x1b7/0x390
>  [ ]  ggtt_node_remove+0xa5/0xf0 [xe]
>  [ ]  xe_ggtt_node_remove+0x35/0x70 [xe]
>  [ ]  xe_ttm_bo_destroy+0x123/0x220 [xe]
>  [ ]  intel_user_framebuffer_destroy+0x44/0x70 [xe]
>  [ ]  intel_plane_destroy_state+0x3b/0xc0 [xe]
>  [ ]  drm_atomic_state_default_clear+0x1cd/0x2f0
>  [ ]  intel_atomic_state_clear+0x9/0x20 [xe]
>  [ ]  __drm_atomic_state_free+0x1d/0xb0
> 
> Fix that by using pf_release_ggtt() on the error path, which now
> works regardless if the node has GGTT allocation or not.
> 
> Fixes: 34e804220f69 ("drm/xe: Make xe_ggtt_node struct independent")
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index 062a0c2fd2cd..192643d63d22 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -395,6 +395,8 @@ static void pf_release_ggtt(struct xe_tile *tile, struct xe_ggtt_node *node)
>  		 * the xe_ggtt_clear() called by below xe_ggtt_remove_node().
>  		 */
>  		xe_ggtt_node_remove(node, false);
> +	} else {
> +		xe_ggtt_node_fini(node);
>  	}
>  }
>  
> @@ -450,7 +452,7 @@ static int pf_provision_vf_ggtt(struct xe_gt *gt, unsigned int vfid, u64 size)
>  	config->ggtt_region = node;
>  	return 0;
>  err:
> -	xe_ggtt_node_fini(node);
> +	pf_release_ggtt(tile, node);
>  	return err;
>  }
>  
> -- 
> 2.43.0
>