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

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

[Douglas Anderson]

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.

Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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

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

[Douglas Anderson]

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.

Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@mediatek.com>

> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection
> capability")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 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

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

[CK Hu (胡俊光)]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

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@mediatek.com>

> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection
> capability")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 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

[-- Attachment #2: Type: text/html, Size: 5937 bytes --]

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@mediatek.com>

> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection
> capability")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

Il 28/03/24 17:22, Douglas Anderson ha scritto:
> 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.
> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

P.S.: I really dislike the dynamic selection stuff, as that's only a partial
solution for something that should've been in DT from day 1 instead.
P.P.S.: I took care of that already - a series is about to come in a few days.

Cheers,
Angelo

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

[AngeloGioacchino Del Regno]

Il 28/03/24 17:22, Douglas Anderson ha scritto:
> 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.
> 
> Fixes: 01389b324c97 ("drm/mediatek: Add connector dynamic selection capability")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

P.S.: I really dislike the dynamic selection stuff, as that's only a partial
solution for something that should've been in DT from day 1 instead.
P.P.S.: I took care of that already - a series is about to come in a few days.

Cheers,
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel