Re: [PATCH] staging/android: refactor SYNC_IOC_FILE_INFO

Op 26-02-16 om 22:00 schreef Gustavo Padovan:

From: Gustavo Padovan gustavo.padovan@collabora.co.uk

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 gustavo.padovan@collabora.co.uk

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