From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752600AbcCAIgG (ORCPT ); Tue, 1 Mar 2016 03:36:06 -0500 Received: from mga02.intel.com ([134.134.136.20]:34735 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751577AbcCAIgD (ORCPT ); Tue, 1 Mar 2016 03:36:03 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,522,1449561600"; d="scan'208";a="914227798" Subject: Re: [PATCH] staging/android: refactor SYNC_IOC_FILE_INFO To: Gustavo Padovan , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , =?UTF-8?Q?Arve_Hj=c3=b8nnev=c3=a5g?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Gustavo Padovan 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> From: Maarten Lankhorst Message-ID: <56D5546E.4010701@linux.intel.com> Date: Tue, 1 Mar 2016 09:35:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <20160229220839.GD2479@joana> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. ~Maarten