From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932452AbcDNSDt (ORCPT ); Thu, 14 Apr 2016 14:03:49 -0400 Received: from mail-pa0-f54.google.com ([209.85.220.54]:35370 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753116AbcDNSDr (ORCPT ); Thu, 14 Apr 2016 14:03:47 -0400 Date: Thu, 14 Apr 2016 11:03:44 -0700 From: Gustavo Padovan To: Greg Kroah-Hartman Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Maarten Lankhorst , Gustavo Padovan , akpm@linux-foundation.org, joe@perches.com Subject: Re: [PATCH v10 3/3] staging/android: refactor SYNC IOCTLs Message-ID: <20160414180344.GC3312@joana> Mail-Followup-To: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Maarten Lankhorst , Gustavo Padovan , akpm@linux-foundation.org, joe@perches.com References: <1458307660-31986-1-git-send-email-gustavo@padovan.org> <1458307660-31986-3-git-send-email-gustavo@padovan.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458307660-31986-3-git-send-email-gustavo@padovan.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Greg, Any comment on this? Thanks, Gustavo 2016-03-18 Gustavo Padovan : > From: Gustavo Padovan > > 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 > Reviewed-by: Maarten Lankhorst > Acked-by: Greg Hackmann > Acked-by: Rob Clark > Acked-by: Daniel Vetter > > --- > 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 >