On Mon, 2 Dec 2019 09:52:43 +0100 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 29, 2019 at 10:09:24PM +0100, Boris Brezillon wrote:
On Fri, 29 Nov 2019 21:12:13 +0100 Daniel Vetter daniel@ffwll.ch wrote:
On Fri, Nov 29, 2019 at 02:59:06PM +0100, Boris Brezillon wrote:
We don't want imported/exported BOs to be purges, as those are shared with other processes that might still use them. We should also refuse to export a BO if it's been marked purgeable or has already been purged.
Fixes: 013b65101315 ("drm/panfrost: Add madvise and shrinker support") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
drivers/gpu/drm/panfrost/panfrost_drv.c | 19 ++++++++++++++++- drivers/gpu/drm/panfrost/panfrost_gem.c | 27 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 1c67ac434e10..751df975534f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -343,6 +343,7 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, struct drm_panfrost_madvise *args = data; struct panfrost_device *pfdev = dev->dev_private; struct drm_gem_object *gem_obj;
- int ret;
gem_obj = drm_gem_object_lookup(file_priv, args->handle); if (!gem_obj) { @@ -350,6 +351,19 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, return -ENOENT; }
- /*
* We don't want to mark exported/imported BOs as purgeable: we're not* the only owner in that case.*/- mutex_lock(&dev->object_name_lock);
Kinda not awesome that you have to take this core lock here and encumber core drm locking with random driver stuff.
Looks like drm_gem_shmem_is_purgeable() already does the !imported && !exported check. For the imported case, I think we're good, since userspace can't change the madv value before ->import_attach has been set. For the exporter case, we need to make sure there's no race between the export and madvise operations, which I can probably do from the gem->export() hook by taking the shrinker or bo->mappings lock.
Okay, I tried that, and I actually need an extra panfrost_gem_object->exported field that's set to true from the ->export() hook with the mappings lock held, otherwise the code is still racy (->dma_buf is assigned after ->export() returns).
Can't this be solved with your own locking only and some reasonable ordering of checks? big locks because it's easy is endless long-term pain.
Also exporting purgeable objects is kinda a userspace bug, all you have to do is not oops in dma_buf_attachment_map. No need to prevent the damage here imo.
I feel like making sure an exported BO can't be purged or a purged BO can't be exported would be much simpler than making sure all importers are ready to have the sgt freed.
If you free the sgt while someone is using it, that's kinda a different bug I think. You already have a pages refcount, that should be enough to prevent this?
My bad, I thought drm_gem_shmem_get_pages_sgt() was used as the ->get_sg_table() implem, but we actually use drm_gem_shmem_get_sg_table() which allocates a new sgt. I still need to make sure we're safe against sgt destruction in panfrost.