| Message ID | 1456511507-2534-5-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Gustavo, On 26 February 2016 at 18:31, Gustavo Padovan <gustavo@padovan.org> wrote: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > 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@collabora.co.uk> > --- > 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). > }; > > /** > @@ -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' ? Regards, Emil
Hi Emil, 2016-02-27 Emil Velikov <emil.l.velikov@gmail.com>: > Hi Gustavo, > > On 26 February 2016 at 18:31, Gustavo Padovan <gustavo@padovan.org> wrote: > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > 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@collabora.co.uk> > > --- > > 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. > > > }; > > > > /** > > @@ -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. Gustavo
On 27 February 2016 at 15:27, Gustavo Padovan <gustavo.padovan@collabora.co.uk> wrote: > Hi Emil, > > 2016-02-27 Emil Velikov <emil.l.velikov@gmail.com>: > >> Hi Gustavo, >> >> On 26 February 2016 at 18:31, Gustavo Padovan <gustavo@padovan.org> wrote: >> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >> > >> > 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@collabora.co.uk> >> > --- >> > 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
Op 26-02-16 om 19:31 schreef Gustavo Padovan: > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > 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@collabora.co.uk> > --- > 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; > + goto err_put_fd; > + } -EINVAL > 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; > }; > > /** > @@ -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; > }; > > @@ -44,6 +48,7 @@ struct sync_fence_info { > * struct sync_file_info - data returned from fence info ioctl > * @name: name of fence > * @status: status of fence. 1: signaled 0:active <0:error > + * @flags: sync_file_info flags > * @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. > @@ -52,6 +57,7 @@ struct sync_fence_info { > struct sync_file_info { > char name[32]; > __s32 status; > + __u32 flags; > __u32 num_fences; > __u32 len; >
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; + 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; 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; }; /** @@ -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; }; @@ -44,6 +48,7 @@ struct sync_fence_info { * struct sync_file_info - data returned from fence info ioctl * @name: name of fence * @status: status of fence. 1: signaled 0:active <0:error + * @flags: sync_file_info flags * @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. @@ -52,6 +57,7 @@ struct sync_fence_info { struct sync_file_info { char name[32]; __s32 status; + __u32 flags; __u32 num_fences; __u32 len;