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)

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

[Lizhi Hou]

On 8/26/26 01:37, Christian König wrote:
> 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.

We know that it would be problem on non-x86 architectures. And currently 
the device is X86 only. Is flushing through vmap buggy on x86 as well?

To make the flush running on all architectures,  I can remove the vmap 
completely here and only use drm_clflush_pages. Does this sound right?

>
> What is the complete command submission flow here?

So the submission flow is roughly:  alloc bo -> mmap BO -> write BO 
content (and flush CPU cache by user space runtime) -> submit/wait io 
command.


Thanks,

Lizhi

>
> 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)