Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs

On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
[off-list ref] wrote:

On 8/10/22 18:08, Rob Clark wrote:

quoted

On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter [off-list ref] wrote:

quoted

On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:

quoted

On 7/6/22 00:48, Rob Clark wrote:

quoted

On Tue, Jul 5, 2022 at 4:51 AM Christian König [off-list ref] wrote:

quoted

Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:

quoted

Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
handle imported dma-bufs properly, which results in mapping of something
else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
userspace writes to the memory mapping of a dma-buf that was imported into
Tegra's DRM GEM.

Majority of DRM drivers prohibit mapping of the imported GEM objects.
Mapping of imported GEMs require special care from userspace since it
should sync dma-buf because mapping coherency of the exporter device may
not match the DRM device. Let's prohibit the mapping for all DRM drivers
for consistency.

Suggested-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I'm pretty sure that this is the right approach, but it's certainly more
than possible that somebody abused this already.

I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc.  And then when you
hit readpix...

You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?

Given that that only drivers which use DRM-SHMEM potentially could've
map imported dma-bufs (Panfrost, Lima) and they already don't allow to
do that, I think we're good.

So can I have an ack from Rob here or are there still questions that this
might go boom?

Dmitry, since you have a bunch of patches merged now I think would also be
good to get commit rights so you can drive this more yourself. I've asked
Daniel Stone to help you out with getting that.

I *think* we'd be ok with this on msm, mostly just by dumb luck.
Because the dma-buf's we import will be self-import.  I'm less sure
about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
special path for imported dma-bufs either, and in that case they won't
be self-imports.. but I guess no one has tried to run android cts on
panfrost).

The last time I tried to mmap dma-buf imported to Panfrost didn't work
because Panfrost didn't implement something needed for that. I'll need
to take a look again because can't recall what it was.

quoted

What about something less drastic to start, like (apologies for
hand-edited patch):

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c

@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object

*obj, unsigned long obj_size,
 {
        int ret;

+       WARN_ON_ONCE(obj->import_attach);

This will hang NVIDIA Tegra, which is what this patch fixed initially.
If neither of upstream DRM drivers need to map imported dma-bufs and
never needed, then why do we need this?

oh, tegra isn't using shmem helpers?  I assumed it was.  Well my point
was to make a more targeted fail on tegra, and a WARN_ON for everyone
else to make it clear that what they are doing is undefined behavior.
Because so far existing userspace (or well, panfrost and freedreno at
least, those are the two I know or checked) don't make special cases
for mmap'ing against the dmabuf fd against the dmabuf fd instead of
the drm device fd.

I *think* it should work out that we don't hit this path with
freedreno but on android I can't really guarantee or prove it.  So
your patch would potentially break existing working userspace.  Maybe
it is userspace that isn't portable (but OTOH it isn't like you are
going to be using freedreno on tegra).  So why don't you go for a more
targeted fix that only returns an error on hw where this is
problematic?

BR,
-R
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization