Nouveau - Feb 2024 - [PATCH] drm/nouveau: fix several DMA buffer leaks

Nouveau manages DMA buffers with nvkm_gsp_mem objects.  Several of these
buffers used to load and communicate with GSP-RM are allocated
but never dealloced.  Some of them can be deallocated after GSP-RM is
initialized, but the rest need to stay until the driver unloads.

Also futher bullet-proof these objects by clearing mem->addr when
the are deallocated.

Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting
GSP-RM")
Signed-off-by: Timur Tabi <ttabi at nvidia.com>
---
 .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c    | 54 +++++++++++--------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 9ee58e2a0eb2..463b1c153222 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -997,6 +997,27 @@ r535_gsp_rpc_get_gsp_static_info(struct nvkm_gsp *gsp)
 	return 0;
 }
 
+static void
+nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem)
+{
+	if (mem->data) {
+		dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data,
mem->addr);
+		mem->data = NULL;
+		mem->addr = 0;
+	}
+}
+
+static int
+nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, size_t size, struct nvkm_gsp_mem *mem)
+{
+	mem->size = size;
+	mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size,
&mem->addr, GFP_KERNEL);
+	if (WARN_ON(!mem->data))
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int
 r535_gsp_postinit(struct nvkm_gsp *gsp)
 {
@@ -1024,6 +1045,13 @@ r535_gsp_postinit(struct nvkm_gsp *gsp)
 
 	nvkm_inth_allow(&gsp->subdev.inth);
 	nvkm_wr32(device, 0x110004, 0x00000040);
+
+	/* Release the DMA buffers that were needed only for boot and init */
+	nvkm_gsp_mem_dtor(gsp, &gsp->boot.fw);
+	nvkm_gsp_mem_dtor(gsp, &gsp->libos);
+	nvkm_gsp_mem_dtor(gsp, &gsp->rmargs);
+	nvkm_gsp_mem_dtor(gsp, &gsp->wpr_meta);
+
 	return ret;
 }
 
@@ -1532,27 +1560,6 @@ r535_gsp_msg_run_cpu_sequencer(void *priv, u32 fn, void
*repv, u32 repc)
 	return 0;
 }
 
-static void
-nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem)
-{
-	if (mem->data) {
-		dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data,
mem->addr);
-		mem->data = NULL;
-	}
-}
-
-static int
-nvkm_gsp_mem_ctor(struct nvkm_gsp *gsp, u32 size, struct nvkm_gsp_mem *mem)
-{
-	mem->size = size;
-	mem->data = dma_alloc_coherent(gsp->subdev.device->dev, size,
&mem->addr, GFP_KERNEL);
-	if (WARN_ON(!mem->data))
-		return -ENOMEM;
-
-	return 0;
-}
-
-
 static int
 r535_gsp_booter_unload(struct nvkm_gsp *gsp, u32 mbox0, u32 mbox1)
 {
@@ -2150,6 +2157,11 @@ r535_gsp_dtor(struct nvkm_gsp *gsp)
 	mutex_destroy(&gsp->cmdq.mutex);
 
 	r535_gsp_dtor_fws(gsp);
+
+	nvkm_gsp_mem_dtor(gsp, &gsp->shm.mem);
+	nvkm_gsp_mem_dtor(gsp, &gsp->loginit);
+	nvkm_gsp_mem_dtor(gsp, &gsp->logintr);
+	nvkm_gsp_mem_dtor(gsp, &gsp->logrm);
 }
 
 int
-- 
2.34.1

On Thu, 2024-02-01 at 13:55 -0600, Timur Tabi wrote:
> +static void
> +nvkm_gsp_mem_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_mem *mem)
> +{
> +???????if (mem->data) {
> +???????????????dma_free_coherent(gsp->subdev.device->dev,
mem->size, mem-
> >data, mem->addr);
> +???????????????mem->data = NULL;
> +???????????????mem->addr = 0;
> +???????}
> +}
Dave, what do you think about doing this:

if (mem->data) {

	memset(mem->data, 0, mem->size);   <-------

	dma_free_coherent(gsp->subdev.device->dev, mem->size,
mem->data, mem->addr);
	mem->data = NULL;
	mem->addr = 0;
}

This would help situations where a buffer is access by GSP-RM after we think
it's safe to free it.