| Message ID | 20241014093605.1.Ia1217cecec9ef09eb3c6d125360cc6c8574b0e73@changeid |
|---|---|
| State | Accepted |
| Commit | 293f53263266bc4340d777268ab4328a97f041fa |
| Headers | show |
| Series | [1/3] drm/msm: Avoid NULL dereference in msm_disp_state_print_regs() | expand |
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>
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,
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");
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(-)