Re: [PATCH 1/2] drm/xe/ras: Fix error handling in register_nodes()

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Apr 07, 2026 at 10:59:12PM +0000, Shuicheng Lin wrote:
> Fix two issues in register_nodes():
> 
> 1. When the loop fails mid-way, previously registered nodes are not
>    cleaned up. Add goto-based error unwinding that walks backwards
>    through completed iterations.

Did you check with the author if this is expected behaviour?

> 2. When allocate_and_copy_counters() fails, assign_node_params()
>    leaves ras->info[severity] as an ERR_PTR and returns. The caller
>    then passes that ERR_PTR to kfree() via cleanup_node_param(),
>    causing an invalid free. Fix by making assign_node_params()
>    self-contained on error: NULL out the stale ERR_PTR and free
>    device_name before returning.

Can this be rather fixed using a local pointer?

Raag

> Fixes: b40db12b542f ("drm/xe/xe_drm_ras: Add support for XE DRM RAS")
> Cc: Riana Tauro <riana.tauro@intel.com>
> Assisted-by: Claude:claude-opus-4.6
> Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_drm_ras.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_drm_ras.c b/drivers/gpu/drm/xe/xe_drm_ras.c
> index e07dc23a155e..802e4bcb731c 100644
> --- a/drivers/gpu/drm/xe/xe_drm_ras.c
> +++ b/drivers/gpu/drm/xe/xe_drm_ras.c
> @@ -73,6 +73,7 @@ static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	struct xe_drm_ras *ras = &xe->ras;
>  	const char *device_name;
> +	int ret;
>  
>  	device_name = kasprintf(GFP_KERNEL, "%04x:%02x:%02x.%d",
>  				pci_domain_nr(pdev->bus), pdev->bus->number,
> @@ -89,8 +90,11 @@ static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
>  	node->priv = xe;
>  
>  	ras->info[severity] = allocate_and_copy_counters(xe);
> -	if (IS_ERR(ras->info[severity]))
> -		return PTR_ERR(ras->info[severity]);
> +	if (IS_ERR(ras->info[severity])) {
> +		ret = PTR_ERR(ras->info[severity]);
> +		ras->info[severity] = NULL;
> +		goto err_free_name;
> +	}
>  
>  	if (severity == DRM_XE_RAS_ERR_SEV_CORRECTABLE)
>  		node->query_error_counter = query_correctable_error_counter;
> @@ -98,6 +102,11 @@ static int assign_node_params(struct xe_device *xe, struct drm_ras_node *node,
>  		node->query_error_counter = query_uncorrectable_error_counter;
>  
>  	return 0;
> +
> +err_free_name:
> +	kfree(device_name);
> +	node->device_name = NULL;
> +	return ret;
>  }
>  
>  static void cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_error_severity severity)
> @@ -114,26 +123,30 @@ static void cleanup_node_param(struct xe_drm_ras *ras, const enum drm_xe_ras_err
>  static int register_nodes(struct xe_device *xe)
>  {
>  	struct xe_drm_ras *ras = &xe->ras;
> -	int i;
> +	int i, ret;
>  
>  	for_each_error_severity(i) {
>  		struct drm_ras_node *node = &ras->node[i];
> -		int ret;
>  
>  		ret = assign_node_params(xe, node, i);
> -		if (ret) {
> -			cleanup_node_param(ras, i);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_unwind;
>  
>  		ret = drm_ras_node_register(node);
>  		if (ret) {
>  			cleanup_node_param(ras, i);
> -			return ret;
> +			goto err_unwind;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_unwind:
> +	while (i--) {
> +		drm_ras_node_unregister(&ras->node[i]);
> +		cleanup_node_param(ras, i);
> +	}
> +	return ret;
>  }
>  
>  static void xe_drm_ras_unregister_nodes(struct drm_device *device, void *arg)
> -- 
> 2.43.0
>