Re: [PATCH v4 1/5] nouveau/gsp/rpc: Document RPC function return value contracts

[sashiko-bot@kernel.org]

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 <zenghongling@kylinos.cn>
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/20260622035454.28003-1-zenghongling@kylinos.cn?part=1