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