Messages in this thread | | | Subject | Re: [PATCH] dma-buf: simplified calling conventions for dma_buf_fd() | From | Laura Abbott <> | Date | Wed, 23 May 2018 12:00:55 -0700 |
| |
On 05/22/2018 01:09 AM, Jens Wiklander wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Make dma_buf_fd() drop the file reference on failure, folding > dma_buf_put() into it. > > Users of dma_buf_fd() (drm, videobuf2, ion and tee) are updated to take > the new calling convention into account. >
For the Ion portion,
Acked-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > [jw: rebased and updated commit message] > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > > I received this patch as a response to what has become > bb765d1c331f ("tee: shm: fix use-after-free via temporarily dropped reference") > > drivers/dma-buf/dma-buf.c | 16 +++++---- > drivers/gpu/drm/drm_prime.c | 15 +++----- > drivers/gpu/drm/i915/gvt/dmabuf.c | 4 +-- > drivers/gpu/drm/ttm/ttm_object.c | 3 +- > .../media/common/videobuf2/videobuf2-core.c | 14 ++++---- > drivers/staging/android/ion/ion.c | 7 +--- > drivers/tee/tee_core.c | 35 ++++++------------- > drivers/tee/tee_shm.c | 8 +---- > 8 files changed, 36 insertions(+), 66 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index d78d5fc173dc..78981928758e 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -471,17 +471,21 @@ EXPORT_SYMBOL_GPL(dma_buf_export); > */ > int dma_buf_fd(struct dma_buf *dmabuf, int flags) > { > + struct file *file; > int fd; > > - if (!dmabuf || !dmabuf->file) > + if (!dmabuf) > return -EINVAL; > > - fd = get_unused_fd_flags(flags); > - if (fd < 0) > - return fd; > - > - fd_install(fd, dmabuf->file); > + file = dmabuf->file; > + if (!file) > + return -EINVAL; > > + fd = get_unused_fd_flags(flags); > + if (fd >= 0) > + fd_install(fd, file); > + else > + fput(file); > return fd; > } > EXPORT_SYMBOL_GPL(dma_buf_fd); > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 7856a9b3f8a8..06d1b7d3751a 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -662,8 +662,10 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > ret = drm_prime_add_buf_handle(&file_priv->prime, > dmabuf, handle); > mutex_unlock(&dev->object_name_lock); > - if (ret) > - goto fail_put_dmabuf; > + if (unlikely(ret)) { > + dma_buf_put(dmabuf); > + goto out; > + } > > out_have_handle: > ret = dma_buf_fd(dmabuf, flags); > @@ -673,17 +675,10 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, > * and that is invariant as long as a userspace gem handle exists. > * Closing the handle will clean out the cache anyway, so we don't leak. > */ > - if (ret < 0) { > - goto fail_put_dmabuf; > - } else { > + if (ret >= 0) { > *prime_fd = ret; > ret = 0; > } > - > - goto out; > - > -fail_put_dmabuf: > - dma_buf_put(dmabuf); > out: > drm_gem_object_put_unlocked(obj); > out_unlock: > diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c > index 6f4f8e941fc2..1783c724d96c 100644 > --- a/drivers/gpu/drm/i915/gvt/dmabuf.c > +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c > @@ -479,7 +479,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned int dmabuf_id) > ret = dma_buf_fd(dmabuf, DRM_CLOEXEC | DRM_RDWR); > if (ret < 0) { > gvt_vgpu_err("create dma-buf fd failed ret:%d\n", ret); > - goto out_free_dmabuf; > + goto out_free_gem; > } > dmabuf_fd = ret; > > @@ -502,8 +502,6 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu, unsigned int dmabuf_id) > > return dmabuf_fd; > > -out_free_dmabuf: > - dma_buf_put(dmabuf); > out_free_gem: > i915_gem_object_put(obj); > out: > diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c > index 1aa2baa83959..30d29d17653f 100644 > --- a/drivers/gpu/drm/ttm/ttm_object.c > +++ b/drivers/gpu/drm/ttm/ttm_object.c > @@ -730,8 +730,7 @@ int ttm_prime_handle_to_fd(struct ttm_object_file *tfile, > if (ret >= 0) { > *prime_fd = ret; > ret = 0; > - } else > - dma_buf_put(dma_buf); > + } > > out_unref: > if (base) > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index d3f7bb33a54d..3baf689241cc 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -1881,15 +1881,13 @@ int vb2_core_expbuf(struct vb2_queue *q, int *fd, unsigned int type, > if (ret < 0) { > dprintk(3, "buffer %d, plane %d failed to export (%d)\n", > index, plane, ret); > - dma_buf_put(dbuf); > - return ret; > + } else { > + dprintk(3, "buffer %d, plane %d exported as %d descriptor\n", > + index, plane, ret); > + *fd = ret; > + ret = 0; > } > - > - dprintk(3, "buffer %d, plane %d exported as %d descriptor\n", > - index, plane, ret); > - *fd = ret; > - > - return 0; > + return ret; > } > EXPORT_SYMBOL_GPL(vb2_core_expbuf); > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > index e74db7902549..c9fcb2a2f518 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -381,7 +381,6 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags) > struct ion_buffer *buffer = NULL; > struct ion_heap *heap; > DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > - int fd; > struct dma_buf *dmabuf; > > pr_debug("%s: len %zu heap_id_mask %u flags %x\n", __func__, > @@ -425,11 +424,7 @@ int ion_alloc(size_t len, unsigned int heap_id_mask, unsigned int flags) > return PTR_ERR(dmabuf); > } > > - fd = dma_buf_fd(dmabuf, O_CLOEXEC); > - if (fd < 0) > - dma_buf_put(dmabuf); > - > - return fd; > + return dma_buf_fd(dmabuf, O_CLOEXEC); > } > > int ion_query_heaps(struct ion_heap_query *query) > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > index dd46b758852a..1cb54f6f3c6a 100644 > --- a/drivers/tee/tee_core.c > +++ b/drivers/tee/tee_core.c > @@ -125,7 +125,6 @@ static int tee_ioctl_version(struct tee_context *ctx, > static int tee_ioctl_shm_alloc(struct tee_context *ctx, > struct tee_ioctl_shm_alloc_data __user *udata) > { > - long ret; > struct tee_ioctl_shm_alloc_data data; > struct tee_shm *shm; > > @@ -144,25 +143,18 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx, > data.flags = shm->flags; > data.size = shm->size; > > - if (copy_to_user(udata, &data, sizeof(data))) > - ret = -EFAULT; > - else > - ret = tee_shm_get_fd(shm); > + if (unlikely(copy_to_user(udata, &data, sizeof(data)))) { > + tee_shm_put(shm); > + return -EFAULT; > + } > > - /* > - * When user space closes the file descriptor the shared memory > - * should be freed or if tee_shm_get_fd() failed then it will > - * be freed immediately. > - */ > - tee_shm_put(shm); > - return ret; > + return tee_shm_get_fd(shm); > } > > static int > tee_ioctl_shm_register(struct tee_context *ctx, > struct tee_ioctl_shm_register_data __user *udata) > { > - long ret; > struct tee_ioctl_shm_register_data data; > struct tee_shm *shm; > > @@ -182,17 +174,12 @@ tee_ioctl_shm_register(struct tee_context *ctx, > data.flags = shm->flags; > data.length = shm->size; > > - if (copy_to_user(udata, &data, sizeof(data))) > - ret = -EFAULT; > - else > - ret = tee_shm_get_fd(shm); > - /* > - * When user space closes the file descriptor the shared memory > - * should be freed or if tee_shm_get_fd() failed then it will > - * be freed immediately. > - */ > - tee_shm_put(shm); > - return ret; > + if (unlikely(copy_to_user(udata, &data, sizeof(data)))) { > + tee_shm_put(shm); > + return -EFAULT; > + } > + > + return tee_shm_get_fd(shm); > } > > static int params_from_user(struct tee_context *ctx, struct tee_param *params, > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > index 07d3be6f0780..53b35b5e5776 100644 > --- a/drivers/tee/tee_shm.c > +++ b/drivers/tee/tee_shm.c > @@ -355,16 +355,10 @@ EXPORT_SYMBOL_GPL(tee_shm_register); > */ > int tee_shm_get_fd(struct tee_shm *shm) > { > - int fd; > - > if (!(shm->flags & TEE_SHM_DMA_BUF)) > return -EINVAL; > > - get_dma_buf(shm->dmabuf); > - fd = dma_buf_fd(shm->dmabuf, O_CLOEXEC); > - if (fd < 0) > - dma_buf_put(shm->dmabuf); > - return fd; > + return dma_buf_fd(shm->dmabuf, O_CLOEXEC); > } > > /** >
| |