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@xxxxxxxxxxxxxxx>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> 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.
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.
-Daniel
> + if (gem_obj->dma_buf)
> + ret = -EINVAL;
> + else
> + ret = 0;
> +
> + if (ret)
> + goto out_unlock_object_name;
> +
> mutex_lock(&pfdev->shrinker_lock);
> args->retained = drm_gem_shmem_madvise(gem_obj, args->madv);
>
> @@ -364,8 +378,11 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
> }
> mutex_unlock(&pfdev->shrinker_lock);
>
> +out_unlock_object_name:
> + mutex_unlock(&dev->object_name_lock);
> +
> drm_gem_object_put_unlocked(gem_obj);
> - return 0;
> + return ret;
> }
>
> int panfrost_unstable_ioctl_check(void)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 92a95210a899..31d6417dd21c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -99,6 +99,32 @@ void panfrost_gem_close(struct drm_gem_object *obj, struct drm_file *file_priv)
> spin_unlock(&priv->mm_lock);
> }
>
> +static struct dma_buf *
> +panfrost_gem_export(struct drm_gem_object *obj, int flags)
> +{
> + struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> + int ret;
> +
> + /*
> + * We must make sure the BO has not been marked purgeable/purged before
> + * adding the mapping.
> + * Note that we don't need to protect this test with a lock because
> + * &drm_gem_object_funcs.export() is called with
> + * &drm_device.object_lock held, and panfrost_ioctl_madvise() takes
> + * this lock before calling drm_gem_shmem_madvise() (the function that
> + * modifies bo->base.madv).
> + */
> + if (bo->base.madv == PANFROST_MADV_WILLNEED)
> + ret = -EINVAL;
> + else
> + ret = 0;
> +
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return drm_gem_prime_export(obj, flags);
> +}
> +
> static int panfrost_gem_pin(struct drm_gem_object *obj)
> {
> if (to_panfrost_bo(obj)->is_heap)
> @@ -112,6 +138,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
> .open = panfrost_gem_open,
> .close = panfrost_gem_close,
> .print_info = drm_gem_shmem_print_info,
> + .export = panfrost_gem_export,
> .pin = panfrost_gem_pin,
> .unpin = drm_gem_shmem_unpin,
> .get_sg_table = drm_gem_shmem_get_sg_table,
> --
> 2.23.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
