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 <[email protected]> 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/[email protected]?part=1
