[PATCH 1/3] drm/msm: Avoid NULL dereference in msm_disp_state_print_regs()

[PATCH 1/3] drm/msm: Avoid NULL dereference in msm_disp_state_print_regs()

[Douglas Anderson]

If the allocation in msm_disp_state_dump_regs() failed then
`block->state` can be NULL. The msm_disp_state_print_regs() function
_does_ have code to try to handle it with:

  if (*reg)
    dump_addr = *reg;

...but since "dump_addr" is initialized to NULL the above is actually
a noop. The code then goes on to dereference `dump_addr`.

Make the function print "Registers not stored" when it sees a NULL to
solve this. Since we're touching the code, fix
msm_disp_state_print_regs() not to pointlessly take a double-pointer
and properly mark the pointer as `const`.

Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index add72bbc28b1..bb149281d31f 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -48,20 +48,21 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	}
 }
 
-static void msm_disp_state_print_regs(u32 **reg, u32 len, void __iomem *base_addr,
-		struct drm_printer *p)
+static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
+		void __iomem *base_addr, struct drm_printer *p)
 {
 	int i;
-	u32 *dump_addr = NULL;
 	void __iomem *addr;
 	u32 num_rows;
 
+	if (!dump_addr) {
+		drm_printf(p, "Registers not stored\n");
+		return;
+	}
+
 	addr = base_addr;
 	num_rows = len / REG_DUMP_ALIGN;
 
-	if (*reg)
-		dump_addr = *reg;
-
 	for (i = 0; i < num_rows; i++) {
 		drm_printf(p, "0x%lx : %08x %08x %08x %08x\n",
 				(unsigned long)(addr - base_addr),
@@ -89,7 +90,7 @@ void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
 
 	list_for_each_entry_safe(block, tmp, &state->blocks, node) {
 		drm_printf(p, "====================%s================\n", block->name);
-		msm_disp_state_print_regs(&block->state, block->size, block->base_addr, p);
+		msm_disp_state_print_regs(block->state, block->size, block->base_addr, p);
 	}
 
 	drm_printf(p, "===================dpu drm state================\n");
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/3] drm/msm: Allocate memory for disp snapshot with kvzalloc()

[Douglas Anderson]

With the "drm/msm: add a display mmu fault handler" series [1] we saw
issues in the field where memory allocation was failing when
allocating space for registers in msm_disp_state_dump_regs().
Specifically we were seeing an order 5 allocation fail. It's not
surprising that order 5 allocations will sometimes fail after the
system has been up and running for a while.

There's no need here for contiguous memory. Change the allocation to
kvzalloc() which should make it much less likely to fail.

[1] https://lore.kernel.org/r/20240628214848.4075651-1-quic_abhinavk@quicinc.com/

Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index bb149281d31f..4d55e3cf570f 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -26,7 +26,7 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	end_addr = base_addr + aligned_len;
 
 	if (!(*reg))
-		*reg = kzalloc(len_padded, GFP_KERNEL);
+		*reg = kvzalloc(len_padded, GFP_KERNEL);
 
 	if (*reg)
 		dump_addr = *reg;
@@ -162,7 +162,7 @@ void msm_disp_state_free(void *data)
 
 	list_for_each_entry_safe(block, tmp, &disp_state->blocks, node) {
 		list_del(&block->node);
-		kfree(block->state);
+		kvfree(block->state);
 		kfree(block);
 	}
 
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 2/3] drm/msm: Allocate memory for disp snapshot with kvzalloc()

[Abhinav Kumar]

On 10/14/2024 9:36 AM, Douglas Anderson wrote:
> With the "drm/msm: add a display mmu fault handler" series [1] we saw
> issues in the field where memory allocation was failing when
> allocating space for registers in msm_disp_state_dump_regs().
> Specifically we were seeing an order 5 allocation fail. It's not
> surprising that order 5 allocations will sometimes fail after the
> system has been up and running for a while.
> 
> There's no need here for contiguous memory. Change the allocation to
> kvzalloc() which should make it much less likely to fail.
> 
> [1] https://lore.kernel.org/r/20240628214848.4075651-1-quic_abhinavk@quicinc.com/
> 
> Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

I had some doubts on how this issue happens considering that the devcore 
should automatically release the memory within 5 sec even if userspace 
had not read this. So there is no leak as such, its just that in a 
heavily loaded system, this can happen.

Fix looks okay to me,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 3/3] drm/msm: Simplify NULL checking in msm_disp_state_dump_regs()

