From: Danilo Krummrich <dakr@redhat.com>
To: Timur Tabi <ttabi@nvidia.com>
Cc: Dave Airlie <airlied@redhat.com>, nouveau@lists.freedesktop.org
Subject: Re: [PATCH 1/2] [v3] drm/nouveau: fix several DMA buffer leaks
Date: Mon, 5 Feb 2024 19:08:13 +0100 [thread overview]
Message-ID: <4bcf8896-7adb-406f-a6aa-dfa990afbfdb@redhat.com> (raw)
In-Reply-To: <20240202230608.1981026-1-ttabi@nvidia.com>
On 2/3/24 00:06, Timur Tabi wrote:
> Nouveau manages GSP-RM DMA buffers with nvkm_gsp_mem objects. Several of
> these buffers are never dealloced. Some of them can be deallocated
> right after GSP-RM is initialized, but the rest need to stay until the
> driver unloads.
>
> Also futher bullet-proof these objects by poisoning the buffer and
> clearing the nvkm_gsp_mem object when it is deallocated. Poisoning
> the buffer should trigger an error (or crash) from GSP-RM if it tries
> to access the buffer after we've deallocated it, because we were wrong
> about when it is safe to deallocate.
>
> Finally, change the mem->size field to a size_t because that's the same
> type that dma_alloc_coherent expects.
>
> Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
Series applied to drm-misc-fixes, thanks!
> ---
> v3: use size_t
> v2: add buffer poisoning
>
> .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 2 +-
> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 59 ++++++++++++-------
> 2 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> index 5c6b8536e31c..3fbc57b16a05 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -9,7 +9,7 @@
> #define GSP_PAGE_SIZE BIT(GSP_PAGE_SHIFT)
>
> struct nvkm_gsp_mem {
> - u32 size;
> + size_t size;
> void *data;
> dma_addr_t addr;
> };
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> index 17fc429ee50b..a9030eb83b4d 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> @@ -999,6 +999,32 @@ 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) {
> + /*
> + * Poison the buffer to catch any unexpected access from
> + * GSP-RM if the buffer was prematurely freed.
> + */
> + memset(mem->data, 0xFF, mem->size);
> +
> + dma_free_coherent(gsp->subdev.device->dev, mem->size, mem->data, mem->addr);
> + memset(mem, 0, sizeof(*mem));
> + }
> +}
> +
> +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)
> {
> @@ -1026,6 +1052,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;
> }
>
> @@ -1760,27 +1793,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)
> {
> @@ -2378,6 +2390,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
prev parent reply other threads:[~2024-02-05 18:08 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-02 23:06 [PATCH 1/2] [v3] drm/nouveau: fix several DMA buffer leaks Timur Tabi
2024-02-02 23:06 ` [PATCH 2/2] drm/nouveau: nvkm_gsp_radix3_sg() should use nvkm_gsp_mem_ctor() Timur Tabi
2024-02-05 18:08 ` Danilo Krummrich [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4bcf8896-7adb-406f-a6aa-dfa990afbfdb@redhat.com \
--to=dakr@redhat.com \
--cc=airlied@redhat.com \
--cc=nouveau@lists.freedesktop.org \
--cc=ttabi@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).