Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The documentation for r535_gsp_rpc_done() incorrectly states that it frees a payload returned by r535_gsp_rpc_get(). Pre-existing issues: - [High] The newly added documentation for RPC functions introduces contradictory return value contracts that mask a pre-existing NULL pointer dereference bug on error paths. Specifically, `r535_gsp_msgq_recv()` returns `NULL` when the RPC length is invalid, which propagates up and causes a NULL pointer dereference in higher-level callers, contradicting the newly added docs that claim `ERR_PTR()` is returned on error. - [High] Large RPC payloads (`payload_size > max_payload_size`) leak the original `msg` memory if an error occurs during chunk sending or reply handling. -- commit ebdd2cc2dbf735e60a62853951f275c61160e93f Author: Hongling Zeng <[email protected]> Subject: nouveau/gsp/rpc: Document RPC function return value contracts This commit adds kernel-doc comments to document the return value semantics of RPC functions in r535/rpc.c. It clarifies which functions can return NULL or error pointers. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > index 3ca3de8f43408..2ca6de4094e63 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c [ ... ] > @@ -663,6 +663,11 @@ r535_gsp_rpc_send(struct nvkm_gsp *gsp, void *payload, > return r535_gsp_rpc_handle_reply(gsp, fn, policy, gsp_rpc_len); > } > > +/** > + * r535_gsp_rpc_done - Free an RPC message > + * @gsp: GSP device > + * @repv: Pointer to RPC payload data returned by r535_gsp_rpc_get() > + */ > static void > r535_gsp_rpc_done(struct nvkm_gsp *gsp, void *repv) [Severity: Medium] Is this parameter description accurate? The payload returned by r535_gsp_rpc_get() is the request, which is automatically freed upon transmission in functions like r535_gsp_cmdq_push(). It appears r535_gsp_rpc_done() is intended to be used by callers to free the reply payload returned by r535_gsp_rpc_push() or nvkm_gsp_rm_ctrl_rd(). Could documenting this as freeing the request payload encourage callers to explicitly free it after sending, leading to a double-free? [ ... ] > @@ -704,6 +704,17 @@ r535_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 > payload_size) > return rpc->data; > } > > +/** > + * r535_gsp_rpc_push - Push RPC message to GSP and wait for reply > + * @gsp: GSP device > + * @payload: RPC payload to send > + * @policy: Reply policy (NOWAIT, NOSEQ, RECV, or POLL) > + * @gsp_rpc_len: Expected RPC length in the reply > + * > + * Return: NULL when policy is NOWAIT/NOSEQ (no reply expected) or > + * POLL (successful message processing), pointer to reply data > + * on success, or ERR_PTR() on error. > + */ > static void * > r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload, > enum nvkm_gsp_rpc_reply_policy policy, u32 gsp_rpc_len) [Severity: High] This is a pre-existing issue, but does this documentation mask an underlying NULL pointer dereference? r535_gsp_msgq_recv() can return NULL when the RPC length is invalid: r535_gsp_msgq_recv() { if (WARN_ON(rpc->length > max_rpc_size)) return NULL; } This NULL propagates up to r535_gsp_rpc_push() which the documentation states will return ERR_PTR() on error. However, if r535_gsp_rpc_push() returns NULL on an error path, callers like r535_gsp_rpc_rm_ctrl_push() evaluate PTR_ERR(NULL) as 0 (success): r535_gsp_rpc_rm_ctrl_push() { if (IS_ERR_OR_NULL(rpc)) { *params = NULL; return PTR_ERR(rpc); } } This causes callers like r535_disp_get_static_info() to assume success and dereference the NULL pointer: r535_disp_get_static_info() { ctrl = nvkm_gsp_rm_ctrl_rd(...); if (IS_ERR(ctrl)) return PTR_ERR(ctrl); disp->wndw.mask = ctrl->windowPresentMask; } Should the documentation note this, or should the underlying NULL propagation be fixed to return an error pointer? [Severity: High] This isn't a bug introduced by this patch, but while reviewing the documentation for this function, it appears there is a memory leak for large RPC payloads in r535_gsp_rpc_push(). When payload_size is greater than max_payload_size, the code allocates smaller chunks and sends them sequentially. If any chunk fails to send, the code takes the goto done path: r535_gsp_rpc_push() { ... next = r535_gsp_rpc_get(gsp, fn, max_payload_size); if (IS_ERR(next)) { repv = next; goto done; } ... } At the end of the chunking block, the original msg is only freed if repv is not an error pointer: repv = r535_gsp_rpc_handle_reply(...); if (!IS_ERR(repv)) kvfree(msg); Since callers expect the function to consume the payload and do not free it on error, doesn't this permanently leak the original msg memory on these error paths? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
