Re: [PATCH] accel/amdxdna: put the chained BO when its mapping fails

Re: [PATCH] accel/amdxdna: put the chained BO when its mapping fails

[Lizhi Hou]

On 8/19/26 16:08, Taimuraz Kaitmazov wrote:
> amdxdna_cmd_set_error() looks up the first BO of a command chain, which
> takes a reference, and drops it at the end of the function. The mapping of
> that BO is established in between, and the failure path returns without the
> put, so the reference is leaked.
>
> Ordinary use does not reach it. The chain has been submitted before any of
> this runs, so aie2_cmdlist_fill_slot() has already called
> amdxdna_cmd_get_op() on that BO and amdxdna_gem_vmap() has cached its
> address. What makes it reachable is that the BO is resolved again by
> handle here, and the handle is userspace's to recycle: closing it after
> submission and importing a dma-buf whose exporter implements no vmap onto
> the same id leaves amdxdna_gem_get_obj() returning an object this cannot
> map, since prime_import() types every import AMDXDNA_BO_SHARE.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_ctx.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c b/drivers/accel/amdxdna/amdxdna_ctx.c
> index 855da8c79a1c..4e4a9cf5c327 100644
> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
> @@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo,
>   		if (!abo)
>   			return -EINVAL;
>   		cmd = amdxdna_gem_vmap(abo);
> -		if (!cmd)
> +		if (!cmd) {
> +			amdxdna_gem_put_obj(abo);
>   			return -ENOMEM;
> +		}

Thanks for the fix. I will add 'Fixes' tag when I merge.

Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>


Lizhi

>   	}
>   
>   	memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));

Re: [PATCH] accel/amdxdna: put the chained BO when its mapping fails

[Lizhi Hou]

Applied to drm-misc-fixes

On 8/21/26 08:39, Lizhi Hou wrote:
>
> On 8/19/26 16:08, Taimuraz Kaitmazov wrote:
>> amdxdna_cmd_set_error() looks up the first BO of a command chain, which
>> takes a reference, and drops it at the end of the function. The 
>> mapping of
>> that BO is established in between, and the failure path returns 
>> without the
>> put, so the reference is leaked.
>>
>> Ordinary use does not reach it. The chain has been submitted before 
>> any of
>> this runs, so aie2_cmdlist_fill_slot() has already called
>> amdxdna_cmd_get_op() on that BO and amdxdna_gem_vmap() has cached its
>> address. What makes it reachable is that the BO is resolved again by
>> handle here, and the handle is userspace's to recycle: closing it after
>> submission and importing a dma-buf whose exporter implements no vmap 
>> onto
>> the same id leaves amdxdna_gem_get_obj() returning an object this cannot
>> map, since prime_import() types every import AMDXDNA_BO_SHARE.
>>
>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>> ---
>>   drivers/accel/amdxdna/amdxdna_ctx.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c 
>> b/drivers/accel/amdxdna/amdxdna_ctx.c
>> index 855da8c79a1c..4e4a9cf5c327 100644
>> --- a/drivers/accel/amdxdna/amdxdna_ctx.c
>> +++ b/drivers/accel/amdxdna/amdxdna_ctx.c
>> @@ -183,8 +183,10 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj 
>> *abo,
>>           if (!abo)
>>               return -EINVAL;
>>           cmd = amdxdna_gem_vmap(abo);
>> -        if (!cmd)
>> +        if (!cmd) {
>> +            amdxdna_gem_put_obj(abo);
>>               return -ENOMEM;
>> +        }
>
> Thanks for the fix. I will add 'Fixes' tag when I merge.
>
> Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
>
>
> Lizhi
>
>>       }
>>         memset(cmd->data, 0xff, abo->mem.size - sizeof(*cmd));