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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The newly added kernel-doc claims that several high-level RPC functions only return ERR_PTR on error, but they can actually propagate NULL.

Pre-existing issues:
- [High] Returning NULL on an invalid RPC message length causes a queue deadlock and subsequent NULL pointer dereferences in downstream callers that only check IS_ERR().
--

commit db05356d173cc7558f718a62efd7f0f223cf7152
Author: Hongling Zeng <zenghongling@kylinos.cn>

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 the nouveau GSP code to clarify which functions can
return error pointers or NULL.

> 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..35272c1538699 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

[ ... ]

> @@ -283,6 +300,15 @@ r535_gsp_msgq_recv_one_elem(struct nvkm_gsp *gsp,
>  	return buf;
>  }
>  
> +/**
> + * r535_gsp_msgq_recv - Receive RPC message(s) from GSP queue
> + * @gsp: GSP device
> + * @gsp_rpc_len: Expected RPC length
> + * @retries: Retry counter
> + *
> + * Return: Pointer to received buffer on success, ERR_PTR() on error,
> + *         or NULL if RPC length is invalid.
> + */
>  static void *
>  r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries)

[Severity: High]
This is a pre-existing issue, but does returning NULL here on an invalid RPC
message length cause issues for callers?

When r535_gsp_msgq_recv() encounters an invalid length, it returns NULL
without advancing the queue read pointer:

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 return propagates up to API wrappers like nvkm_gsp_rpc_rd() and
nvkm_gsp_rpc_wr(). Downstream callers such as r535_gsp_get_static_info()
check for errors using IS_ERR(). Since IS_ERR(NULL) is false, the error check
is bypassed. This leads to silent successes (like PTR_ERR(NULL) == 0) or
immediate NULL pointer dereferences when the payload is accessed.

Also, since the queue read pointer is not advanced, wouldn't subsequent reads
process the same invalid message and deadlock the queue?

[ ... ]

> @@ -450,6 +476,20 @@ r535_gsp_msg_dump(struct nvkm_gsp *gsp, struct nvfw_gsp_rpc *msg, int lvl)
>  	}
>  }
>  
> +/**
> + * r535_gsp_msg_recv - Receive RPC message from GSP message queue
> + * @gsp: GSP device
> + * @fn: Expected RPC function number (0 to accept any)
> + * @gsp_rpc_len: Expected RPC length (0 for POLL mode)
> + *
> + * Return: Pointer to RPC message on success, ERR_PTR() on error.
> + *         When fn!=0: retries until timeout, returns ERR_PTR(-ETIMEDOUT)
> + *                    if no match found (never returns NULL for timeout).

[Severity: Medium]
Is this documentation accurate regarding error returns?

The kernel-doc states this function returns ERR_PTR() on error, but it
can actually propagate the NULL returned by r535_gsp_msgq_recv() when an
invalid RPC length is encountered:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msg_recv() {
        ...
        rpc = r535_gsp_msgq_recv(gsp, gsp_rpc_len, &retries);
        if (IS_ERR_OR_NULL(rpc))
                return rpc;
        ...
}

Could this mislead developers into incorrectly replacing IS_ERR_OR_NULL()
checks with IS_ERR() in the callers?

[ ... ]

> @@ -628,6 +703,16 @@ 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),
> + *         pointer to reply data on success, or ERR_PTR() on error.
> + */
>  static void *
>  r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload,

[Severity: Medium]
Does this documentation also hide a potential NULL return on error?

Similar to r535_gsp_msg_recv() above, this function can return NULL on error
if an invalid RPC length propagates up from the reply handler. Should the
documentation reflect that NULL can be returned as an error state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622021207.14764-1-zenghongling@kylinos.cn?part=1