On 08/12/2023 10:46, Thierry Reding wrote:> From: Thierry Reding <treding at nvidia.com>
>
> Commit 12c9b05da918 ("drm/nouveau/imem: support allocations not
> preserved across suspend") uses container_of() to cast from struct
> nvkm_memory to struct nvkm_instobj, assuming that all instance objects
> are derived from struct nvkm_instobj. For the gk20a family that's not
> the case and they are derived from struct nvkm_memory instead. This
> causes some subtle data corruption (nvkm_instobj.preserve ends up
> mapping to gk20a_instobj.vaddr) that causes a NULL pointer dereference
> in gk20a_instobj_acquire_iommu() (and possibly elsewhere) and also
> prevents suspend/resume from working.
>
> Fix this by making struct gk20a_instobj derive from struct nvkm_instobj
> instead.
>
> Fixes: 12c9b05da918 ("drm/nouveau/imem: support allocations not
preserved across suspend")
> Reported-by: Jonathan Hunter <jonathanh at nvidia.com>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
> Note that this was probably subtly wrong before the above-mentioned
> commit already, but I don't think we've seen any reports that would
> indicate any actual failures related to this before. So I think it's
> good enough to apply this fix for v6.7. The next closest thing would
> be commit d8e83994aaf6 ("drm/nouveau/imem: improve management of
> instance memory"), but that's 8 years old (Linux v4.3)...
> ---
> .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> index 1b811d6972a1..201022ae9214 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c
> @@ -49,14 +49,14 @@
> #include <subdev/mmu.h>
>
> struct gk20a_instobj {
> - struct nvkm_memory memory;
> + struct nvkm_instobj base;
> struct nvkm_mm_node *mn;
> struct gk20a_instmem *imem;
>
> /* CPU mapping */
> u32 *vaddr;
> };
> -#define gk20a_instobj(p) container_of((p), struct gk20a_instobj, memory)
> +#define gk20a_instobj(p) container_of((p), struct gk20a_instobj,
base.memory)
>
> /*
> * Used for objects allocated using the DMA API
> @@ -148,7 +148,7 @@ gk20a_instobj_iommu_recycle_vaddr(struct
gk20a_instobj_iommu *obj)
> list_del(&obj->vaddr_node);
> vunmap(obj->base.vaddr);
> obj->base.vaddr = NULL;
> - imem->vaddr_use -= nvkm_memory_size(&obj->base.memory);
> + imem->vaddr_use -= nvkm_memory_size(&obj->base.base.memory);
> nvkm_debug(&imem->base.subdev, "vaddr used: %x/%x\n",
imem->vaddr_use,
> imem->vaddr_max);
> }
> @@ -283,7 +283,7 @@ gk20a_instobj_map(struct nvkm_memory *memory, u64
offset, struct nvkm_vmm *vmm,
> {
> struct gk20a_instobj *node = gk20a_instobj(memory);
> struct nvkm_vmm_map map = {
> - .memory = &node->memory,
> + .memory = &node->base.memory,
> .offset = offset,
> .mem = node->mn,
> };
> @@ -391,8 +391,8 @@ gk20a_instobj_ctor_dma(struct gk20a_instmem *imem, u32
npages, u32 align,
> return -ENOMEM;
> *_node = &node->base;
>
> - nvkm_memory_ctor(&gk20a_instobj_func_dma, &node->base.memory);
> - node->base.memory.ptrs = &gk20a_instobj_ptrs;
> + nvkm_memory_ctor(&gk20a_instobj_func_dma,
&node->base.base.memory);
> + node->base.base.memory.ptrs = &gk20a_instobj_ptrs;
>
> node->base.vaddr = dma_alloc_attrs(dev, npages << PAGE_SHIFT,
> &node->handle, GFP_KERNEL,
> @@ -438,8 +438,8 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem,
u32 npages, u32 align,
> *_node = &node->base;
> node->dma_addrs = (void *)(node->pages + npages);
>
> - nvkm_memory_ctor(&gk20a_instobj_func_iommu,
&node->base.memory);
> - node->base.memory.ptrs = &gk20a_instobj_ptrs;
> + nvkm_memory_ctor(&gk20a_instobj_func_iommu,
&node->base.base.memory);
> + node->base.base.memory.ptrs = &gk20a_instobj_ptrs;
>
> /* Allocate backing memory */
> for (i = 0; i < npages; i++) {
> @@ -533,7 +533,7 @@ gk20a_instobj_new(struct nvkm_instmem *base, u32 size,
u32 align, bool zero,
> else
> ret = gk20a_instobj_ctor_dma(imem, size >> PAGE_SHIFT,
> align, &node);
> - *pmemory = node ? &node->memory : NULL;
> + *pmemory = node ? &node->base.memory : NULL;
> if (ret)
> return ret;
>
Tested-by: Jon Hunter <jonathanh at nvidia.com>
Thanks!
Jon
--
nvpublic