| Message ID | 20191129135908.2439529-7-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | panfrost: Fixes for 5.4 | expand |
On Fri, 29 Nov 2019 14:59:06 +0100 Boris Brezillon <boris.brezillon@collabora.com> 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); > + 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. ^s/adding the mapping/exporting it/ > + * 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,
On 29/11/2019 13:59, Boris Brezillon wrote: > We don't want imported/exported BOs to be purges, as those are shared s/purges/purged/ > 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); > + if (gem_obj->dma_buf) > + ret = -EINVAL; > + else > + ret = 0; > + > + if (ret) > + goto out_unlock_object_name; > + This might be better by setting ret = 0 at the beginning of the function. This can then be re-written as the more normal: if (gem_obj->dma_buf) { ret = -EINVAL; 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); No need for ret here: if (bo->base.madv == PANFROST_MADV_WILLNEED) return ERR_PTR(-EINVAL); Steve > + > + 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, >
On Fri, 29 Nov 2019 14:45:41 +0000 Steven Price <steven.price@arm.com> wrote: > On 29/11/2019 13:59, Boris Brezillon wrote: > > We don't want imported/exported BOs to be purges, as those are shared > > s/purges/purged/ > > > 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); > > + if (gem_obj->dma_buf) > > + ret = -EINVAL; > > + else > > + ret = 0; > > + > > + if (ret) > > + goto out_unlock_object_name; > > + > > This might be better by setting ret = 0 at the beginning of the > function. This can then be re-written as the more normal: > > if (gem_obj->dma_buf) { > ret = -EINVAL; > goto out_unlock_object_name; > ] Agreed. > > > 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); > > No need for ret here: > > if (bo->base.madv == PANFROST_MADV_WILLNEED) > return ERR_PTR(-EINVAL); This if/else section was previously surrounded by a lock/unlock, hence the convoluted logic. I'll rework as suggested. Thanks.
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. 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. > > 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.
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. > > > > > 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? -Daniel
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.
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); + 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,
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(-)