[Fix/Bug] memory allocator's allocMemory() should not create hwmem

This patch fixes the bug that memory allocator's allocMemory() has
created hwmem instance. It could make a double-free bug.

Signed-off-by: Dongju Chae <dongju.chae@samsung.com>

diff --git a/src/core/ne-handler.cc b/src/core/ne-handler.cc
index 53227c2..3b71a16 100644 (file)
--- a/src/core/ne-handler.cc
+++ b/src/core/ne-handler.cc
@@ -749,14 +749,14 @@ HostHandler::allocGenericBuffer (generic_buffer *buffer)
     case BUFFER_DMABUF:
     {
       /* now, npu-engine always provides dmabuf-based allocation */
-      HWmem *hwmem;
-      int status = device_->allocMemory (buffer->size, &hwmem);
-      if (status != 0)
-        return status;
-
-      buffer->dmabuf = hwmem->getDmabuf();
-      buffer->offset = hwmem->getOffset();
-      buffer->addr = hwmem->getData();
+      void *addr = nullptr;
+      int dmabuf = device_->allocMemory (buffer->size, &addr);
+      if (dmabuf < 0)
+        return dmabuf;
+
+      buffer->dmabuf = dmabuf;
+      buffer->offset = 0;
+      buffer->addr = addr;
     } break;
     default:
       return -EINVAL;
@@ -783,7 +783,7 @@ HostHandler::deallocGenericBuffer (generic_buffer *buffer)
       break;
     case BUFFER_MAPPED:
     case BUFFER_DMABUF:
-      status = device_->deallocMemory (buffer->dmabuf);
+      status = device_->deallocMemory (buffer->dmabuf, buffer->size, buffer->addr);
       break;
     default:
       status = -EINVAL;
@@ -953,42 +953,47 @@ Device::stop (bool force_stop)
 
 /**
  * @brief allocate generic memory buffer
- * @param[out] hwmem_ptr hwmem instance pointer
- * @return 0 if no error, otherwise a negative errno
+ * @param[in] size the size to allocate
+ * @param[out] addr the mapped address
+ * @return dmabuf fd if no error, otherwise a negative errno
  */
 int
-Device::allocMemory (size_t size, HWmem ** hwmem_ptr)
+Device::allocMemory (size_t size, void **addr)
 {
   if (!initialized ()) {
     logerr (TAG, "Uninitialized device; should use libnpuhost APIs\n");
     return -EPERM;
   }
 
-  if (size == 0 || hwmem_ptr == nullptr)
+  if (size == 0 || addr == nullptr) {
+    logerr (TAG, "Invalid arguments\n");
     return -EINVAL;
+  }
 
-  HWmem *hwmem = mem_->allocMemory (size);
-  if (hwmem == nullptr)
-    return -ENOMEM;
-
-  *hwmem_ptr = hwmem;
-  return 0;
+  return mem_->allocMemory (size, addr);
 }
 
 /**
  * @brief deallocate generic memory buffer
  * @param[in] dmabuf_fd dmabuf file descriptor
+ * @param[in] size buffer size
+ * @param[in] addr mapped addr
  * @return 0 if no error, otherwise a negative errno
  */
 int
-Device::deallocMemory (int dmabuf_fd)
+Device::deallocMemory (int dmabuf_fd, size_t size, void * addr)
 {
   if (!initialized ()) {
     logerr (TAG, "Uninitialized device; should use libnpuhost APIs\n");
     return -EPERM;
   }
 
-  return mem_->deallocMemory (dmabuf_fd);
+  if (dmabuf_fd < 0 || size == 0 || addr == nullptr) {
+    logerr (TAG, "Invalid arguments\n");
+    return -EINVAL;
+  }
+
+  return mem_->deallocMemory (dmabuf_fd, size, addr);
 }
 
 /**

diff --git a/src/core/ne-handler.h b/src/core/ne-handler.h
index c17a43c..6670e0e 100644 (file)
--- a/src/core/ne-handler.h
+++ b/src/core/ne-handler.h
@@ -98,8 +98,8 @@ class Device {
 
     /** @brief stops all requests from this device (choose wait or force) */
     int stop (bool force_stop);
