Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1837703002:
gpu: Export/import buffers created by Ozone GMB factory. (Closed)
|
Created:
4 years, 8 months ago by reveman Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, dnicoara, jam, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: Export/import buffers created by Ozone GMB factory.
This allows GMBs created by one instance of the GPU process to
be used with another instance. It also allows the client to
synchronize creation of one buffer with destruction of another
buffer as they are now created when imported for CHROMIUM_image.
Note: GMB factory creation of buffer from handle can be
eliminated as a follow up to this.
BUG=597932
, b/29239573
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/57111c9ffef56e96560373618aa02756b5194b36
Cr-Commit-Position: refs/heads/master@{#400730}
Patch Set 1 #
Total comments: 3
Patch Set 2 : gpu: Export/import buffers created by Ozone GMB factory. #
Total comments: 8
Patch Set 3 : nits #
Total comments: 6
Patch Set 4 : check result of dup #
Total comments: 2
Patch Set 5 : move dup() to ShareGpuMemoryBufferToGpuProcess #
Messages
Total messages: 44 (16 generated)
The CQ bit was checked by reveman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837703002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1837703002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reveman@chromium.org changed reviewers: + piman@chromium.org
fyi, follow up cleanup that makes import of buffers more efficient: https://codereview.chromium.org/1835173003
https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:78: handle.native_pixmap_handle); Is this safe to do if native_pixmap_handle is untrusted? I think the map was used to check that.
https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:78: handle.native_pixmap_handle); On 2016/03/28 at 16:41:31, piman wrote: > Is this safe to do if native_pixmap_handle is untrusted? I think the map was used to check that. FYI, originally importing a handle was not supported as the handle didn't contain anything but an ID. Now with this import code in place (initially added for components/exo) we can properly pass the ownership with the handle all the way to the renderer. This import code has been used to import untrusted buffers through components/exo so it assumes that the handle can't be trusted to be valid. So to answer your question, yes this is safe. Or it at least has been designed to be safe.
piman@chromium.org changed reviewers: + mdempsky@chromium.org
+mdempsky to comment about the security implications https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:78: handle.native_pixmap_handle); On 2016/03/28 17:38:33, reveman wrote: > On 2016/03/28 at 16:41:31, piman wrote: > > Is this safe to do if native_pixmap_handle is untrusted? I think the map was > used to check that. > > FYI, originally importing a handle was not supported as the handle didn't > contain anything but an ID. Now with this import code in place (initially added > for components/exo) we can properly pass the ownership with the handle all the > way to the renderer. > > This import code has been used to import untrusted buffers through > components/exo so it assumes that the handle can't be trusted to be valid. So to > answer your question, yes this is safe. Or it at least has been designed to be > safe. AFAICT, gbm_bo_import will fail if native_pixmap_handle is not a PRIME fd (although, that is kernel-driver specific, but kernel drivers that use drm_gem_prime_fd_to_handle would, and I think that it's all of them that we use on Chrome OS?), so that part seems fine. But it seems like we're feeding untrusted sizes and formats to gbm, without checks - among others, no protection against overflow at https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/gbm.c#276, and I'm not sure what happens if they don't match the actual buffer. I'm not a expert, so I think I'd prefer having a second pair of eyes from someone better versed in the security implications of drm...
On 2016/03/28 at 20:42:29, piman wrote: > +mdempsky to comment about the security implications > > https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... > File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): > > https://codereview.chromium.org/1837703002/diff/1/content/common/gpu/gpu_memo... > content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:78: handle.native_pixmap_handle); > On 2016/03/28 17:38:33, reveman wrote: > > On 2016/03/28 at 16:41:31, piman wrote: > > > Is this safe to do if native_pixmap_handle is untrusted? I think the map was > > used to check that. > > > > FYI, originally importing a handle was not supported as the handle didn't > > contain anything but an ID. Now with this import code in place (initially added > > for components/exo) we can properly pass the ownership with the handle all the > > way to the renderer. > > > > This import code has been used to import untrusted buffers through > > components/exo so it assumes that the handle can't be trusted to be valid. So to > > answer your question, yes this is safe. Or it at least has been designed to be > > safe. > > AFAICT, gbm_bo_import will fail if native_pixmap_handle is not a PRIME fd (although, that is kernel-driver specific, but kernel drivers that use drm_gem_prime_fd_to_handle would, and I think that it's all of them that we use on Chrome OS?), so that part seems fine. But it seems like we're feeding untrusted sizes and formats to gbm, without checks - among others, no protection against overflow at https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/gbm.c#276, and I'm not sure what happens if they don't match the actual buffer. > > I'm not a expert, so I think I'd prefer having a second pair of eyes from someone better versed in the security implications of drm... Sounds good. We want this to be secure even without this change so we should probably add some size+formats checks before calling gbm if that's not expected to be handled by gbm in a secure way.
Description was changed from ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932 ========== to ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
reveman@chromium.org changed reviewers: + dcastagna@chromium.org
+dcastagna This will allow Arc++ to survive GPU process crashes as buffer ownership is now passed with the handle in the exact same way we pass ownership of shared memory backed GMBs. I had to include gmb_bo_import support as part of this patch as it's an obvious requirement for passing ownership using the FDs and we can unfortunately not land gmb_bo_import support before this as it would create a problem where we'd end up importing the same GEM handle twice as a result of GMB destruction being delayed when we're not passing ownership with FDs.
Description was changed from ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932, b/29239573 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by reveman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837703002/20001
https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD fd) nit: Make this rvalue reference or pointer, so it's clear from the caller POV that this is taking owner https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:60: base::FileDescriptor(HANDLE_EINTR(dup(scoped_fd.get())), true)); nit: you can omit base::FileDescriptor. But leaving it can be useful to see the type you're pushing back. https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/service/gpu_mem... File gpu/ipc/service/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/service/gpu_mem... gpu/ipc/service/gpu_memory_buffer_factory_ozone_native_pixmap.cc:54: int client_id) { Maybe add a todo to remove this method, since this is not doing much at this point. https://codereview.chromium.org/1837703002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1837703002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.cc:175: if (use_scanout && !buffer->GetFramebufferId()) nit: I'd add a comment explaning why the !framebufferid check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD fd) On 2016/06/16 at 21:14:26, Daniele Castagna wrote: > nit: Make this rvalue reference or pointer, so it's clear from the caller POV that this is taking owner done https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:60: base::FileDescriptor(HANDLE_EINTR(dup(scoped_fd.get())), true)); On 2016/06/16 at 21:14:26, Daniele Castagna wrote: > nit: you can omit base::FileDescriptor. But leaving it can be useful to see the type you're pushing back. Done. https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/service/gpu_mem... File gpu/ipc/service/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/20001/gpu/ipc/service/gpu_mem... gpu/ipc/service/gpu_memory_buffer_factory_ozone_native_pixmap.cc:54: int client_id) { On 2016/06/16 at 21:14:26, Daniele Castagna wrote: > Maybe add a todo to remove this method, since this is not doing much at this point. Done. https://codereview.chromium.org/1837703002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1837703002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.cc:175: if (use_scanout && !buffer->GetFramebufferId()) On 2016/06/16 at 21:14:26, Daniele Castagna wrote: > nit: I'd add a comment explaning why the !framebufferid check. Added a comment. The check is consistent with GbmBuffer::CreateBuffer and we can't crash here as the buffer might come from an untrusted client.
lgtm
reveman@chromium.org changed reviewers: + dnicoara@chromium.org
+dnicoara mdempsky, dnicoara, please take a look.
The CQ bit was checked by reveman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837703002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for security https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD&& fd) nit: I think just "base::ScopedFD fd" is preferable here?
reveman@chromium.org changed reviewers: + spang@chromium.org - dnicoara@chromium.org
+spang for ui/ozone/platform/drm/gpu ui/ozone part is really just a revert of: https://codereview.chromium.org/1834643002
lgtm https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD&& fd) On 2016/06/17 19:38:19, mdempsky wrote: > nit: I think just "base::ScopedFD fd" is preferable here? +1 https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:59: native_pixmap_handle.fds.emplace_back(HANDLE_EINTR(dup(scoped_fd.get())), dup might fail with EMFILE.. I think we should be checking for this instead of pushing -1 into the vector.
https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD&& fd) On 2016/06/17 at 20:00:25, spang wrote: > On 2016/06/17 19:38:19, mdempsky wrote: > > nit: I think just "base::ScopedFD fd" is preferable here? > > +1 I assume this is for consistency with existing code in chromium? Initial patch had "base::ScopedFD fd" but dcastagna@ asked me to use a rvalue reference here to make it more clear that it's being passed. Either way is fine with me but I changed to "base::ScopedFD fd" in latest patch as the majority seems to prefer that. https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:59: native_pixmap_handle.fds.emplace_back(HANDLE_EINTR(dup(scoped_fd.get())), On 2016/06/17 at 20:00:25, spang wrote: > dup might fail with EMFILE.. I think we should be checking for this instead of pushing -1 into the vector. Done.
https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/40001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: base::ScopedFD&& fd) On 2016/06/17 20:34:02, reveman wrote: > On 2016/06/17 at 20:00:25, spang wrote: > > On 2016/06/17 19:38:19, mdempsky wrote: > > > nit: I think just "base::ScopedFD fd" is preferable here? > > > > +1 > > I assume this is for consistency with existing code in chromium? Initial patch > had "base::ScopedFD fd" but dcastagna@ asked me to use a rvalue reference here > to make it more clear that it's being passed. Either way is fine with me but I > changed to "base::ScopedFD fd" in latest patch as the majority seems to prefer > that. Not just consistency, this usage is against the style guide. "Use rvalue references only to define move constructors and move assignment operators" (This is a constructor but it isn't the move constructor) Accepting a move constructed argument by value is the approved way.
https://codereview.chromium.org/1837703002/diff/60001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/60001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:139: base::ScopedFD scoped_fd(HANDLE_EINTR(dup(fd_.get()))); Should this really dup? I'm concerned that there are callers that will not release the handle(s). See e.g. https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?sq=pac... or https://cs.chromium.org/chromium/src/gpu/ipc/client/command_buffer_proxy_impl... auto_close=true only does anything when sending via IPC.
ptal https://codereview.chromium.org/1837703002/diff/60001/gpu/ipc/client/gpu_memo... File gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1837703002/diff/60001/gpu/ipc/client/gpu_memo... gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:139: base::ScopedFD scoped_fd(HANDLE_EINTR(dup(fd_.get()))); On 2016/06/20 at 15:03:13, piman OOO back 2016-06-20 wrote: > Should this really dup? I'm concerned that there are callers that will not release the handle(s). See e.g. https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?sq=pac... or https://cs.chromium.org/chromium/src/gpu/ipc/client/command_buffer_proxy_impl... > > auto_close=true only does anything when sending via IPC. Good call. I moved the dup logic to GpuChannelHost::ShareGpuMemoryBufferToGpuProcess in latest patch where it belongs. Ozone native pixmaps should now be consistent with shmem buffers. FYI, another major benefit of this change is that we no longer need sync points when creating and destroying GL images.
lgtm
The CQ bit was checked by reveman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdempsky@chromium.org, dcastagna@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1837703002/#ps80001 (title: "move dup() to ShareGpuMemoryBufferToGpuProcess")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837703002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932, b/29239573 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Export/import buffers created by Ozone GMB factory. This allows GMBs created by one instance of the GPU process to be used with another instance. It also allows the client to synchronize creation of one buffer with destruction of another buffer as they are now created when imported for CHROMIUM_image. Note: GMB factory creation of buffer from handle can be eliminated as a follow up to this. BUG=597932, b/29239573 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/57111c9ffef56e96560373618aa02756b5194b36 Cr-Commit-Position: refs/heads/master@{#400730} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/57111c9ffef56e96560373618aa02756b5194b36 Cr-Commit-Position: refs/heads/master@{#400730} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
