Re: [PATCH v10 3/3] staging/android: refactor SYNC IOCTLs

[Gustavo Padovan]

Hi Greg,

Any comment on this?

Thanks,

Gustavo

2016-03-18 Gustavo Padovan <gustavo@xxxxxxxxxxx>:

> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> 
> Change SYNC_IOC_FILE_INFO (former SYNC_IOC_FENCE_INFO) behaviour to avoid
> future API breaks and optimize buffer allocation.
> 
> Now num_fences can be filled by the caller to inform how many fences it
> wants to retrieve from the kernel. If the num_fences passed is greater
> than zero info->sync_fence_info should point to a buffer with enough space
> to fit all fences.
> 
> However if num_fences passed to the kernel is 0, the kernel will reply
> with number of fences of the sync_file.
> 
> Sending first an ioctl with num_fences = 0 can optimize buffer allocation,
> in a first call with num_fences = 0 userspace will receive the actual
> number of fences in the num_fences filed.
> 
> Then it can allocate a buffer with the correct size on sync_fence_info and
> call SYNC_IOC_FILE_INFO again, but now with the actual value of num_fences
> in the sync_file.
> 
> info->sync_fence_info was converted to __u64 pointer to prevent 32bit
> compatibility issues. And a flags member was added.
> 
> An example userspace code for the later would be:
> 
> 	struct sync_file_info *info;
> 	int err, size, num_fences;
> 
> 	info = malloc(sizeof(*info));
> 
> 	info.flags = 0;
> 	err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> 	num_fences = info->num_fences;
> 
> 	if (num_fences) {
> 		info.flags = 0;
> 		size = sizeof(struct sync_fence_info) * num_fences;
> 		info->num_fences = num_fences;
> 		info->sync_fence_info = (uint64_t) calloc(num_fences,
> 							  sizeof(struct sync_fence_info));
> 
> 		err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> 	}
> 
> Finally the IOCTLs numbers were changed to avoid any potential old
> userspace running the old API to get weird errors. Changing the opcodes
> will make them fail right away. This is just a precaution, there no
> upstream users of these interfaces yet and the only user is Android, but
> we don't expect anyone trying to run android userspace and all it
> dependencies on top of upstream kernels.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Acked-by: Greg Hackmann <ghackmann@xxxxxxxxxx>
> Acked-by: Rob Clark <robdclark@xxxxxxxxx>
> Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> 
> ---
> v2: fix fence_info memory leak
> 
> v3: Comments from Emil Velikov
> 	- improve commit message
> 	- remove __u64 cast
> 	- remove check for output fields in file_info
> 	- clean up sync_fill_fence_info()
> 
>     Comments from Maarten Lankhorst
> 	- remove in.num_fences && !in.sync_fence_info check
> 	- remove info->len and use only num_fences to calculate size
> 
>     Comments from Dan Carpenter
> 	- fix info->sync_fence_info documentation
> 
> v4: remove allocated struct sync_file_info (comment from Maarten)
> 
> v5: merge all commits that were changing the ABI
> 
> v6: fix -Wint-to-pointer-cast error on info.sync_fence_info
> ---
>  drivers/staging/android/sync.c      | 76 ++++++++++++++++++++-----------------
>  drivers/staging/android/uapi/sync.h | 36 +++++++++++++-----
>  2 files changed, 67 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index 3a8f210..f9c6094 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -445,6 +445,11 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>  		goto err_put_fd;
>  	}
>  
> +	if (data.flags || data.pad) {
> +		err = -EINVAL;
> +		goto err_put_fd;
> +	}
> +
>  	fence2 = sync_file_fdget(data.fd2);
>  	if (!fence2) {
>  		err = -ENOENT;
> @@ -479,13 +484,9 @@ err_put_fd:
>  	return err;
>  }
>  
> -static int sync_fill_fence_info(struct fence *fence, void *data, int size)
> +static void sync_fill_fence_info(struct fence *fence,
> +				struct sync_fence_info *info)
>  {
> -	struct sync_fence_info *info = data;
> -
> -	if (size < sizeof(*info))
> -		return -ENOMEM;
> -
>  	strlcpy(info->obj_name, fence->ops->get_timeline_name(fence),
>  		sizeof(info->obj_name));
>  	strlcpy(info->driver_name, fence->ops->get_driver_name(fence),
> @@ -495,58 +496,63 @@ static int sync_fill_fence_info(struct fence *fence, void *data, int size)
>  	else
>  		info->status = 0;
>  	info->timestamp_ns = ktime_to_ns(fence->timestamp);
> -
> -	return sizeof(*info);
>  }
>  
>  static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>  					unsigned long arg)
>  {
> -	struct sync_file_info *info;
> +	struct sync_file_info info;
> +	struct sync_fence_info *fence_info = NULL;
>  	__u32 size;
> -	__u32 len = 0;
>  	int ret, i;
>  
> -	if (copy_from_user(&size, (void __user *)arg, sizeof(size)))
> +	if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
>  		return -EFAULT;
>  
> -	if (size < sizeof(struct sync_file_info))
> +	if (info.flags || info.pad)
>  		return -EINVAL;
>  
> -	if (size > 4096)
> -		size = 4096;
> -
> -	info = kzalloc(size, GFP_KERNEL);
> -	if (!info)
> -		return -ENOMEM;
> -
> -	strlcpy(info->name, sync_file->name, sizeof(info->name));
> -	info->status = atomic_read(&sync_file->status);
> -	if (info->status >= 0)
> -		info->status = !info->status;
> -
> -	len = sizeof(struct sync_file_info);
> +	/*
> +	 * Passing num_fences = 0 means that userspace doesn't want to
> +	 * retrieve any sync_fence_info. If num_fences = 0 we skip filling
> +	 * sync_fence_info and return the actual number of fences on
> +	 * info->num_fences.
> +	 */
> +	if (!info.num_fences)
> +		goto no_fences;
>  
> -	for (i = 0; i < sync_file->num_fences; ++i) {
> -		struct fence *fence = sync_file->cbs[i].fence;
> +	if (info.num_fences < sync_file->num_fences)
> +		return -EINVAL;
>  
> -		ret = sync_fill_fence_info(fence, (u8 *)info + len, size - len);
> +	size = sync_file->num_fences * sizeof(*fence_info);
> +	fence_info = kzalloc(size, GFP_KERNEL);
> +	if (!fence_info)
> +		return -ENOMEM;
>  
> -		if (ret < 0)
> -			goto out;
> +	for (i = 0; i < sync_file->num_fences; ++i)
> +		sync_fill_fence_info(sync_file->cbs[i].fence, &fence_info[i]);
>  
> -		len += ret;
> +	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
> +			 size)) {
> +		ret = -EFAULT;
> +		goto out;
>  	}
>  
> -	info->len = len;
> +no_fences:
> +	strlcpy(info.name, sync_file->name, sizeof(info.name));
> +	info.status = atomic_read(&sync_file->status);
> +	if (info.status >= 0)
> +		info.status = !info.status;
> +
> +	info.num_fences = sync_file->num_fences;
>  
> -	if (copy_to_user((void __user *)arg, info, len))
> +	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
>  		ret = -EFAULT;
>  	else
>  		ret = 0;
>  
>  out:
> -	kfree(info);
> +	kfree(fence_info);
>  
>  	return ret;
>  }
> @@ -560,7 +566,7 @@ static long sync_file_ioctl(struct file *file, unsigned int cmd,
>  	case SYNC_IOC_MERGE:
>  		return sync_file_ioctl_merge(sync_file, arg);
>  
> -	case SYNC_IOC_FENCE_INFO:
> +	case SYNC_IOC_FILE_INFO:
>  		return sync_file_ioctl_fence_info(sync_file, arg);
>  
>  	default:
> diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h
> index 4467c76..fbadb8a 100644
> --- a/drivers/staging/android/uapi/sync.h
> +++ b/drivers/staging/android/uapi/sync.h
> @@ -16,14 +16,18 @@
>  
>  /**
>   * struct sync_merge_data - data passed to merge ioctl
> - * @fd2:	file descriptor of second fence
>   * @name:	name of new fence
> + * @fd2:	file descriptor of second fence
>   * @fence:	returns the fd of the new fence to userspace
> + * @flags:	merge_data flags
> + * @pad:	padding for 64-bit alignment, should always be zero
>   */
>  struct sync_merge_data {
> -	__s32	fd2;
>  	char	name[32];
> +	__s32	fd2;
>  	__s32	fence;
> +	__u32	flags;
> +	__u32	pad;
>  };
>  
>  /**
> @@ -31,42 +35,54 @@ struct sync_merge_data {
>   * @obj_name:		name of parent sync_timeline
>   * @driver_name:	name of driver implementing the parent
>   * @status:		status of the fence 0:active 1:signaled <0:error
> + * @flags:		fence_info flags
>   * @timestamp_ns:	timestamp of status change in nanoseconds
>   */
>  struct sync_fence_info {
>  	char	obj_name[32];
>  	char	driver_name[32];
>  	__s32	status;
> +	__u32	flags;
>  	__u64	timestamp_ns;
>  };
>  
>  /**
>   * struct sync_file_info - data returned from fence info ioctl
> - * @len:	ioctl caller writes the size of the buffer its passing in.
> - *		ioctl returns length of sync_file_info returned to
> - *		userspace including pt_info.
>   * @name:	name of fence
>   * @status:	status of fence. 1: signaled 0:active <0:error
> - * @sync_fence_info: array of sync_fence_info for every fence in the sync_file
> + * @flags:	sync_file_info flags
> + * @num_fences	number of fences in the sync_file
> + * @pad:	padding for 64-bit alignment, should always be zero
> + * @sync_fence_info: pointer to array of structs sync_fence_info with all
> + *		 fences in the sync_file
>   */
>  struct sync_file_info {
> -	__u32	len;
>  	char	name[32];
>  	__s32	status;
> +	__u32	flags;
> +	__u32	num_fences;
> +	__u32	pad;
>  
> -	__u8	sync_fence_info[0];
> +	__u64	sync_fence_info;
>  };
>  
>  #define SYNC_IOC_MAGIC		'>'
>  
>  /**
> + * Opcodes  0, 1 and 2 were burned during a API change to avoid users of the
> + * old API to get weird errors when trying to handling sync_files. The API
> + * change happened during the de-stage of the Sync Framework when there was
> + * no upstream users available.
> + */
> +
> +/**
>   * DOC: SYNC_IOC_MERGE - merge two fences
>   *
>   * Takes a struct sync_merge_data.  Creates a new fence containing copies of
>   * the sync_pts in both the calling fd and sync_merge_data.fd2.  Returns the
>   * new fence's fd in sync_merge_data.fence
>   */
> -#define SYNC_IOC_MERGE		_IOWR(SYNC_IOC_MAGIC, 1, struct sync_merge_data)
> +#define SYNC_IOC_MERGE		_IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data)
>  
>  /**
>   * DOC: SYNC_IOC_FENCE_INFO - get detailed information on a fence
> @@ -79,6 +95,6 @@ struct sync_file_info {
>   * pt_info is a buffer containing sync_pt_infos for every sync_pt in the fence.
>   * To iterate over the sync_pt_infos, use the sync_pt_info.len field.
>   */
> -#define SYNC_IOC_FENCE_INFO	_IOWR(SYNC_IOC_MAGIC, 2, struct sync_file_info)
> +#define SYNC_IOC_FILE_INFO	_IOWR(SYNC_IOC_MAGIC, 4, struct sync_file_info)
>  
>  #endif /* _UAPI_LINUX_SYNC_H */
> -- 
> 2.5.0
>