From a6470475a988360d64847ad223fbb63af0f3b8ee Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Wed, 28 Oct 2020 09:21:08 +0300 Subject: [PATCH 1/2] [SYCL] Fix the check for read-only host pointer during memobj creation Previously, whenever a native memory object was created, the original host pointer passed by the user was checked for being read-only, regardless of what host pointer was actually used at that point. This patch fixes the issue by assuming a writeable host pointer unless it's the one provided by the user. As a result, this fixes a double free issue that appeared with the following usage pattern: 1. Create a buffer with a const user pointer 2. Write-access it on host 3. Access it on a non-host device 4. Access it on host This error was caused by the allocation performed at step 2 being treated as read-only and, therefore, used with PI_MEM_FLAGS_HOST_PTR_COPY rather than PI_MEM_FLAGS_HOST_PTR_USE at step 3, while it was assumed otherwise during the release of this allocation. Signed-off-by: Sergey Semenov --- sycl/source/detail/buffer_impl.cpp | 25 +++++++++-------- sycl/source/detail/image_impl.cpp | 24 ++++++++-------- .../buffer/native_buffer_creation_flags.cpp | 28 +++++++++++++++++++ 3 files changed, 54 insertions(+), 23 deletions(-) create mode 100644 sycl/test/basic_tests/buffer/native_buffer_creation_flags.cpp diff --git a/sycl/source/detail/buffer_impl.cpp b/sycl/source/detail/buffer_impl.cpp index 26e189f1abffd..b3e8c64554754 100644 --- a/sycl/source/detail/buffer_impl.cpp +++ b/sycl/source/detail/buffer_impl.cpp @@ -16,21 +16,22 @@ namespace sycl { namespace detail { void *buffer_impl::allocateMem(ContextImplPtr Context, bool InitFromUserData, void *HostPtr, RT::PiEvent &OutEventToWait) { + // Assume a read-write pointer unless reusing a user pointer. + bool HostPtrReadOnly = false; + if (InitFromUserData) { + assert(!HostPtr && "Cannot init from user data and reuse host ptr provided " + "simultaneously"); + HostPtr = BaseT::getUserPtr(); + HostPtrReadOnly = BaseT::MHostPtrReadOnly; + } - assert(!(InitFromUserData && HostPtr) && - "Cannot init from user data and reuse host ptr provided " - "simultaneously"); - - void *UserPtr = InitFromUserData ? BaseT::getUserPtr() : HostPtr; - - assert(!(nullptr == UserPtr && BaseT::useHostPtr() && Context->is_host()) && - "Internal error. Allocating memory on the host " - "while having use_host_ptr property"); + assert(!(nullptr == HostPtr && BaseT::useHostPtr() && Context->is_host()) && + "Internal error. Allocating memory on the host " + "while having use_host_ptr property"); return MemoryManager::allocateMemBuffer( - std::move(Context), this, UserPtr, BaseT::MHostPtrReadOnly, - BaseT::getSize(), BaseT::MInteropEvent, BaseT::MInteropContext, MProps, - OutEventToWait); + std::move(Context), this, HostPtr, HostPtrReadOnly, BaseT::getSize(), + BaseT::MInteropEvent, BaseT::MInteropContext, MProps, OutEventToWait); } } // namespace detail } // namespace sycl diff --git a/sycl/source/detail/image_impl.cpp b/sycl/source/detail/image_impl.cpp index 747f3b193fd85..bd9f2614ff812 100644 --- a/sycl/source/detail/image_impl.cpp +++ b/sycl/source/detail/image_impl.cpp @@ -307,15 +307,17 @@ template void *image_impl::allocateMem(ContextImplPtr Context, bool InitFromUserData, void *HostPtr, RT::PiEvent &OutEventToWait) { + // Assume a read-write pointer unless reusing a user pointer. + bool HostPtrReadOnly = false; + if (InitFromUserData) { + assert(!HostPtr && "Cannot init from user data and reuse host ptr provided " + "simultaneously"); + HostPtr = BaseT::getUserPtr(); + HostPtrReadOnly = BaseT::MHostPtrReadOnly; + } - assert(!(InitFromUserData && HostPtr) && - "Cannot init from user data and reuse host ptr provided " - "simultaneously"); - - void *UserPtr = InitFromUserData ? BaseT::getUserPtr() : HostPtr; - - RT::PiMemImageDesc Desc = getImageDesc(UserPtr != nullptr); - assert(checkImageDesc(Desc, Context, UserPtr) && + RT::PiMemImageDesc Desc = getImageDesc(HostPtr != nullptr); + assert(checkImageDesc(Desc, Context, HostPtr) && "The check an image desc failed."); RT::PiMemImageFormat Format = getImageFormat(); @@ -323,9 +325,9 @@ void *image_impl::allocateMem(ContextImplPtr Context, "The check an image format failed."); return MemoryManager::allocateMemImage( - std::move(Context), this, UserPtr, BaseT::MHostPtrReadOnly, - BaseT::getSize(), Desc, Format, BaseT::MInteropEvent, - BaseT::MInteropContext, MProps, OutEventToWait); + std::move(Context), this, HostPtr, HostPtrReadOnly, BaseT::getSize(), + Desc, Format, BaseT::MInteropEvent, BaseT::MInteropContext, MProps, + OutEventToWait); } template diff --git a/sycl/test/basic_tests/buffer/native_buffer_creation_flags.cpp b/sycl/test/basic_tests/buffer/native_buffer_creation_flags.cpp new file mode 100644 index 0000000000000..68e912e3e4637 --- /dev/null +++ b/sycl/test/basic_tests/buffer/native_buffer_creation_flags.cpp @@ -0,0 +1,28 @@ +// RUN: %clangxx -fsycl -fsycl-targets=%sycl_triple %s -o %t.out +// RUN: %CPU_RUN_PLACEHOLDER SYCL_PI_TRACE=-1 %t.out 2>&1 %CPU_CHECK_PLACEHOLDER + +#include + +class Foo; +using namespace cl::sycl; +int main() { + const int BufVal = 42; + buffer Buf{&BufVal, range<1>(1)}; + queue Q; + + { + // This should trigger memory allocation on host since the pointer passed by + // the user is read-only. + auto BufAcc = Buf.get_access(); + } + + Q.submit([&](handler &Cgh) { + // Now that we have a read-write host allocation, check that the native + // buffer is created with the PI_MEM_FLAGS_HOST_PTR_USE flag. + // CHECK: piMemBufferCreate + // CHECK-NEXT: {{.*}} : {{.*}} + // CHECK-NEXT: {{.*}} : 9 + auto BufAcc = Buf.get_access(Cgh); + Cgh.single_task([=]() { int A = BufAcc[0]; }); + }); +} From 90cedff4756e7819baef9835ea82da70641acbf1 Mon Sep 17 00:00:00 2001 From: Sergey Semenov Date: Mon, 2 Nov 2020 13:20:15 +0300 Subject: [PATCH 2/2] Update comment --- sycl/source/detail/buffer_impl.cpp | 6 +++++- sycl/source/detail/image_impl.cpp | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/sycl/source/detail/buffer_impl.cpp b/sycl/source/detail/buffer_impl.cpp index b3e8c64554754..11079a5c0be71 100644 --- a/sycl/source/detail/buffer_impl.cpp +++ b/sycl/source/detail/buffer_impl.cpp @@ -16,7 +16,11 @@ namespace sycl { namespace detail { void *buffer_impl::allocateMem(ContextImplPtr Context, bool InitFromUserData, void *HostPtr, RT::PiEvent &OutEventToWait) { - // Assume a read-write pointer unless reusing a user pointer. + // The host pointer for the allocation can be provided in 2 ways: + // 1. Initialize the allocation from user data. Check if the user pointer is + // read-only. + // 2. Use a HostPtr allocated by the runtime. Assume any such pointer to be + // read-write. bool HostPtrReadOnly = false; if (InitFromUserData) { assert(!HostPtr && "Cannot init from user data and reuse host ptr provided " diff --git a/sycl/source/detail/image_impl.cpp b/sycl/source/detail/image_impl.cpp index bd9f2614ff812..03854d3d8c6e6 100644 --- a/sycl/source/detail/image_impl.cpp +++ b/sycl/source/detail/image_impl.cpp @@ -307,7 +307,11 @@ template void *image_impl::allocateMem(ContextImplPtr Context, bool InitFromUserData, void *HostPtr, RT::PiEvent &OutEventToWait) { - // Assume a read-write pointer unless reusing a user pointer. + // The host pointer for the allocation can be provided in 2 ways: + // 1. Initialize the allocation from user data. Check if the user pointer is + // read-only. + // 2. Use a HostPtr allocated by the runtime. Assume any such pointer to be + // read-write. bool HostPtrReadOnly = false; if (InitFromUserData) { assert(!HostPtr && "Cannot init from user data and reuse host ptr provided "