Re: [PATCH] nouveau/instmem: use iomapping interface for instmem handling (v2)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unchecked failure in `check_io_mapping` leads to bogus pointer generation in `nv50_instobj_boot`, causing a kernel panic on dereference.
- [Medium] Unconditional call to `io_mapping_fini` during teardown can pass a NULL pointer to `iounmap`, leading to architecture-specific panics or warnings.
- [Medium] Unbounded retries of `ioremap_wc` on hot paths if `check_io_mapping` fails, leading to severe performance degradation.
--

commit 96779776b5649805dcd1602bcce1a5916b4e55a8
Author: Dave Airlie <airlied@redhat.com>

nouveau/instmem: use iomapping interface for instmem handling (v2)

This patch modifies the nouveau driver to use the iomapping interface for
instmem handling. It delays creating the IO mapping until the first use to
avoid resource mapping conflicts with simpledrm during initialization.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
> index 6fbaa1e5876dc..7314650f6d230 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/nv50.c
[ ... ]
> @@ -172,8 +175,7 @@ nv50_instobj_kmap(struct nv50_instobj *iobj, struct nvkm_vmm *vmm)
>  
>  	/* Make the mapping visible to the host. */
>  	iobj->bar = bar;
> -	iobj->map = ioremap_wc(device->func->resource_addr(device, NVKM_BAR2_INST) +
> -			       (u32)iobj->bar->addr, size);
> +	iobj->map = io_mapping_map_wc(&imem->iomap, (u32)iobj->bar->addr, size);
>  	if (!iobj->map) {

[Severity: High]
If check_io_mapping() fails to initialize the mapping, imem->iomap.iomem will 
be NULL. io_mapping_map_wc() computes mapping->iomem + offset, which means it
will return NULL + (u32)iobj->bar->addr.

If the offset is greater than 0, this results in a non-NULL but invalid 
pointer. The check if (!iobj->map) will fail to catch it. Can this lead 
to a kernel panic when the bogus pointer is later dereferenced?

>  		nvkm_warn(subdev, "PRAMIN ioremap failed\n");
>  		nvkm_vmm_put(vmm, &iobj->bar);
> @@ -188,6 +190,17 @@ nv50_instobj_map(struct nvkm_memory *memory, u64 offset, struct nvkm_vmm *vmm,
>  	return nvkm_memory_map(memory, offset, vmm, vma, argv, argc);
>  }
>  
> +static void
> +check_io_mapping(struct nv50_instmem *imem)
> +{
> +	if (imem->iomap.size == 0) {
> +		struct nvkm_device *device = imem->base.subdev.device;
> +		io_mapping_init_wc(&imem->iomap,
> +				   device->func->resource_addr(device, NVKM_BAR2_INST),
> +				   device->func->resource_size(device, NVKM_BAR2_INST));

[Severity: Medium]
If io_mapping_init_wc() fails, it returns NULL and leaves imem->iomap.size 
as 0. 

Since check_io_mapping() doesn't record the failure or return an error code, 
will the driver endlessly retry this expensive ioremap_wc() operation on 
subsequent calls in hot paths (like nv50_instobj_acquire()) because 
imem->iomap.size is still 0?

> +	}
> +}
> +
[ ... ]
> @@ -277,6 +292,7 @@ nv50_instobj_boot(struct nvkm_memory *memory, struct nvkm_vmm *vmm)
>  		iobj->lru.next = NULL;
>  	}
>  
> +	check_io_mapping(iobj->imem);
>  	nv50_instobj_kmap(iobj, vmm);

[Severity: High]
Since check_io_mapping() does not return an error status, we unconditionally
call nv50_instobj_kmap() here even if the lazy initialization failed. 
Should this verify that the mapping initialization actually succeeded before
proceeding, to prevent generating the bogus pointer mentioned above?

>  	nvkm_instmem_boot(imem);
>  	mutex_unlock(&imem->mutex);
[ ... ]
> @@ -406,10 +422,13 @@ nv50_instmem_fini(struct nvkm_instmem *base)
>  	nv50_instmem(base)->addr = ~0ULL;
>  }
>  
> -static void *
> +void *
>  nv50_instmem_dtor(struct nvkm_instmem *base)
>  {
> -	return nv50_instmem(base);
> +	struct nv50_instmem *imem = nv50_instmem(base);
> +
> +	io_mapping_fini(&imem->iomap);

[Severity: Medium]
Is it safe to call this unconditionally? If the IO mapping was never 
initialized (e.g., if probe failed early, or check_io_mapping() was never 
called), imem->iomap.iomem will be NULL. 

Calling io_mapping_fini() will invoke iounmap(NULL), which can trigger 
warnings or panics on some architectures.

> +	return imem;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706022812.834677-1-airlied@gmail.com?part=1