[RFC] dma-buf: Import/export the implicit fences on the dma-buf

[Christian König]

Am 11.07.2016 um 23:59 schrieb Chris Wilson:
> When dealing with user interfaces that utilize explicit fences, it is
> convenient to sometimes create those fences after the fact, i.e. to
> query the dma-buf for the current set of implicit fences, encapsulate
> those into a sync_file and hand that fd back to userspace.
> Correspondingly, being able to add an explicit fence back into the mix
> of fences being tracked by the dma-buf allows that userspace fence to be
> included in any implicit tracking.

Well I think that this is a rather questionable interface.

For example how do you deal with race conditions? E.g. between querying 
the fences from the reservation object and adding a new one we could 
gain new fences because of the kernel swapping things out or another 
thread making some submission with this buffer.

Additional to that IIRC reservation_object_add_excl_fence() currently 
replaces all shared fences with the exclusive one. A malicious 
application could use this to trick the kernel driver into thinking that 
this buffer object is idle while it is still accessed by some operation. 
At least with GPU operations you can easily take over the system when 
you manage to get access to a page table with this.

Regards,
Christian.

>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Cc: Rob Clark <robdclark at gmail.com>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> ---
>
> Gustavo, could you look at the sync-file/fence-array handling? There's
> definitely room for a cleaner sync_file_create() API.
>
> I was thinking about this for the "why not sync-file" question Gustavo
> posed for the vgem_fences. I wanted to add an ioctl to the vgem to allow
> exporting and creating fences from sync-file for testing passing
> explicit userspaces around between drivers, and realised that I was just
> writing a generic mechanism that only required dma-buf.
>
> Whilst this interface could be used for lazily creating explicit fences,
> drivers will also likely want to support specific ioctl to skip the
> dmabuf creation, I think this interfaces will be useful with the vgem
> fences for testing sync_file handling in drivers (on i915 at the moment,
> my tests for sync_file involve sleeping and a few white lies). So
> fulfilling a similar role in driver testing to debugfs/sw_sync?
> (sw_sync is still more apt for testing timelines etc, vgem feels more
> apt for ease of testing rendering.)
> -Chris
>
> ---
>   drivers/dma-buf/dma-buf.c    | 100 +++++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/dma-buf.h |   9 ++++
>   2 files changed, 109 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 41fbce0c273a..6f066a8affd7 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -26,11 +26,13 @@
>   #include <linux/slab.h>
>   #include <linux/dma-buf.h>
>   #include <linux/fence.h>
> +#include <linux/fence-array.h>
>   #include <linux/anon_inodes.h>
>   #include <linux/export.h>
>   #include <linux/debugfs.h>
>   #include <linux/module.h>
>   #include <linux/seq_file.h>
> +#include <linux/sync_file.h>
>   #include <linux/poll.h>
>   #include <linux/reservation.h>
>   #include <linux/mm.h>
> @@ -254,6 +256,97 @@ out:
>   	return events;
>   }
>   
> +static long dma_buf_import_fence_ioctl(struct dma_buf *dmabuf,
> +				       struct dma_buf_fence  __user *arg)
> +{
> +	struct reservation_object *resv = dmabuf->resv;
> +	struct dma_buf_fence cmd;
> +	struct fence *fence;
> +	int ret;
> +
> +	if (copy_from_user(&cmd, arg, sizeof(cmd)))
> +		return -EFAULT;
> +
> +	fence = NULL;
> +	//fence = sync_file_get_fence(cmd.fd);
> +	if (!fence)
> +		return -EINVAL;
> +
> +	mutex_lock(&resv->lock.base);
> +	if (cmd.flags & DMA_BUF_FENCE_WRITE)
> +		reservation_object_add_excl_fence(resv, fence);
> +	else if ((ret = reservation_object_reserve_shared(resv)) == 0)
> +		reservation_object_add_shared_fence(resv, fence);
> +	mutex_unlock(&resv->lock.base);
> +
> +	fence_put(fence);
> +	return ret;
> +}
> +
> +static long dma_buf_export_fence_ioctl(struct dma_buf *dmabuf,
> +				       struct dma_buf_fence  __user *arg)
> +{
> +	struct reservation_object *resv = dmabuf->resv;
> +	struct dma_buf_fence cmd;
> +	struct fence *excl, **shared;
> +	struct sync_file *sync = NULL;
> +	unsigned int count, n;
> +	int ret;
> +
> +	if (get_user(cmd.flags, &arg->flags))
> +		return -EFAULT;
> +
> +	ret = reservation_object_get_fences_rcu(resv, &excl, &count, &shared);
> +	if (ret)
> +		return ret;
> +
> +	if (cmd.flags & DMA_BUF_FENCE_WRITE) {
> +		if (excl) {
> +			sync = sync_file_create(excl);
> +			if (!sync) {
> +				ret = -ENOMEM;
> +				fence_put(excl);
> +			}
> +		}
> +		for (n = 0; n < count; n++)
> +			fence_put(shared[n]);
> +		kfree(shared);
> +	} else {
> +		if (count) {
> +			struct fence_array *array;
> +
> +			array = fence_array_create(count, shared, 0, 0, false);
> +			if (!array) {
> +				for (n = 0; n < count; n++)
> +					fence_put(shared[n]);
> +				kfree(shared);
> +			} else
> +				sync = sync_file_create(&array->base);
> +			if (!sync) {
> +				ret = -ENOMEM;
> +				fence_put(&array->base);
> +			}
> +		}
> +		fence_put(excl);
> +	}
> +	if (ret)
> +		return ret;
> +
> +	cmd.fd = get_unused_fd_flags(O_CLOEXEC);
> +	if (cmd.fd < 0) {
> +		fput(sync->file);
> +		return cmd.fd;
> +	}
> +
> +	if (put_user(cmd.fd, &arg->fd)) {
> +		fput(sync->file);
> +		return -EFAULT;
> +	}
> +
> +	fd_install(cmd.fd, sync->file);
> +	return 0;
> +}
> +
>   static long dma_buf_ioctl(struct file *file,
>   			  unsigned int cmd, unsigned long arg)
>   {
> @@ -292,6 +385,13 @@ static long dma_buf_ioctl(struct file *file,
>   			ret = dma_buf_begin_cpu_access(dmabuf, direction);
>   
>   		return ret;
> +
> +	case DMA_BUF_IOCTL_IMPORT_FENCE:
> +		return dma_buf_import_fence_ioctl(dmabuf, (void __user *)arg);
> +
> +	case DMA_BUF_IOCTL_EXPORT_FENCE:
> +		return dma_buf_export_fence_ioctl(dmabuf, (void __user *)arg);
> +
>   	default:
>   		return -ENOTTY;
>   	}
> diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
> index fb0dedb7c121..8d9a0d73ebaa 100644
> --- a/include/uapi/linux/dma-buf.h
> +++ b/include/uapi/linux/dma-buf.h
> @@ -37,4 +37,13 @@ struct dma_buf_sync {
>   #define DMA_BUF_BASE		'b'
>   #define DMA_BUF_IOCTL_SYNC	_IOW(DMA_BUF_BASE, 0, struct dma_buf_sync)
>   
> +struct dma_buf_fence {
> +	__s32 fd;
> +	__u32 flags;
> +#define DMA_BUF_FENCE_WRITE	(1 << 0)
> +};
> +
> +#define DMA_BUF_IOCTL_IMPORT_FENCE _IOW(DMA_BUF_BASE, 1, struct dma_buf_fence)
> +#define DMA_BUF_IOCTL_EXPORT_FENCE _IOWR(DMA_BUF_BASE, 2, struct dma_buf_fence)
> +
>   #endif