From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753375AbcCALzg (ORCPT ); Tue, 1 Mar 2016 06:55:36 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:42494 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752628AbcCALzf (ORCPT ); Tue, 1 Mar 2016 06:55:35 -0500 Date: Tue, 1 Mar 2016 08:55:27 -0300 From: Gustavo Padovan To: Maarten Lankhorst Cc: 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 Subject: Re: [PATCH] staging/android: refactor SYNC_IOC_FILE_INFO Message-ID: <20160301115527.GE20887@joana> References: <1456511507-2534-4-git-send-email-gustavo@padovan.org> <1456520410-28195-1-git-send-email-gustavo@padovan.org> <56D400CD.6070804@linux.intel.com> <20160229220839.GD2479@joana> <56D5546E.4010701@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56D5546E.4010701@linux.intel.com> 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 Maarten, 2016-03-01 Maarten Lankhorst : > Op 29-02-16 om 23:08 schreef Gustavo Padovan: > > Hi Maarten, > > > > 2016-02-29 Maarten Lankhorst : > > > >> Op 26-02-16 om 22:00 schreef Gustavo Padovan: > >>> From: Gustavo Padovan > >>> > >>> Change SYNC_IOC_FILE_INFO behaviour to avoid future API breaks and > >>> optimize buffer allocation. In the new approach the ioctl needs to be called > >>> twice to retrieve the array of fence_infos pointed by info->sync_fence_info. > >>> > >>> The first call should pass num_fences = 0, the kernel will then fill > >>> info->num_fences. Userspace receives back the number of fences and > >>> allocates a buffer size num_fences * sizeof(struct sync_fence_info) on > >>> info->sync_fence_info. > >>> > >>> It then call the ioctl again passing num_fences received in info->num_fences. > >>> The kernel checks if info->num_fences > 0 and if yes it fill > >>> info->sync_fence_info with an array containing all fence_infos. > >>> > >>> info->len now represents the length of the buffer sync_fence_info points > >>> to. Also, info->sync_fence_info was converted to __u64 pointer. > >>> > >>> An example userspace code would be: > >>> > >>> struct sync_file_info *info; > >>> int err, size, num_fences; > >>> > >>> info = malloc(sizeof(*info)); > >>> > >>> memset(info, 0, sizeof(*info)); > >>> > >>> err = ioctl(fd, SYNC_IOC_FILE_INFO, info); > >>> num_fences = info->num_fences; > >>> > >>> if (num_fences) { > >>> memset(info, 0, sizeof(*info)); > >> Would this memset still be needed if we didn't check for nulls in info->status and info->name ? > >> > >> Seems to me that it could be skipped in that case. > > Yes, I agree. > > > >>> size = sizeof(struct sync_fence_info) * num_fences; > >>> info->len = size; > >>> 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); > >>> } > >>> > >>> v2: fix fence_info memory leak > >>> > >>> Signed-off-by: Gustavo Padovan > >>> --- > >>> drivers/staging/android/sync.c | 52 +++++++++++++++++++++++++++++-------- > >>> drivers/staging/android/uapi/sync.h | 9 +++---- > >>> 2 files changed, 45 insertions(+), 16 deletions(-) > >>> > >>> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > >>> index dc5f382..2379f23 100644 > >>> --- a/drivers/staging/android/sync.c > >>> +++ b/drivers/staging/android/sync.c > >>> @@ -502,21 +502,22 @@ static int sync_fill_fence_info(struct fence *fence, void *data, int size) > >>> static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > >>> unsigned long arg) > >>> { > >>> - struct sync_file_info *info; > >>> + struct sync_file_info in, *info; > >>> + struct sync_fence_info *fence_info = NULL; > >>> __u32 size; > >>> __u32 len = 0; > >> = 0 unneeded. > >>> int ret, i; > >>> > >>> - if (copy_from_user(&size, (void __user *)arg, sizeof(size))) > >>> + if (copy_from_user(&in, (void __user *)arg, sizeof(*info))) > >>> return -EFAULT; > >>> > >>> - if (size < sizeof(struct sync_file_info)) > >>> - return -EINVAL; > >>> + if (in.status || strcmp(in.name, "\0")) > >>> + return -EFAULT; > >> These members always get written by the fence ioctl, I'm not sure it adds value to have them explicitly zero'd out by userspace. > >>> - if (size > 4096) > >>> - size = 4096; > >>> + if (in.num_fences && !in.sync_fence_info) > >>> + return -EFAULT; > >> This check is unneeded, it will happen in the copy_to_user call anyway. > >>> - info = kzalloc(size, GFP_KERNEL); > >>> + info = kzalloc(sizeof(*info), GFP_KERNEL); > >>> if (!info) > >>> return -ENOMEM; > >>> > >>> @@ -525,14 +526,33 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > >>> if (info->status >= 0) > >>> info->status = !info->status; > >>> > >>> - info->num_fences = sync_file->num_fences; > >>> + /* > >>> + * Passing num_fences = 0 means that userspace want to know how > >>> + * many fences are in the sync_file to be able to allocate a buffer to > >>> + * fit all sync_fence_infos and call the ioctl again with the buffer > >>> + * assigned to info->sync_fence_info. The second call pass the > >>> + * num_fences value received in the first call. > >>> + */ > >>> + if (!in.num_fences) > >>> + goto no_fences; > >>> + > >>> + size = sync_file->num_fences * sizeof(*fence_info); > >>> + if (in.len != size) { > >>> + ret = -EFAULT; > >>> + goto out; > >>> + } > >> Maybe check for in.len < size, and set set to size? > >> > >> > >>> - len = sizeof(struct sync_file_info); > >>> + fence_info = kzalloc(size, GFP_KERNEL); > >>> + if (!fence_info) { > >>> + ret = -ENOMEM; > >>> + goto out; > >>> + } > >>> > >>> for (i = 0; i < sync_file->num_fences; ++i) { > >>> struct fence *fence = sync_file->cbs[i].fence; > >>> > >>> - ret = sync_fill_fence_info(fence, (u8 *)info + len, size - len); > >>> + ret = sync_fill_fence_info(fence, (u8 *)fence_info + len, > >>> + size - len); > >>> > >>> if (ret < 0) > >>> goto out; > >>> @@ -540,14 +560,24 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > >>> len += ret; > >>> } > >>> > >>> + if (copy_to_user((void __user *)in.sync_fence_info, fence_info, size)) { > >>> + ret = -EFAULT; > >>> + goto out; > >>> + } > >>> + > >>> info->len = len; > >>> + info->sync_fence_info = (__u64) in.sync_fence_info; > >>> + > >>> +no_fences: > >>> + 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(fence_info); > >>> kfree(info); > >>> > >>> return ret; > >>> diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > >>> index f0b41ce..9aad623 100644 > >>> --- a/drivers/staging/android/uapi/sync.h > >>> +++ b/drivers/staging/android/uapi/sync.h > >>> @@ -42,21 +42,20 @@ struct sync_fence_info { > >>> > >>> /** > >>> * 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 > >>> * @num_fences number of fences in the sync_file > >>> + * @len: ioctl caller writes the size of the buffer its passing in. > >>> + * ioctl returns length of all fence_infos summed. > >>> * @sync_fence_info: array of sync_fence_info for every fence in the sync_file > >>> */ > >>> struct sync_file_info { > >>> - __u32 len; > >>> char name[32]; > >>> __s32 status; > >>> __u32 num_fences; > >>> + __u32 len; > >>> > >>> - __u8 sync_fence_info[0]; > >>> + __u64 sync_fence_info; > >>> }; > >>> > >>> #define SYNC_IOC_MAGIC '>' > >> Not sure if len adds anything here, userspace knows to allocate num_fences * sizeof(struct sync_fence_info); > > I think of len being useful if we decide to extend struct sync_fence_info in > > the future, so we may use len to help discover the size of each > > sync_fence_info. What do you think? > > > I don't think you could extend it arbitrarily, you could make userspace pass a flag to indicate the struct is extended, so kernel space can choose > whether to use the bigger size struct or not. > > something like sync_file_info.flags = FENCE_INFO_V2; -- kernel can reject with -EINVAL if unsupported, or fill in a v2 struct. Fair enough, I'll just remove len then. Gustavo