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