[PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[Chris Wilson]

Quoting John Einar Reitan (2017-10-03 14:51:00)
> sync_file_ioctl_fence_info has a race between filling the status
> of the underlying fences and the overall status of the sync_file.
> If fence transitions in the time frame between its sync_fill_fence_info
> and the later dma_fence_is_signaled for the sync_file, the returned
> information is inconsistent showing non-signaled underlying fences but
> an overall signaled state.
> 
> This patch changes sync_file_ioctl_fence_info to track what has been
> encoded and using that as the overall sync_file status.
> 
> Tested-by: Vamsidhar Reddy Gaddam <vamsidhar.gaddam at arm.com>
> Signed-off-by: John Einar Reitan <john.reitan at arm.com>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: Gustavo Padovan <gustavo at padovan.org>
> Cc: dri-devel at lists.freedesktop.org
> ---
>  drivers/dma-buf/sync_file.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 66fb40d0ebdb..ebf5764b868c 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -383,7 +383,7 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>         return err;
>  }
>  
> -static void sync_fill_fence_info(struct dma_fence *fence,
> +static int sync_fill_fence_info(struct dma_fence *fence,
>                                  struct sync_fence_info *info)
>  {
>         strlcpy(info->obj_name, fence->ops->get_timeline_name(fence),
> @@ -399,6 +399,8 @@ static void sync_fill_fence_info(struct dma_fence *fence,
>                 test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags) ?
>                 ktime_to_ns(fence->timestamp) :
>                 ktime_set(0, 0);
> +
> +       return info->status;

In hindsight, having both the per-fence and global variable be called
info was a mistake. Certainly made this diff a little harder to parse
than required!

>  }
>  
>  static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
> @@ -408,7 +410,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>         struct sync_fence_info *fence_info = NULL;
>         struct dma_fence **fences;
>         __u32 size;
> -       int num_fences, ret, i;
> +       int num_fences, ret, i, fences_status = 1;
>  
>         if (copy_from_user(&info, (void __user *)arg, sizeof(info)))
>                 return -EFAULT;
> @@ -424,8 +426,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>          * sync_fence_info and return the actual number of fences on
>          * info->num_fences.
>          */
> -       if (!info.num_fences)
> +       if (!info.num_fences) {
> +               fences_status = dma_fence_is_signaled(sync_file->fence);

Personally I would have set info.status directly rather than add a new
fences_status. But that's immaterial to the bug fix,

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris