[PATCH v4 0/5] accel/amdxdna: SYNC_BO correctness fixes

Taimuraz Kaitmazov <[email protected]>
Newsgroups org.freedesktop.lists.dri-devel
Message-ID <[email protected]>
Five fixes in and around amdxdna_drm_sync_bo_ioctl().

Patch 1 refuses an I/O memory mapping the driver would otherwise store as
if it were an ordinary kernel address. Patch 2 checks the device-BO range
for overflow. Patch 4 refuses a flush of an imported BO, which is why
patch 3 comes first: the ioctl answers a rejected flush with drm_WARN(),
so without it an ordinary sync on an imported BO splats. Patch 5 stops
the ioctl reporting failure for a flush that succeeded.

v3's zero-length patch has left this series. On hardware it turns out to
be a page fault in drm_clflush_virt_range() rather than the tidy-up its
commit message described, so it is a -fixes patch now, sent separately as
"accel/amdxdna: return early from a zero-length flush" with Fixes: and
Cc: stable. Patch 4 here needs its hunk, so this series wants that one
first.

Changes in v4:
  - patch 1: the is_iomem check moved from the .vmap callback into
    amdxdna_gem_vmap(), per Lizhi, and it logs at debug level rather than
    error, since an unprivileged caller can repeat it.
  - new patch 3: an unprivileged SYNC_BO with an out-of-range offset
    already reaches that drm_WARN() today. Sashiko's review of v3 3/3
    flagged the same thing.
  - new patch 4: refuses the flush for every imported BO, as asked.
  - new patch 5: the debug-BO sync I mentioned on the v2 thread. Only a BO
    attached with ATTACH_DEBUG_BO has an assigned hwctx, so every other
    FROM_DEVICE sync ends in -EINVAL with its flush already done. The
    -EINVAL reproduces on a Strix Point NPU.

On patch 4, one consequence worth deciding before it lands. A heap BO is
created through amdxdna_drm_create_share_bo(), so a device running on
carveout memory reaches its heap through a cbuf, is_import_bo() is true of
it, and SYNC_BO on every AMDXDNA_BO_DEV now answers -EOPNOTSUPP. Today
that path flushes nothing anyway -- amdxdna_cbuf_map() fills in only the
DMA address and length, so drm_clflush_sg() walks zero pages -- so the
change is silent no-op to hard error, and XRT's dbg_buffer::sync() reaches
it without Debug.force_driver_sync. If you would rather keep our own
exporters working, I have the variant keyed on the exporter's ops, which
confines the refusal to foreign buffers. Say which you prefer.

v3: https://lore.kernel.org/all/[email protected]/

Built on drm-misc-next, each commit on its own: x86_64 with
DRM_ACCEL_AMDXDNA=m, clang 22.1.8, no warnings. Patch 5's reproducer was
run against 7.1.8's in-tree driver, where that call is unchanged.

Taimuraz Kaitmazov (5):
  accel/amdxdna: refuse an I/O memory mapping of an imported BO
  accel/amdxdna: check the sync range for overflow on a device BO
  accel/amdxdna: do not warn when a sync request is rejected
  accel/amdxdna: refuse to flush an imported BO
  accel/amdxdna: do not fail a sync for a BO with no debug context

 drivers/accel/amdxdna/amdxdna_ctx.c |  3 ++-
 drivers/accel/amdxdna/amdxdna_gem.c | 27 +++++++++++++++++++--------
 2 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.