Nouveau - Aug 2025 - [PATCH] drm/nouveau/gsp: fix mismatched alloc/free for kvmalloc()

On Mon, 2025-08-11 at 17:19 +0800, Qianfeng Rong wrote:
> Replace kfree() with kvfree() for memory allocated by kvmalloc().
> 
> Compile-tested only.
> 
> Signed-off-by: Qianfeng Rong <rongqianfeng at vivo.com>
Reviewed-by: Timur Tabi <ttabi at nvidia.com>

This does fix a real bug.

However, I think the real problem is that it's really confusing that
r535_gsp_msgq_recv_one_elem(gsp, &info) returns info.gsp_rpc_buf instead of
just success/failure.
r535_gsp_msgq_recv() does this:

	buf = kvmalloc(max_t(u32, rpc->length, expected), GFP_KERNEL);
...
	info.gsp_rpc_buf = buf;
... 
	buf = r535_gsp_msgq_recv_one_elem(gsp, &info);

You wouldn't know it, but this does not change the value of 'buf'
unless
r535_gsp_msgq_recv_one_elem() fails.  If it does fail, the code does this:

	if (IS_ERR(buf)) {
		kvfree(info.gsp_rpc_buf);

It would be a lot clearer if we could kvfree(buf) here, but we can't because
'buf' no longer points
to the buffer, even though the buffer still exists.

On Wed Aug 13, 2025 at 12:52 AM CEST, Timur Tabi wrote:
> On Mon, 2025-08-11 at 17:19 +0800, Qianfeng Rong wrote:
>> Replace kfree() with kvfree() for memory allocated by kvmalloc().
>> 
>> Compile-tested only.
>> 
>> Signed-off-by: Qianfeng Rong <rongqianfeng at vivo.com>
>
> Reviewed-by: Timur Tabi <ttabi at nvidia.com>
>
> This does fix a real bug.
>
> However, I think the real problem is that it's really confusing that
> r535_gsp_msgq_recv_one_elem(gsp, &info) returns info.gsp_rpc_buf
instead of just success/failure.
> r535_gsp_msgq_recv() does this:
>
> 	buf = kvmalloc(max_t(u32, rpc->length, expected), GFP_KERNEL);
> ...
> 	info.gsp_rpc_buf = buf;
> ... 
> 	buf = r535_gsp_msgq_recv_one_elem(gsp, &info);
>
> You wouldn't know it, but this does not change the value of
'buf' unless
> r535_gsp_msgq_recv_one_elem() fails.  If it does fail, the code does this:
Ick! That makes no sense, r535_gsp_msgq_recv_one_elem() should just return an
int and we shouldn't overwrite buf -- that's a footgun.
> 	if (IS_ERR(buf)) {
> 		kvfree(info.gsp_rpc_buf);
Should just be

	if (ret) {
		kvfree(buf);
		return ERR_PTR(ret);
	}

It also doesn't need the info.gsp_rpc_buf = NULL; assignment, info is local
anyways.
> It would be a lot clearer if we could kvfree(buf) here, but we can't
because 'buf' no longer points
> to the buffer, even though the buffer still exists.
Agreed.

Zhi, Timur do you want to send a follow-up patch for this?

On 13/08/2025 1.52, Timur Tabi wrote:
> On Mon, 2025-08-11 at 17:19 +0800, Qianfeng Rong wrote:
>> Replace kfree() with kvfree() for memory allocated by kvmalloc().
>>
>> Compile-tested only.
>>
>> Signed-off-by: Qianfeng Rong <rongqianfeng at vivo.com>
> 
> Reviewed-by: Timur Tabi <ttabi at nvidia.com>
> 
> This does fix a real bug.
> 
Agree with the coding details.

I felt the core issue is that GSP RPC lifecycle management in NVKM is 
not handled cleanly. For example, the caller?s RPC buffer is freed 
silently in the receive path, and a new buffer is allocated and returned 
without explicit coordination.

Introducing large GSP RPCs - such as factoring out 
r535_gsp_msgq_recv_one_elem() - only makes this flaw more apparent, and 
even the refactoring process is cumbersome and tricky.

Ideally, there should be a clear ownership and lifecycle flow between 
the caller and the GSP RPC routines: the caller allocates and frees the 
RPC buffer, while the low-level routines focus solely on send/receive 
operations. r535_gsp_msgq_recv_one_elem() is just on its half way.

Z.
> However, I think the real problem is that it's really confusing that
> r535_gsp_msgq_recv_one_elem(gsp, &info) returns info.gsp_rpc_buf
instead of just success/failure.
> r535_gsp_msgq_recv() does this:
> 
> 	buf = kvmalloc(max_t(u32, rpc->length, expected), GFP_KERNEL);
> ...
> 	info.gsp_rpc_buf = buf;
> ...
> 	buf = r535_gsp_msgq_recv_one_elem(gsp, &info);
> 
> You wouldn't know it, but this does not change the value of
'buf' unless
> r535_gsp_msgq_recv_one_elem() fails.  If it does fail, the code does this:
> 
> 	if (IS_ERR(buf)) {
> 		kvfree(info.gsp_rpc_buf);
> 
> It would be a lot clearer if we could kvfree(buf) here, but we can't
because 'buf' no longer points
> to the buffer, even though the buffer still exists.
> 
>