[PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[John Einar Reitan]

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;
 }
 
 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);
 		goto no_fences;
+	}
 
 	if (info.num_fences < num_fences)
 		return -EINVAL;
@@ -435,8 +439,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]);
+		fences_status = fences_status <= 0 ? fences_status : status;
+	}
 
 	if (copy_to_user(u64_to_user_ptr(info.sync_fence_info), fence_info,
 			 size)) {
@@ -446,7 +452,7 @@ 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.status = fences_status;
 	info.num_fences = num_fences;
 
 	if (copy_to_user((void __user *)arg, &info, sizeof(info)))
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[John Einar Reitan]

On Wed, Oct 04, 2017 at 11:43:23AM +0100, Chris Wilson wrote:
> 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!

I agree, I had to re-read the code a few times to find & fix the bug.

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

I can clean that up if there is a need for a v2.

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

Thanks!

John

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[John Einar Reitan]

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)))
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[Chris Wilson]

Quoting John Einar Reitan (2017-10-09 14:49:36)
> 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
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[Gustavo Padovan]

2017-10-09 Chris Wilson <chris@chris-wilson.co.uk>:

> Quoting John Einar Reitan (2017-10-09 14:49:36)
> > 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
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Pushed to drm-misc-fixes.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] sync_file: Return consistent status in SYNC_IOC_FILE_INFO

[Chunming Zhou]

On 2017年10月09日 21:49, John Einar Reitan wrote:
> 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)
Out of curious, why name to sync_fill_xxx not sync_file_xxx?

Regards,
David Zhou
>   {
>   	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)))

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel