Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The commit message explains that `nvkm_gsp_rpc_push()` should not return `NULL` for RECV policies, but this assumption is incorrect, leaving callers vulnerable to NULL pointer dereferences. - [Medium] Memory leak of large RPC payloads in `r535_gsp_rpc_push()` on error paths. -- commit 1659548cc5b32b66b3c5a9f4367151960bb1562d Author: Hongling Zeng <[email protected]> nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd() nvkm_gsp_rpc_rd() calls nvkm_gsp_rpc_get() which never returns NULL, only valid pointers or error pointers. Clean up by using IS_ERR() instead, matching the actual return value contract. > Note: nvkm_gsp_rpc_push() can return NULL when policy is NOWAIT or > NOSEQ, but nvkm_gsp_rpc_rd() uses RECV policy which expects a reply. > If the RPC fails to get a reply, it should return an error pointer, > not NULL. [Severity: High] This isn't a bug introduced by this patch, but is this description accurate? If the RPC reply has an invalid length, r535_gsp_msgq_recv() returns NULL: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv() { ... if (WARN_ON(rpc->length > max_rpc_size)) return NULL; ... } This NULL propagates up through r535_gsp_msg_recv() and r535_gsp_rpc_handle_reply(), causing nvkm_gsp_rpc_push() to return NULL even with the RECV policy. Since callers like r535_gsp_get_static_info() only check IS_ERR(), won't they dereference the NULL pointer? drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c:r535_gsp_get_static_info() { ... rpc = nvkm_gsp_rpc_rd(gsp, NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO, 0); if (IS_ERR(rpc)) return PTR_ERR(rpc); gsp->internal.client.object.client = &gsp->internal.client; gsp->internal.client.object.parent = NULL; gsp->internal.client.object.handle = rpc->hInternalClient; ... } > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > index 64fed208e4cf8..a061779e0107b 100644 > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h > @@ -293,7 +293,7 @@ nvkm_gsp_rpc_rd(struct nvkm_gsp *gsp, u32 fn, u32 argc) > { > void *argv = nvkm_gsp_rpc_get(gsp, fn, argc); > > - if (IS_ERR_OR_NULL(argv)) > + if (IS_ERR(argv)) > return argv; > > return nvkm_gsp_rpc_push(gsp, argv, NVKM_GSP_RPC_REPLY_RECV, argc); [Severity: Medium] This is a pre-existing issue, but does r535_gsp_rpc_push() leak memory on error paths for large payloads? If a large RPC payload fails to send, the kvfree(msg) call is skipped: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() { ... /* Wait for reply. */ repv = r535_gsp_rpc_handle_reply(gsp, fn, policy, payload_size + sizeof(*rpc)); if (!IS_ERR(repv)) kvfree(msg); } else { repv = r535_gsp_rpc_send(gsp, payload, policy, gsp_rpc_len); } ... } Since small RPCs unconditionally free msg via r535_gsp_cmdq_push(), callers assume the RPC layer consumes the buffer. Will this leak the large staging buffer if r535_gsp_rpc_get(), r535_gsp_rpc_send(), or r535_gsp_rpc_handle_reply() fails and returns an error pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
