On 27 February 2016 at 15:27, Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx> wrote: > Hi Emil, > > 2016-02-27 Emil Velikov <emil.l.velikov@xxxxxxxxx>: > >> Hi Gustavo, >> >> On 26 February 2016 at 18:31, Gustavo Padovan <gustavo@xxxxxxxxxxx> wrote: >> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx> >> > >> > 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 <gustavo.padovan@xxxxxxxxxxxxxxx> >> > --- >> > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel