Re: [PATCH] drm/mediatek: Init `ddp_comp` with devm_kcalloc()

[CK Hu (胡俊光)]

Hi, Douglas:

On Thu, 2024-03-28 at 09:22 -0700, Douglas Anderson wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  In the case where `conn_routes` is true we allocate an extra slot in
> the `ddp_comp` array but mtk_drm_crtc_create() never seemed to
> initialize it in the test case I ran. For me, this caused a later
> crash when we looped through the array in mtk_drm_crtc_mode_valid().
> This showed up for me when I booted with `slub_debug=FZPUA` which
> poisons the memory initially. Without `slub_debug` I couldn't
> reproduce, presumably because the later code handles the value being
> NULL and in most cases (not guaranteed in all cases) the memory the
> allocator returned started out as 0.
> 
> It really doesn't hurt to initialize the array with devm_kcalloc()
> since the array is small and the overhead of initting a handful of
> elements to 0 is small. In general initting memory to zero is a safer
> practice and usually it's suggested to only use the non-initting
> alloc
> functions if you really need to.
> 
> Let's switch the function to use an allocation function that zeros
> the
> memory. For me, this avoids the crash.

Reviewed-by: CK Hu <ck.hu@xxxxxxxxxxxx>

> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection
> capability")
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> I don't have a ton of experience with this driver to know if the fact
> that the array item was still uninitialized when
> mtk_drm_crtc_mode_valid() ran is the sign of a bug that should be
> fixed. However, even if it is a bug and that bug is fixed then
> zeroing
> memory when we allocate is still safer. If it's a bug that this
> memory
> wasn't initialized then please consider this patch a bug report. ;-)
> 
> I'll also note that I reproduced this on a downstream 6.1-based
> kernel. It appears that only mt8188 uses `conn_routes` and, as far as
> I can tell, mt8188 isn't supported upstream yet.
> 
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index a04499c4f9ca..29207b2756c1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -1009,10 +1009,10 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  
>  	mtk_crtc->mmsys_dev = priv->mmsys_dev;
>  	mtk_crtc->ddp_comp_nr = path_len;
> -	mtk_crtc->ddp_comp = devm_kmalloc_array(dev,
> -						mtk_crtc->ddp_comp_nr +
> (conn_routes ? 1 : 0),
> -						sizeof(*mtk_crtc-
> >ddp_comp),
> -						GFP_KERNEL);
> +	mtk_crtc->ddp_comp = devm_kcalloc(dev,
> +					  mtk_crtc->ddp_comp_nr +
> (conn_routes ? 1 : 0),
> +					  sizeof(*mtk_crtc->ddp_comp),
> +					  GFP_KERNEL);
>  	if (!mtk_crtc->ddp_comp)
>  		return -ENOMEM;
>  
> -- 
> 2.44.0.396.g6e790dbe36-goog