From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752592AbcB2I1T (ORCPT ); Mon, 29 Feb 2016 03:27:19 -0500 Received: from mga14.intel.com ([192.55.52.115]:48221 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752010AbcB2I1R (ORCPT ); Mon, 29 Feb 2016 03:27:17 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,519,1449561600"; d="scan'208";a="923252519" From: Maarten Lankhorst Subject: Re: [PATCH] staging/android: refactor SYNC_IOC_FILE_INFO To: Gustavo Padovan , Greg Kroah-Hartman References: <1456511507-2534-4-git-send-email-gustavo@padovan.org> <1456520410-28195-1-git-send-email-gustavo@padovan.org> Cc: 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 Message-ID: <56D400CD.6070804@linux.intel.com> Date: Mon, 29 Feb 2016 09:26:53 +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: <1456520410-28195-1-git-send-email-gustavo@padovan.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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); It could probably be removed since num_fences would do the same. ~Maarten