Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Lizhi Hou]

On 8/13/26 09:46, Taimuraz Kaitmazov wrote:
> amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
> rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
> exporter answering with an I/O mapping leaves a void __iomem pointer in
> abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.
>
> amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
> buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
> peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
> no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
> object, so an NPU paired with nouveau or radeon does.
>
> Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
> path is reachable from an unprivileged ioctl, so it does not warn.
>
> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
> ---
>   drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
> index 1f190b319bb..b66ec9e4828 100644
> --- a/drivers/accel/amdxdna/amdxdna_gem.c
> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>   
>   	dma_resv_assert_held(obj->resv);
>   
> -	if (is_import_bo(abo))
> +	if (is_import_bo(abo)) {
>   		ret = dma_buf_vmap(abo->dma_buf, map);
> -	else
> +		/* Callers use mem.kva as an ordinary kernel address. */
> +		if (!ret && map->is_iomem) {
> +			dma_buf_vunmap(abo->dma_buf, map);
> +			return -EOPNOTSUPP;
> +		}

Thanks for the fix. The 'is_iomem' check should be moved to 
amdxdna_gem_vmap() to cover all the cases.

Lizhi

> +	} else {
>   		ret = drm_gem_shmem_object_vmap(obj, map);
> +	}
>   	if (ret)
>   		return ret;
>   	if (!map->vaddr)

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Christian König]

On 8/17/26 19:53, Lizhi Hou wrote:
> 
> On 8/13/26 09:46, Taimuraz Kaitmazov wrote:
>> amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
>> rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
>> exporter answering with an I/O mapping leaves a void __iomem pointer in
>> abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.
>>
>> amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
>> buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
>> peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
>> no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
>> object, so an NPU paired with nouveau or radeon does.
>>
>> Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
>> path is reachable from an unprivileged ioctl, so it does not warn.
>>
>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>> ---
>>   drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
>> index 1f190b319bb..b66ec9e4828 100644
>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>>         dma_resv_assert_held(obj->resv);
>>   -    if (is_import_bo(abo))
>> +    if (is_import_bo(abo)) {
>>           ret = dma_buf_vmap(abo->dma_buf, map);

Mhm, why does amdxdna a vmap in the first place? For some workaround?

Usually DMA-buf only provides that framebuffer emulation scanout inside the kernel.

On the other hand as far as I can see that here should work correctly.

Regards,
Christian.

>> -    else
>> +        /* Callers use mem.kva as an ordinary kernel address. */
>> +        if (!ret && map->is_iomem) {
>> +            dma_buf_vunmap(abo->dma_buf, map);
>> +            return -EOPNOTSUPP;
>> +        }
> 
> Thanks for the fix. The 'is_iomem' check should be moved to amdxdna_gem_vmap() to cover all the cases.
> 
> Lizhi
> 
>> +    } else {
>>           ret = drm_gem_shmem_object_vmap(obj, map);
>> +    }
>>       if (ret)
>>           return ret;
>>       if (!map->vaddr)

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Lizhi Hou]

On 8/24/26 07:06, Christian König wrote:
> On 8/17/26 19:53, Lizhi Hou wrote:
>> On 8/13/26 09:46, Taimuraz Kaitmazov wrote:
>>> amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
>>> rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
>>> exporter answering with an I/O mapping leaves a void __iomem pointer in
>>> abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.
>>>
>>> amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
>>> buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
>>> peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
>>> no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
>>> object, so an NPU paired with nouveau or radeon does.
>>>
>>> Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
>>> path is reachable from an unprivileged ioctl, so it does not warn.
>>>
>>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>>> ---
>>>    drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
>>> index 1f190b319bb..b66ec9e4828 100644
>>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>>> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>>>          dma_resv_assert_held(obj->resv);
>>>    -    if (is_import_bo(abo))
>>> +    if (is_import_bo(abo)) {
>>>            ret = dma_buf_vmap(abo->dma_buf, map);
> Mhm, why does amdxdna a vmap in the first place? For some workaround?

This is used for flushing the imported BO before. Based on our 
discussion before, the driver should not flush imported BO, so this 
becomes a invalid case.

On the other hand, vmap on a io_mem should not happen. So I suggested to 
move the check to amdxdna_gem_vmap() for an extra check.


Thanks,

Lizhi

>
> Usually DMA-buf only provides that framebuffer emulation scanout inside the kernel.
>
> On the other hand as far as I can see that here should work correctly.
>
> Regards,
> Christian.
>
>>> -    else
>>> +        /* Callers use mem.kva as an ordinary kernel address. */
>>> +        if (!ret && map->is_iomem) {
>>> +            dma_buf_vunmap(abo->dma_buf, map);
>>> +            return -EOPNOTSUPP;
>>> +        }
>> Thanks for the fix. The 'is_iomem' check should be moved to amdxdna_gem_vmap() to cover all the cases.
>>
>> Lizhi
>>
>>> +    } else {
>>>            ret = drm_gem_shmem_object_vmap(obj, map);
>>> +    }
>>>        if (ret)
>>>            return ret;
>>>        if (!map->vaddr)