-    int allocMemory (size_t size, HWmem ** hwmem_ptr);
-    int deallocMemory (int dmabuf_fd);
+    int allocMemory (size_t size, void ** addr);
+    int deallocMemory (int dmabuf_fd, size_t size, void * addr);
 
     /** virtual methods to implement each device's behaviors */
     virtual int setModel (const generic_buffer *model, Model ** model_ptr) { return -EPERM; }

diff --git a/src/core/ne-mem.cc b/src/core/ne-mem.cc
index e3d7ac6..d35e18e 100644 (file)
--- a/src/core/ne-mem.cc
+++ b/src/core/ne-mem.cc
@@ -27,17 +27,25 @@ class MemDefault : public MemAllocator {
     MemDefault (const DriverAPI *api) : MemAllocator (api) {}
 
     /** For library users. its backing storage is always a device driver */
-    HWmem* allocMemory (size_t size) {
-      HWmem * hwmem = new HWmem (new HWmemDevice);
-      hwmem->setDriverAPI (api_);
-      if (hwmem->alloc (size) == 0)
-        return hwmem;
-
-      delete hwmem;
-      return nullptr;
+    int allocMemory (size_t size, void ** addr) {
+      int dmabuf = api_->alloc (size);
+      if (dmabuf < 0)
+        return dmabuf;
+
+      *addr = api_->mmap (dmabuf, size);
+      if (*addr == nullptr) {
+        api_->dealloc (dmabuf);
+        return -EINVAL;
+      }
+
+      return dmabuf;
     }
 
-    int deallocMemory (int dmabuf_fd) {
+    int deallocMemory (int dmabuf_fd, size_t size, void * addr) {
+      int status = api_->munmap (addr, size);
+      if (status != 0)
+        return status;
+
       return api_->dealloc (dmabuf_fd);
     }
 

diff --git a/src/core/ne-mem.h b/src/core/ne-mem.h
index a6201e6..eab5904 100644 (file)
--- a/src/core/ne-mem.h
+++ b/src/core/ne-mem.h
@@ -44,8 +44,8 @@ class MemAllocator {
     /**
      * Below allocates the actual memory. So, the caller don't need to call alloc().
      */
-    virtual HWmem * allocMemory (size_t size) { return nullptr; }
-    virtual int deallocMemory (int dmabuf) { return -EINVAL; }
+    virtual int allocMemory (size_t size, void **addr) { return -EINVAL; }
+    virtual int deallocMemory (int dmabuf, size_t size, void *addr) { return -EINVAL; }
 
     /**
      * Below does not allocate actual memory as we don't know the type of generic buffer

diff --git a/tests/unittests/ne_core_handler_test.cc b/tests/unittests/ne_core_handler_test.cc
index 580cb20..4a07752 100644 (file)
--- a/tests/unittests/ne_core_handler_test.cc
+++ b/tests/unittests/ne_core_handler_test.cc
@@ -90,13 +90,13 @@ TEST (ne_core_handler_test, device_instance_uninitilized_n)
   std::unique_ptr<Device> triv (new TrinityVision (0));
   EXPECT_EQ (triv->initialized (), false);
 
-  HWmem * hwmem;
+  void * addr = nullptr;
   Model *model;
   generic_buffer model_buf;
   input_buffers input;
 
-  EXPECT_EQ (triv->allocMemory (4096, &hwmem), -EPERM);
-  EXPECT_EQ (triv->deallocMemory (0), -EPERM);
+  EXPECT_EQ (triv->allocMemory (4096, &addr), -EPERM);
+  EXPECT_EQ (triv->deallocMemory (0, 4096, addr), -EPERM);
   EXPECT_EQ (triv->stop (true), -EPERM);
   EXPECT_EQ (triv->setModel (&model_buf, &model), -EPERM);
   EXPECT_EQ (triv->run (NPUINPUT_HOST, model, &input, nullptr, nullptr, nullptr), -EPERM);
@@ -104,8 +104,8 @@ TEST (ne_core_handler_test, device_instance_uninitilized_n)
   std::unique_ptr<Device> triv2 (new TrinityVision2 (0));
   EXPECT_EQ (triv2->initialized (), false);
 
-  EXPECT_EQ (triv2->allocMemory (4096, &hwmem), -EPERM);
-  EXPECT_EQ (triv2->deallocMemory (0), -EPERM);
+  EXPECT_EQ (triv2->allocMemory (4096, &addr), -EPERM);
+  EXPECT_EQ (triv2->deallocMemory (0, 4096, addr), -EPERM);
   EXPECT_EQ (triv2->stop (true), -EPERM);
   EXPECT_EQ (triv2->setModel (&model_buf, &model), -EPERM);
   EXPECT_EQ (triv2->run (NPUINPUT_HOST, model, &input, nullptr, nullptr, nullptr), -EPERM);
@@ -113,8 +113,8 @@ TEST (ne_core_handler_test, device_instance_uninitilized_n)
   std::unique_ptr<Device> tria (new TrinityAsr (0));
   EXPECT_EQ (tria->initialized (), false);
 
-  EXPECT_EQ (tria->allocMemory (4096, &hwmem), -EPERM);
-  EXPECT_EQ (tria->deallocMemory (0), -EPERM);
+  EXPECT_EQ (tria->allocMemory (4096, &addr), -EPERM);
+  EXPECT_EQ (tria->deallocMemory (0, 4096, addr), -EPERM);
   EXPECT_EQ (tria->stop (true), -EPERM);
   EXPECT_EQ (tria->setModel (&model_buf, &model), -EPERM);
   EXPECT_EQ (tria->run (NPUINPUT_HOST, model, &input, nullptr, nullptr, nullptr), -EPERM);
@@ -128,9 +128,11 @@ TEST (ne_core_handler_test, device_memory)
   std::unique_ptr<Device> device (Device::createInstance (NPUCOND_TRIV_CONN_SOCIP, 0));
   EXPECT_NE (device.get (), nullptr);
 
-  HWmem * hwmem;
-  EXPECT_EQ (device->allocMemory (4096, &hwmem), 0);
-  EXPECT_EQ (device->deallocMemory (hwmem->getDmabuf()), 0);
+  void * addr;
+  int dmabuf;
+  dmabuf = device->allocMemory (4096, &addr);
+  EXPECT_GE (dmabuf, 0);
+  EXPECT_EQ (device->deallocMemory (dmabuf, 4096, addr), 0);
 }
 
 /**
@@ -141,14 +143,19 @@ TEST (ne_core_handler_test, device_memory_args_n)
   std::unique_ptr<Device> device (Device::createInstance (NPUCOND_TRIV_CONN_SOCIP, 0));
   EXPECT_NE (device.get (), nullptr);
 
-  HWmem * hwmem;
-  EXPECT_NE (device->allocMemory (0, &hwmem), 0);
+  void * addr;
+  int dmabuf;
+  EXPECT_NE (device->allocMemory (0, &addr), 0);
   EXPECT_NE (device->allocMemory (4096, nullptr), 0);
 
-  EXPECT_EQ (device->allocMemory (4096, &hwmem), 0);
-  EXPECT_NE (device->deallocMemory (-1), 0);
-  EXPECT_NE (device->deallocMemory (hwmem->getDmabuf () + 1), 0);
-  EXPECT_EQ (device->deallocMemory (hwmem->getDmabuf ()), 0);
+  dmabuf = device->allocMemory (4096, &addr);
+  ASSERT_GE (dmabuf, 0);
+  EXPECT_NE (device->deallocMemory (-1, 0, nullptr), 0);
+  EXPECT_NE (device->deallocMemory (dmabuf, 0, nullptr), 0);
+  EXPECT_NE (device->deallocMemory (dmabuf, 4096, nullptr), 0);
+  EXPECT_NE (device->deallocMemory (dmabuf, 0, addr), 0);
+  EXPECT_NE (device->deallocMemory (dmabuf + 1, 4096, addr), 0);
+  ASSERT_EQ (device->deallocMemory (dmabuf, 4096, addr), 0);
 }
 
 static void
@@ -1370,7 +1377,7 @@ TEST (ne_core_handler_test, handler_triv_run_async_n)
 TEST (ne_core_handler_test, handler_triv_get_memory_status)
 {
   // disabled until this feature is implemented
-#if 0
+#if defined (ENABLE_EMUL)
   std::unique_ptr<Device> device (Device::createInstance (NPUCOND_TRIV_CONN_SOCIP, 0));
   ASSERT_NE (device.get (), nullptr);
 

diff --git a/tests/unittests/ne_core_inf_test.cc b/tests/unittests/ne_core_inf_test.cc
index 83fff1b..0fe5215 100644 (file)
--- a/tests/unittests/ne_core_inf_test.cc
+++ b/tests/unittests/ne_core_inf_test.cc
@@ -51,8 +51,8 @@ TEST (ne_core_inf_test, invoke)
  */
 TEST (ne_core_inf_test, invoke_triv2)
 {
-  // disable until triv2 driver is prepared.
-#if 0
+// disabled until triv2 driver is prepared.
+#if defined (ENABLE_EMUL)
   std::unique_ptr<DriverAPI> api;
   api = DriverAPI::createDriverAPI (NPUCOND_TRIV2_CONN_SOCIP, 0);
 

diff --git a/tests/unittests/ne_core_mem_test.cc b/tests/unittests/ne_core_mem_test.cc
index 02ee87a..668aa82 100644 (file)
--- a/tests/unittests/ne_core_mem_test.cc
+++ b/tests/unittests/ne_core_mem_test.cc
@@ -37,11 +37,11 @@ TEST (ne_core_mem_test, mem_default_primitives)
   std::unique_ptr<MemAllocator> mem = MemAllocator::createInstance (api.get ());
   EXPECT_NE (mem.get (), nullptr);
 
-  HWmem * hwmem = mem->allocMemory (4 * K);
-  EXPECT_NE (hwmem, nullptr);
-  EXPECT_GE (hwmem->getDmabuf (), 0);
-  EXPECT_EQ (mem->deallocMemory (hwmem->getDmabuf ()), 0);
-  delete hwmem;
+  void * addr = nullptr;
+  int dmabuf = mem->allocMemory (4 * K, &addr);
+  EXPECT_GE (dmabuf, 0);
+  EXPECT_NE (addr, nullptr);
+  EXPECT_EQ (mem->deallocMemory (dmabuf, 4 * K, addr), 0);
 
   Model * model = mem->allocModel (new HWmemDevice);
   EXPECT_NE (model, nullptr);
@@ -67,8 +67,12 @@ TEST (ne_core_mem_test, mem_pool_primitives_n)
   std::unique_ptr<MemAllocator> mem = MemAllocator::createInstance (api.get ());
   EXPECT_NE (mem.get (), nullptr);
 
-  EXPECT_EQ (mem->allocMemory (4096), nullptr);
-  EXPECT_NE (mem->deallocMemory (0), 0);
+  void * addr;
+  int dmabuf;
+
+  dmabuf = mem->allocMemory (4096, &addr);
+  EXPECT_LT (dmabuf, 0);
+  EXPECT_NE (mem->deallocMemory (dmabuf, 4096, addr), 0);
   EXPECT_EQ (mem->allocModel (nullptr), nullptr);
   EXPECT_EQ (mem->allocBuffer (nullptr), nullptr);
 
@@ -88,20 +92,6 @@ TEST (ne_core_mem_test, create_instance_no_api_n)
 }
 
 /**
- * @brief test deallocMemory () with error handling
- */
-TEST (ne_core_mem_test, dealloc_memory_no_dmabuf_n)
-{
-  std::unique_ptr<DriverAPI> api = DriverAPI::createDriverAPI (NPUCOND_TRIV_CONN_SOCIP, 0);
-  ASSERT_NE (api.get (), nullptr);
-  std::unique_ptr<MemAllocator> mem = MemAllocator::createInstance (api.get ());
-  EXPECT_NE (mem.get (), nullptr);
-
-  EXPECT_NE (mem->deallocMemory (0), 0);
-  EXPECT_NE (mem->deallocMemory (1), 0);
-}
-
-/**
  * @brief main function for unit test
  */
 int

diff --git a/tests/unittests/ne_core_segment_table_test.cc b/tests/unittests/ne_core_segment_table_test.cc
index 90b6a2d..5222b01 100644 (file)
--- a/tests/unittests/ne_core_segment_table_test.cc
+++ b/tests/unittests/ne_core_segment_table_test.cc
@@ -31,6 +31,9 @@ TEST (ne_core_segment_table_test, primitives)
   EXPECT_EQ (table->getNumOutputSegments (), 0);
 }
 
+// disabled until triv2 driver is prepared.
+#if defined (ENABLE_EMUL)
+
 /** @brief create dummy metadata v3 */
 static void
 create_metadata_v3 (npubin_meta & meta)
@@ -184,6 +187,8 @@ TEST (ne_core_segment_table_test, create_segments_n)
   EXPECT_NE (table->createSegments (model.get(), &input), 0);
 }
 
+#endif
+
 /**
  * @brief main function for unit test
  */