Re: [PATCH 1/2] drm/imagination: Switch the GPUVM to immediate mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/imagination: memory leak of gpuvm_bo on pvr_vm_map error path
--

commit 191fc7b0dba1141708c7edc0b3aeac368ef8588f
Author: Gyeyoung Baek <gye976@gmail.com>

drm/imagination: Switch the GPUVM to immediate mode

This commit switches the GPUVM to immediate mode so that each GEM's GPUVA list
is protected by its gpuva.lock. It introduces deferred operations such as
drm_gpuvm_bo_put_deferred() and cleanup via drm_gpuvm_bo_deferred_cleanup().

> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
[ ... ]
> @@ -214,7 +214,7 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
>  
>  static void pvr_vm_bind_op_fini(struct pvr_vm_bind_op *bind_op)
>  {
> -	drm_gpuvm_bo_put(bind_op->gpuvm_bo);
> +	drm_gpuvm_bo_put_deferred(bind_op->gpuvm_bo);
>  
>  	kfree(bind_op->new_va);
>  	kfree(bind_op->prev_va);
[ ... ]
> @@ -758,7 +769,9 @@ pvr_vm_map(struct pvr_vm_context *vm_ctx, struct pvr_gem_object *pvr_obj,
>  
>  err_cleanup:
>  	mutex_unlock(&vm_ctx->lock);
> +
>  	pvr_vm_bind_op_fini(&bind_op);
> +	drm_gpuvm_bo_deferred_cleanup(&vm_ctx->gpuvm_mgr);
>  
>  	return err;
>  }

[Severity: High]
Does this code leak the gpuvm_bo and the referenced drm_gem_object on the
early error path of pvr_vm_map()?

If pvr_vm_bind_op_map_init() fails internally (for example, if memory is
exhausted when calling pvr_gem_object_get_pages_sgt()), it takes its own
error path:

pvr_vm_bind_op_map_init() {
        ...
err_bind_op_fini:
        pvr_vm_bind_op_fini(bind_op);

        return err;
}

This calls the newly modified pvr_vm_bind_op_fini(), which places the BO onto
the gpuvm_mgr bo_defer list via drm_gpuvm_bo_put_deferred().

However, back in pvr_vm_map(), the early return bypasses the newly added
cleanup call:

pvr_vm_map() {
        ...
        int err = pvr_vm_bind_op_map_init(&bind_op, vm_ctx, pvr_obj,
                                          pvr_obj_offset, device_addr,
                                          size);

        if (err)
                return err;
        ...
}

Because drm_gpuvm_bo_deferred_cleanup() is bypassed here, would this cause an
unbounded accumulation of leaked BOs on the bo_defer list? Can this be fixed
by ensuring the cleanup function is also called on this early return path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-pvr-gpuvm-immediate-v1-0-5b298dfce4f8@gmail.com?part=1