Re: [PATCH 4/5] drm/nouveau/gsp/r570: Start saving comptag backing stores

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] The error check `if (cbc_size < 0)` is always false because `cbc_size` is an unsigned `u64`. When `r570_fb_get_compbit_store_size()` fails, it returns a negative error code that is implicitly cast to a large positive `u64`, bypassing the error check and causing an integer wrap-around.
- [Low] Mismatched GSP object handles used for API allocation and cleanup in `r570_fb_get_compbit_store_size()`.

Pre-existing issues:
- [High] Missing cleanup of suspended channels on error paths in `r570_fbsr_suspend()`.
--

commit caaa903dee4578bf9a609b4561ba8eb17ac6e144
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r570: Start saving comptag backing stores

This commit implements support for saving and restoring comptag backing stores
in OpenRM's fbsr process by fetching the required space and allocating it.
The goal is to safely enable bEnteringGcOff in fbsr.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> index 7f5aae0c055b8..58f426ba81191 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> @@ -26,6 +26,24 @@ r570_fbsr_suspend_channels(struct nvkm_gsp *gsp, bool suspend)
>  	return nvkm_gsp_rm_ctrl_wr(&gsp->internal.device.subdevice, ctrl);
>  }
>  
> +static u64
> +r570_fb_get_compbit_store_size(struct nvkm_gsp *gsp)
> +{
> +	NV0080_CTRL_FB_GET_COMPBIT_STORE_INFO_PARAMS *ctrl;
> +	u64 size;
> +
> +	ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.object,
> +				   NV0080_CTRL_CMD_FB_GET_COMPBIT_STORE_INFO,
> +				   sizeof(*ctrl));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);

[Severity: Critical]
Since r570_fb_get_compbit_store_size() returns a u64, does this implicitly
cast the negative signed error code (like -ENOMEM) into a huge positive value?

> +
> +	size = ctrl->Size;
> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.device.subdevice, ctrl);

[Severity: Low]
Does this violate API symmetry by allocating the control struct using
&gsp->internal.device.object but cleaning it up with
&gsp->internal.device.subdevice?

> +	return size;
> +}
> +
>  static int
>  r570_memsys_enable_raw_comp_mode(struct nvkm_gsp *gsp, bool enable)
>  {

[ ... ]

> @@ -151,6 +169,11 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path fail to
undo r570_fbsr_suspend_channels(gsp, true) called earlier in the function?

If the suspend fails here, it seems the channels are left permanently
suspended, making the GPU unusable until a reboot.

>  	}
>  
> +	cbc_size = r570_fb_get_compbit_store_size(gsp);
> +	if (cbc_size < 0)
> +		return cbc_size;

[Severity: Critical]
Is this error check dead code? Since cbc_size is an unsigned u64, it can
never be less than 0.

If r570_fb_get_compbit_store_size() fails, the implicitly cast large positive
error code will bypass this check, leading to an integer wrap-around when
added to size later in the function.

Could this cause an undersized memory allocation that results in memory
corruption when the firmware attempts to write the FBSR state out of bounds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911222428.752641-1-lyude@redhat.com?part=4