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@arm.com Signed-off-by: John Einar Reitan john.reitan@arm.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: dri-devel@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@chris-wilson.co.uk -Chris

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@arm.com Signed-off-by: John Einar Reitan john.reitan@arm.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Gustavo Padovan gustavo@padovan.org Cc: dri-devel@lists.freedesktop.org ---

Changes since v1 (thanks Chris Wilson): - Replaced an unneeded local variable by writing directly to the struct

drivers/dma-buf/sync_file.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 66fb40d0ebdb..03830634e141 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; }

static long sync_file_ioctl_fence_info(struct sync_file *sync_file, @@ -424,8 +426,12 @@ 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) { + info.status = dma_fence_is_signaled(sync_file->fence); goto no_fences; + } else { + info.status = 1; + }

if (info.num_fences < num_fences) return -EINVAL; @@ -435,8 +441,10 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, if (!fence_info) return -ENOMEM;

- for (i = 0; i < num_fences; i++) - sync_fill_fence_info(fences[i], &fence_info[i]); + for (i = 0; i < num_fences; i++) { + int status = sync_fill_fence_info(fences[i], &fence_info[i]); + info.status = info.status <= 0 ? info.status : status; + }

if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info, size)) { @@ -446,7 +454,6 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,

no_fences: sync_file_get_name(sync_file, info.name, sizeof(info.name)); - info.status = dma_fence_is_signaled(sync_file->fence); info.num_fences = num_fences;

if (copy_to_user((void __user *)arg, &info, sizeof(info)))