[Douglas Anderson]

The msm_disp_state_dump_regs():

- Doesn't allocate if the caller already allocated. ...but there's one
  caller and it doesn't allocate so we don't need this check.
- Checks for allocation failure over and over even though it could
  just do it once right after the allocation.

Clean this up.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index 4d55e3cf570f..07a2c1e87219 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -25,24 +25,21 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	addr = base_addr;
 	end_addr = base_addr + aligned_len;
 
-	if (!(*reg))
-		*reg = kvzalloc(len_padded, GFP_KERNEL);
-
-	if (*reg)
-		dump_addr = *reg;
+	*reg = kvzalloc(len_padded, GFP_KERNEL);
+	if (!*reg)
+		return;
 
+	dump_addr = *reg;
 	for (i = 0; i < num_rows; i++) {
 		x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
 		x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
 		x8 = (addr + 0x8 < end_addr) ? readl_relaxed(addr + 0x8) : 0;
 		xc = (addr + 0xc < end_addr) ? readl_relaxed(addr + 0xc) : 0;
 
-		if (dump_addr) {
-			dump_addr[i * 4] = x0;
-			dump_addr[i * 4 + 1] = x4;
-			dump_addr[i * 4 + 2] = x8;
-			dump_addr[i * 4 + 3] = xc;
-		}
+		dump_addr[i * 4] = x0;
+		dump_addr[i * 4 + 1] = x4;
+		dump_addr[i * 4 + 2] = x8;
+		dump_addr[i * 4 + 3] = xc;
 
 		addr += REG_DUMP_ALIGN;
 	}
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 3/3] drm/msm: Simplify NULL checking in msm_disp_state_dump_regs()

[Abhinav Kumar]

On 10/14/2024 9:36 AM, Douglas Anderson wrote:
> The msm_disp_state_dump_regs():
> 
> - Doesn't allocate if the caller already allocated. ...but there's one
>    caller and it doesn't allocate so we don't need this check.
> - Checks for allocation failure over and over even though it could
>    just do it once right after the allocation.
> 
> Clean this up.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   .../gpu/drm/msm/disp/msm_disp_snapshot_util.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 1/3] drm/msm: Avoid NULL dereference in msm_disp_state_print_regs()

[Abhinav Kumar]

On 10/14/2024 9:36 AM, Douglas Anderson wrote:
> If the allocation in msm_disp_state_dump_regs() failed then
> `block->state` can be NULL. The msm_disp_state_print_regs() function
> _does_ have code to try to handle it with:
> 
>    if (*reg)
>      dump_addr = *reg;
> 
> ...but since "dump_addr" is initialized to NULL the above is actually
> a noop. The code then goes on to dereference `dump_addr`.
> 
> Make the function print "Registers not stored" when it sees a NULL to
> solve this. Since we're touching the code, fix
> msm_disp_state_print_regs() not to pointlessly take a double-pointer
> and properly mark the pointer as `const`.
> 
> Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 

LGTM, thanks for the fix

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Re: [PATCH 1/3] drm/msm: Avoid NULL dereference in msm_disp_state_print_regs()

[Dmitry Baryshkov]

On Mon, 14 Oct 2024 09:36:08 -0700, Douglas Anderson wrote:
> If the allocation in msm_disp_state_dump_regs() failed then
> `block->state` can be NULL. The msm_disp_state_print_regs() function
> _does_ have code to try to handle it with:
> 
>   if (*reg)
>     dump_addr = *reg;
> 
> [...]

Applied, thanks!

[3/3] drm/msm: Simplify NULL checking in msm_disp_state_dump_regs()
      https://gitlab.freedesktop.org/lumag/msm/-/commit/74c374648ed0

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>