Re: [PATCH v3 1/3] accel/amdxdna: refuse an I/O memory mapping of an imported BO

[Christian König]

On 8/24/26 18:08, Lizhi Hou wrote:
> On 8/24/26 07:06, Christian König wrote:
>> On 8/17/26 19:53, Lizhi Hou wrote:
>>> On 8/13/26 09:46, Taimuraz Kaitmazov wrote:
>>>> amdxdna_gem_obj_vmap() takes whatever dma_buf_vmap() returns and only
>>>> rejects a NULL vaddr. iosys_map is discriminated by is_iomem, so an
>>>> exporter answering with an I/O mapping leaves a void __iomem pointer in
>>>> abo->mem.kva, which amdxdna_cmd_set_error() memsets and memcpys through.
>>>>
>>>> amdxdna_drm_va_tbl takes a dmabuf_fd, so such a BO can be any exporter's
>>>> buffer. amdgpu cannot reach this: its .pin forces GTT for a non peer to
>>>> peer attachment like ours. An exporter on drm_gem_prime_dmabuf_ops has
>>>> no .pin, and drm_gem_ttm_vmap() answers iomem for a VRAM resident
>>>> object, so an NPU paired with nouveau or radeon does.
>>>>
>>>> Refuse the mapping. vmw_gem_vmap() does the same; unlike that one this
>>>> path is reachable from an unprivileged ioctl, so it does not warn.
>>>>
>>>> Signed-off-by: Taimuraz Kaitmazov <taimuraz@kaitmazov.com>
>>>> ---
>>>>    drivers/accel/amdxdna/amdxdna_gem.c | 10 ++++++++--
>>>>    1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/accel/amdxdna/amdxdna_gem.c b/drivers/accel/amdxdna/amdxdna_gem.c
>>>> index 1f190b319bb..b66ec9e4828 100644
>>>> --- a/drivers/accel/amdxdna/amdxdna_gem.c
>>>> +++ b/drivers/accel/amdxdna/amdxdna_gem.c
>>>> @@ -683,10 +683,16 @@ static int amdxdna_gem_obj_vmap(struct drm_gem_object *obj, struct iosys_map *ma
>>>>          dma_resv_assert_held(obj->resv);
>>>>    -    if (is_import_bo(abo))
>>>> +    if (is_import_bo(abo)) {
>>>>            ret = dma_buf_vmap(abo->dma_buf, map);
>> Mhm, why does amdxdna a vmap in the first place? For some workaround?
> 
> This is used for flushing the imported BO before. Based on our discussion before, the driver should not flush imported BO, so this becomes a invalid case.
> 
> On the other hand, vmap on a io_mem should not happen. So I suggested to move the check to amdxdna_gem_vmap() for an extra check.

Oh my, you should not use a vmap to flush CPU caches even if you own that GEM buffer! That is as buggy as it can be and most likely doesn't work 100% reliable.

What is the complete command submission flow here?

Regards,
Christian.

> 
> 
> Thanks,
> 
> Lizhi
> 
>>
>> Usually DMA-buf only provides that framebuffer emulation scanout inside the kernel.
>>
>> On the other hand as far as I can see that here should work correctly.
>>
>> Regards,
>> Christian.
>>
>>>> -    else
>>>> +        /* Callers use mem.kva as an ordinary kernel address. */
>>>> +        if (!ret && map->is_iomem) {
>>>> +            dma_buf_vunmap(abo->dma_buf, map);
>>>> +            return -EOPNOTSUPP;
>>>> +        }
>>> Thanks for the fix. The 'is_iomem' check should be moved to amdxdna_gem_vmap() to cover all the cases.
>>>
>>> Lizhi
>>>
>>>> +    } else {
>>>>            ret = drm_gem_shmem_object_vmap(obj, map);
>>>> +    }
>>>>        if (ret)
>>>>            return ret;
>>>>        if (!map->vaddr)