Re: [PATCH libdrm 1/2] amdgpu: prevent an integer wraparound of cpu_map_count

[Christian König]

That looks really ugly to me. Mapping the same BO so often is illegal 
and should be handled as error.
Otherwise we will never be able to cleanly recover from a GPU lockup 
with lost state by reloading the client library.
Christian.

Am 23.10.18 um 21:07 schrieb Marek Olšák:
From: Marek Olšák <marek.ol...@amd.com>

---
  amdgpu/amdgpu_bo.c | 19 +++++++++++++++++--
  1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index c0f42e81..81f8a5f7 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -22,20 +22,21 @@
   *
   */
  
  #include <stdlib.h>
  #include <stdio.h>
  #include <stdint.h>
  #include <string.h>
  #include <errno.h>
  #include <fcntl.h>
  #include <unistd.h>
+#include <limits.h>
  #include <sys/ioctl.h>
  #include <sys/mman.h>
  #include <sys/time.h>
  
  #include "libdrm_macros.h"
  #include "xf86drm.h"
  #include "amdgpu_drm.h"
  #include "amdgpu_internal.h"
  #include "util_math.h"
  
@@ -442,21 +443,29 @@ drm_public int amdgpu_bo_cpu_map(amdgpu_bo_handle bo, void **cpu)
  {
        union drm_amdgpu_gem_mmap args;
        void *ptr;
        int r;
  
  	pthread_mutex_lock(&bo->cpu_access_mutex);
  
  	if (bo->cpu_ptr) {
                /* already mapped */
                assert(bo->cpu_map_count > 0);
-               bo->cpu_map_count++;
+
+               /* If the counter has already reached INT_MAX, don't increment
+                * it and assume that the buffer will be mapped indefinitely.
+                * The buffer is pretty unlikely to get unmapped by the user
+                * at this point.
+                */
+               if (bo->cpu_map_count != INT_MAX)
+                       bo->cpu_map_count++;
+
                *cpu = bo->cpu_ptr;
                pthread_mutex_unlock(&bo->cpu_access_mutex);
                return 0;
        }
  
  	assert(bo->cpu_map_count == 0);
  
  	memset(&args, 0, sizeof(args));
  
  	/* Query the buffer address (args.addr_ptr).
@@ -492,21 +501,27 @@ drm_public int amdgpu_bo_cpu_unmap(amdgpu_bo_handle bo)
  
  	pthread_mutex_lock(&bo->cpu_access_mutex);
        assert(bo->cpu_map_count >= 0);
  
  	if (bo->cpu_map_count == 0) {
                /* not mapped */
                pthread_mutex_unlock(&bo->cpu_access_mutex);
                return -EINVAL;
        }
  
-	bo->cpu_map_count--;
+       /* If the counter has already reached INT_MAX, don't decrement it.
+        * This is because amdgpu_bo_cpu_map doesn't increment it past
+        * INT_MAX.
+        */
+       if (bo->cpu_map_count != INT_MAX)
+               bo->cpu_map_count--;
+
        if (bo->cpu_map_count > 0) {
                /* mapped multiple times */
                pthread_mutex_unlock(&bo->cpu_access_mutex);
                return 0;
        }
  
  	r = drm_munmap(bo->cpu_ptr, bo->alloc_size) == 0 ? 0 : -errno;
        bo->cpu_ptr = NULL;
        pthread_mutex_unlock(&bo->cpu_access_mutex);
        return r;
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx