From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752097AbcB2H7T (ORCPT ); Mon, 29 Feb 2016 02:59:19 -0500 Received: from mail-wm0-f49.google.com ([74.125.82.49]:32784 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751925AbcB2H7R (ORCPT ); Mon, 29 Feb 2016 02:59:17 -0500 MIME-Version: 1.0 In-Reply-To: <20160227152756.GC20887@joana> References: <1456511507-2534-1-git-send-email-gustavo@padovan.org> <1456511507-2534-5-git-send-email-gustavo@padovan.org> <20160227152756.GC20887@joana> Date: Mon, 29 Feb 2016 07:59:16 +0000 Message-ID: Subject: Re: [PATCH v4 5/5] staging/android: add flags member to sync ioctl structs From: Emil Velikov To: Gustavo Padovan Cc: Gustavo Padovan , Greg Kroah-Hartman , devel@driverdev.osuosl.org, Daniel Stone , Daniel Vetter , Riley Andrews , ML dri-devel , "Linux-Kernel@Vger. Kernel. Org" , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , John Harrison Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 27 February 2016 at 15:27, Gustavo Padovan wrote: > Hi Emil, > > 2016-02-27 Emil Velikov : > >> Hi Gustavo, >> >> On 26 February 2016 at 18:31, Gustavo Padovan wrote: >> > From: Gustavo Padovan >> > >> > Play safe and add flags member to all structs. So we don't need to >> > break API or create new IOCTL in the future if new features that requires >> > flags arises. >> > >> > v2: check if flags are valid (zero, in this case) >> > >> > Signed-off-by: Gustavo Padovan >> > --- >> > drivers/staging/android/sync.c | 7 ++++++- >> > drivers/staging/android/uapi/sync.h | 6 ++++++ >> > 2 files changed, 12 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c >> > index 837cff5..54fd5ab 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) { >> > + err = -EFAULT; >> -EINVAL ? >> >> > + goto err_put_fd; >> > + } >> > + >> > fence2 = sync_file_fdget(data.fd2); >> > if (!fence2) { >> > err = -ENOENT; >> > @@ -511,7 +516,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, >> > if (copy_from_user(&in, (void __user *)arg, sizeof(*info))) >> > return -EFAULT; >> > >> > - if (in.status || strcmp(in.name, "\0")) >> > + if (in.status || in.flags || strcmp(in.name, "\0")) >> > return -EFAULT; >> -EINVAL ? >> >> > >> > if (in.num_fences && !in.sync_fence_info) >> > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h >> > index 9aad623..f56a6c2 100644 >> > --- a/drivers/staging/android/uapi/sync.h >> > +++ b/drivers/staging/android/uapi/sync.h >> > @@ -19,11 +19,13 @@ >> > * @fd2: file descriptor of second fence >> > * @name: name of new fence >> > * @fence: returns the fd of the new fence to userspace >> > + * @flags: merge_data flags >> > */ >> > struct sync_merge_data { >> > __s32 fd2; >> > char name[32]; >> > __s32 fence; >> > + __u32 flags; >> The overall size of the struct is not multiple of 64bit, so things >> will end up badly if we decide to extend it in the future. Even if >> there's a small chance that update will be needed, we might as well >> pad it now (and check the padding for zero, returning -EINVAL). > > I think name could be the first field here. > Up-to you really. I'm afraid that it doesn't resolve the issue :-( As a test add a u64 value at the end of the struct and check the output of pahole for 32 and 64 bit build. >> >> > }; >> > >> > /** >> > @@ -31,12 +33,14 @@ 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; >> Should we be doing some form of validation in sync_fill_fence_info() >> of 'flags' ? > > Do you think it is necessary? The kernel allocates a zero'ed buffer to > fill sync_fence_info array. > Good point. Missed out the z in kzalloc :-) -Emil