Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1253363004:
ozone gbm: whitelist 3 DRM ioctl code for native GpuMemoryBuffer. (Closed)
|
Created:
5 years, 4 months ago by dshwang Modified:
5 years, 2 months ago Reviewers:
reveman, skyguru, mdempsky, vignatti (out of this project), jbudorick, Jorge Lucangeli Obes, Nico, jln (very slow on Chromium), Robert Sesek, marcheu, brettw, spang CC:
chromium-reviews, rickyz+watch_chromium.org, jln+watch_chromium.org, Kees Cook Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionozone gbm: whitelist 3 DRM ioctl code for native GpuMemoryBuffer.
BUG=475633
Committed: https://crrev.com/2a785aa5f32b793fb51898f82efef096abab6102
Cr-Commit-Position: refs/heads/master@{#354794}
Patch Set 1 #Patch Set 2 : rebase to ToT #
Total comments: 2
Patch Set 3 : use "unsigned int" #
Total comments: 3
Patch Set 4 : move code to content/ #
Total comments: 12
Patch Set 5 : add comments for USE_VGEM_MAP define #
Total comments: 1
Patch Set 6 : define use_vgem_map in single place #
Total comments: 4
Patch Set 7 : Arg<unsigned long> #Patch Set 8 : rebase to ToT #Patch Set 9 : move vgem_map target into ozone.gyp #
Total comments: 1
Patch Set 10 : remove redundant change in build/ #Patch Set 11 : fix chromeos_daisy_chromium_compile_only_ng build failure #
Messages
Total messages: 64 (17 generated)
dongseong.hwang@intel.com changed reviewers: + brettw@chromium.org, spang@chromium.org
brettw, spang, could you review? It's extracted from https://codereview.chromium.org/1134993003/#msg37 as spang requested. zero-copy texture for ChromeOS requires these new IOCTL code in the same manner that Android requires to allow PR_SET_TIMERSLACK_PID PRCTL call.
spang@chromium.org changed reviewers: + marcheu@chromium.org
https://codereview.chromium.org/1253363004/diff/20001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (right): https://codereview.chromium.org/1253363004/diff/20001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:160: auto reference_type = TCGETS; If decltype(TCGETS) is actually too narrow, then it isn't correct and should never be used. Maybe we should just cast to "unsigned long" like the ioctl() declaration actually says?
https://codereview.chromium.org/1253363004/diff/20001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (right): https://codereview.chromium.org/1253363004/diff/20001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:160: auto reference_type = TCGETS; On 2015/08/21 15:55:15, spang wrote: > If decltype(TCGETS) is actually too narrow, then it isn't correct and should > never be used. > > Maybe we should just cast to "unsigned long" like the ioctl() declaration > actually says? that's better idea. ioctl() gets "unsigned long" request, but TCGETS macro is just int. Done.
spang@chromium.org changed reviewers: + jln@chromium.org, jorgelo@chromium.org - brettw@chromium.org
+security/sandbox OWNERS These are the ioctls needed for zero copy rendering via vgem on CrOS.
https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (right): https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:158: .CASES((static_cast<unsigned int>(TCGETS), FIONREAD), Allow()) Why unsigned int rather than unsigned long? The actual parameter type is "unsigned long".
On 2015/08/21 17:12:50, spang wrote: > +security/sandbox OWNERS > > These are the ioctls needed for zero copy rendering via vgem on CrOS. This would affect every policy in Chrome. What type of process do you need this in, besides the GPU process (which does not have ioctl restrictions)? I would prefer if you modified one of the policies in content/common/sandbox_linux instead. Do we have any influence in how these kernel interfaces are designed? This design is very sandbox-unfriendly: we can't filter ioctls per fd type. So allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal file descriptors. Because of collisions in ioctl values between drivers we don't really know what this could mean. If you have any influence on the kernel interface, would it be possible to add new system calls? Another possibility would be to force the |request| parameter to be a fixed random value (and move your other arguments to other parameters).
On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > On 2015/08/21 17:12:50, spang wrote: > > +security/sandbox OWNERS > > > > These are the ioctls needed for zero copy rendering via vgem on CrOS. > > This would affect every policy in Chrome. What type of process do you need this > in, besides the GPU process (which does not have ioctl restrictions)? > > I would prefer if you modified one of the policies in > content/common/sandbox_linux instead. > Julien is correct, this should be overridden in specific policies, not modified here. > Do we have any influence in how these kernel interfaces are designed? This > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > file descriptors. Because of collisions in ioctl values between drivers we don't > really know what this could mean. > If you have any influence on the kernel interface, would it be possible to add > new system calls? Another possibility would be to force the |request| parameter > to be a fixed random value (and move your other arguments to other parameters).
On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > On 2015/08/21 17:12:50, spang wrote: > > +security/sandbox OWNERS > > > > These are the ioctls needed for zero copy rendering via vgem on CrOS. > > This would affect every policy in Chrome. What type of process do you need this > in, besides the GPU process (which does not have ioctl restrictions)? The purpose of the feature is to allow uploads from renderers. > > I would prefer if you modified one of the policies in > content/common/sandbox_linux instead. > > Do we have any influence in how these kernel interfaces are designed? This > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > file descriptors. Because of collisions in ioctl values between drivers we don't > really know what this could mean. > If you have any influence on the kernel interface, would it be possible to add > new system calls? Another possibility would be to force the |request| parameter > to be a fixed random value (and move your other arguments to other parameters). Stephane can probably comment here.
mdempsky@chromium.org changed reviewers: + mdempsky@chromium.org
https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (right): https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:158: .CASES((static_cast<unsigned int>(TCGETS), FIONREAD), Allow()) On 2015/08/21 17:28:51, spang wrote: > Why unsigned int rather than unsigned long? The actual parameter type is > "unsigned long". Annoying. POSIX and the Linux man pages say request is an "int", but Linux <sys/ioctl.h> uses "unsigned long int". FWIW, the consequence of Arg<32-bit> vs Arg<64-bit> here is whether the compiled BPF program will include extra checks to reject "impossible" 64-bit arguments (by crashing). But since the default case is to crash anyway, there's not much practical consequence.
spang@chromium.org changed reviewers: + tiago.vignatti@intel.com
On 2015/08/21 17:36:04, spang wrote: > On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > > On 2015/08/21 17:12:50, spang wrote: > > > +security/sandbox OWNERS > > > > > > These are the ioctls needed for zero copy rendering via vgem on CrOS. > > > > This would affect every policy in Chrome. What type of process do you need > this > > in, besides the GPU process (which does not have ioctl restrictions)? > > The purpose of the feature is to allow uploads from renderers. > > > > > I would prefer if you modified one of the policies in > > content/common/sandbox_linux instead. > > > > Do we have any influence in how these kernel interfaces are designed? This > > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > > file descriptors. Because of collisions in ioctl values between drivers we > don't > > really know what this could mean. > > If you have any influence on the kernel interface, would it be possible to add > > new system calls? Another possibility would be to force the |request| > parameter > > to be a fixed random value (and move your other arguments to other > parameters). > > Stephane can probably comment here. +tiago.vignatti There's actually some ongoing upstream discussions on this as well: http://lists.freedesktop.org/archives/dri-devel/2015-August/088259.html [dmabuf mmap] http://lists.freedesktop.org/archives/dri-devel/2015-August/088809.html [dmabuf sync] If I'm understanding that correctly it would replace these ioctls with a combination of regular mmap() + dmabuf sync API. It may be worthwhile to give feedback there that userspace dmabuf sync API should be a syscall rather than ioctl.
Patchset #4 (id:60001) has been deleted
On 2015/08/21 18:11:54, spang wrote: > On 2015/08/21 17:36:04, spang wrote: > > On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > > > On 2015/08/21 17:12:50, spang wrote: > > > > +security/sandbox OWNERS > > > > > > > > These are the ioctls needed for zero copy rendering via vgem on CrOS. > > > > > > This would affect every policy in Chrome. What type of process do you need > > this > > > in, besides the GPU process (which does not have ioctl restrictions)? > > > > The purpose of the feature is to allow uploads from renderers. > > > > > > > > I would prefer if you modified one of the policies in > > > content/common/sandbox_linux instead. Yes, Done. I followed Android code in sandbox/. Android code should move to content/ as well. > > > > > > Do we have any influence in how these kernel interfaces are designed? This > > > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > > > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, > terminal > > > file descriptors. Because of collisions in ioctl values between drivers we > > don't > > > really know what this could mean. > > > If you have any influence on the kernel interface, would it be possible to > add > > > new system calls? Another possibility would be to force the |request| > > parameter > > > to be a fixed random value (and move your other arguments to other > > parameters). > > > > Stephane can probably comment here. > > +tiago.vignatti > > There's actually some ongoing upstream discussions on this as well: > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088259.html [dmabuf > mmap] > http://lists.freedesktop.org/archives/dri-devel/2015-August/088809.html [dmabuf > sync] > > If I'm understanding that correctly it would replace these ioctls with a > combination of regular mmap() + dmabuf sync API. It may be worthwhile to give > feedback there that userspace dmabuf sync API should be a syscall rather than > ioctl. dmabuf sync API would be IOCTL call. In addition, after the linux patch is landed, DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are still needed. The goal of linux effort is to remove VGEM with DRM_IOCTL_VGEM_MODE_MAP_DUMB.
https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... File sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc (right): https://codereview.chromium.org/1253363004/diff/40001/sandbox/linux/seccomp-b... sandbox/linux/seccomp-bpf-helpers/syscall_parameters_restrictions.cc:158: .CASES((static_cast<unsigned int>(TCGETS), FIONREAD), Allow()) On 2015/08/21 17:39:47, mdempsky wrote: > On 2015/08/21 17:28:51, spang wrote: > > Why unsigned int rather than unsigned long? The actual parameter type is > > "unsigned long". > > Annoying. POSIX and the Linux man pages say request is an "int", but Linux > <sys/ioctl.h> uses "unsigned long int". > > FWIW, the consequence of Arg<32-bit> vs Arg<64-bit> here is whether the compiled > BPF program will include extra checks to reject "impossible" 64-bit arguments > (by crashing). But since the default case is to crash anyway, there's not much > practical consequence. Thx for investigation. I rollback this code to ensure both POSIX and Linux.
On 2015/08/21 18:25:42, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/21 18:11:54, spang wrote: > > On 2015/08/21 17:36:04, spang wrote: > > > On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > > > > On 2015/08/21 17:12:50, spang wrote: > > > > > +security/sandbox OWNERS > > > > > > > > > > These are the ioctls needed for zero copy rendering via vgem on CrOS. > > > > > > > > This would affect every policy in Chrome. What type of process do you need > > > this > > > > in, besides the GPU process (which does not have ioctl restrictions)? > > > > > > The purpose of the feature is to allow uploads from renderers. > > > > > > > > > > > I would prefer if you modified one of the policies in > > > > content/common/sandbox_linux instead. > > Yes, Done. I followed Android code in sandbox/. Android code should move to > content/ as well. > > > > > > > > > Do we have any influence in how these kernel interfaces are designed? This > > > > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > > > > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, > > terminal > > > > file descriptors. Because of collisions in ioctl values between drivers we > > > don't > > > > really know what this could mean. > > > > If you have any influence on the kernel interface, would it be possible to > > add > > > > new system calls? Another possibility would be to force the |request| > > > parameter > > > > to be a fixed random value (and move your other arguments to other > > > parameters). > > > > > > Stephane can probably comment here. > > > > +tiago.vignatti > > > > There's actually some ongoing upstream discussions on this as well: > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088259.html > [dmabuf > > mmap] > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088809.html > [dmabuf > > sync] > > > > If I'm understanding that correctly it would replace these ioctls with a > > combination of regular mmap() + dmabuf sync API. It may be worthwhile to give > > feedback there that userspace dmabuf sync API should be a syscall rather than > > ioctl. > > dmabuf sync API would be IOCTL call. It's not merged yet.. now is the time for feedback if ioctl makes it difficult for us to actually use. > In addition, after the linux patch is > landed, DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are still needed. > The goal of linux effort is to remove VGEM with DRM_IOCTL_VGEM_MODE_MAP_DUMB.
On 2015/08/21 18:38:23, spang wrote: > On 2015/08/21 18:25:42, dshwang_ooo_5.9-27.9 wrote: > > > If I'm understanding that correctly it would replace these ioctls with a > > > combination of regular mmap() + dmabuf sync API. It may be worthwhile to > give > > > feedback there that userspace dmabuf sync API should be a syscall rather > than > > > ioctl. > > > > dmabuf sync API would be IOCTL call. > > It's not merged yet.. now is the time for feedback if ioctl makes it difficult > for us to actually use. (+ keescook FYI) Is one of you a good person to give that feedback? I thought in general Linux was trying to not add more ioctl/multiplexing system calls these days. > > In addition, after the linux patch is > > landed, DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are still needed. > > The goal of linux effort is to remove VGEM with DRM_IOCTL_VGEM_MODE_MAP_DUMB.
On 2015/08/21 18:52:23, jln (slow on Chromium) wrote: > On 2015/08/21 18:38:23, spang wrote: > > On 2015/08/21 18:25:42, dshwang_ooo_5.9-27.9 wrote: > > > > > If I'm understanding that correctly it would replace these ioctls with a > > > > combination of regular mmap() + dmabuf sync API. It may be worthwhile to > > give > > > > feedback there that userspace dmabuf sync API should be a syscall rather > > than > > > > ioctl. > > > > > > dmabuf sync API would be IOCTL call. > > > > It's not merged yet.. now is the time for feedback if ioctl makes it difficult > > for us to actually use. > > (+ keescook FYI) > > Is one of you a good person to give that feedback? I thought in general Linux > was trying to not add more ioctl/multiplexing system calls these days. yes, I'm on it.
mmap on dma-buf may take long time to get brushed, specially because all the driver implementors needs to agree with the idea, which is a *painful* journey. OTOH we been working already for a few months to get VGEM enabled in Chrome OS. Hopefully we'll need couple of CLs more only to get the basics to activate it, and that will bring "lots of" performance & power saving benefits for Chrome OS; the final "before Vs. after" results is a WIP in our table at the moment. That said, I understand that the policy in Chrome would have to be discussed all over again, but that has to be very carefully balanced with the benefits VGEM system will be bringing against the time to design "the next" shared memory allocator. About having a syscall instead an ioctl, it's a good point and I'll keep that in mind. Thank you.
brettw@chromium.org changed reviewers: + brettw@chromium.org
https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:9: import("//ui/ozone/ozone.gni") A better way to do this would be to add a config //ui/ozone:vgem_map and defines USE_VGEM_MAP or not depending on the value of the flag. That way you don't need to import this file and logic about these defines isn't duplicated. https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:11: use_vgem_map = false Can you call this ozone_use_vgem_map instead? Does this ozone name make sense in the way that you use it? I'd like to give hints about what flags affect in their name. If this is easy (i.e. bots don't depend on the old name now) then it would be nice to rename even if we keep it in the BUILD.gn file according to my other comment.
https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:11: use_vgem_map = false On 2015/08/24 21:53:38, brettw wrote: > Can you call this ozone_use_vgem_map instead? Does this ozone name make sense in > the way that you use it? I'd like to give hints about what flags affect in their > name. If this is easy (i.e. bots don't depend on the old name now) then it would > be nice to rename even if we keep it in the BUILD.gn file according to my other > comment. The scope is narrower than ozone, it applies to Chrome OS kernel in particular. If we want some more descriptive words, maybe add "linux kernel" or "chromeos kernel" ?
Maybe just add a meaningful command about what this flag actually means and what vgem is it would be fine.
On 2015/08/24 22:23:17, brettw wrote:
> Maybe just add a meaningful command about what this flag actually means and
what
> vgem is it would be fine.
SGTM. Something like:
# This enables memory-mapped access to accelerated graphics buffers via the VGEM
("virtual GEM") driver.
# This is currently only available on Chrome OS kernels and affects code in the
GBM ozone platform.
On 2015/08/24 22:28:07, spang wrote:
> On 2015/08/24 22:23:17, brettw wrote:
> > Maybe just add a meaningful command about what this flag actually means and
> what
> > vgem is it would be fine.
>
> SGTM. Something like:
>
> # This enables memory-mapped access to accelerated graphics buffers via the
VGEM
> ("virtual GEM") driver.
> # This is currently only available on Chrome OS kernels and affects code in
the
> GBM ozone platform.
That sounds fine.
dongseong.hwang@intel.com changed reviewers: + robert.fekete@intel.com
dongseong.hwang@intel.com changed reviewers: + reveman@chromium.org
On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > Do we have any influence in how these kernel interfaces are designed? This > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > file descriptors. Because of collisions in ioctl values between drivers we don't > really know what this could mean. +skyguru skyguru explained in offline that sane driver must not conflict with drm ioctl code. drm ioctl is assigned to ioctl base 'd'; https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... so drm header defines all ioctl code with offset by 'd'; http://lxr.free-electrons.com/source/include/uapi/drm/drm.h#L684 As you may concern pcmcia/ds.h is conflicted with drm ioctl in ioctl-number.txt#L210, but pcmcia ioctl is removed since 2008; https://wiki.linuxfoundation.org/en/PCMCIA "F0-FF linux/digi1.h" can be conflicted but at least this CL doesn't whitelist range FO-FF. So if we can make sure there aren't any insane thirdparty drivers which use the ioctl code range illegally, it's safe. This CL tries to whitelist three codes; DRM_IOCTL_GEM_CLOSE, DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are defined in drm.h DRM_IOCTL_VGEM_MODE_MAP_DUMB is typedef of DRM_VGEM_MODE_MAP_DUMB which is defined in drm.h https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/d5... ChromeOS linux kernel is under control of chromium development. If we are sure there is not insane drivers, IMHO this CL is fine. Jln, spang, WDYT? On 2015/08/21 18:38:23, spang wrote: > > > > If you have any influence on the kernel interface, would it be possible to add > > > > new system calls? Another possibility would be to force the |request| parameter > > > > to be a fixed random value (and move your other arguments to other parameters). > > > +tiago.vignatti > > > > > > There's actually some ongoing upstream discussions on this as well: > > > > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088259.html > > [dmabuf > > > mmap] > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088809.html > > [dmabuf > > > sync] > > > > > > If I'm understanding that correctly it would replace these ioctls with a > > > combination of regular mmap() + dmabuf sync API. It may be worthwhile to > give > > > feedback there that userspace dmabuf sync API should be a syscall rather > than > > > ioctl. > > > > dmabuf sync API would be IOCTL call. > > It's not merged yet.. now is the time for feedback if ioctl makes it difficult > for us to actually use. Tiago is trying to make it via mmap-msync-munmap flow. https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:11: use_vgem_map = false On 2015/08/24 22:09:09, spang wrote: > On 2015/08/24 21:53:38, brettw wrote: > > Can you call this ozone_use_vgem_map instead? Does this ozone name make sense > in > > the way that you use it? I'd like to give hints about what flags affect in > their > > name. If this is easy (i.e. bots don't depend on the old name now) then it > would > > be nice to rename even if we keep it in the BUILD.gn file according to my > other > > comment. > > The scope is narrower than ozone, it applies to Chrome OS kernel in particular. > > If we want some more descriptive words, maybe add "linux kernel" or "chromeos > kernel" ? got it. I'll rename it to CHROMEOS_USE_VGEM_MAP with comments # This enables memory-mapped access to accelerated graphics buffers via the VGEM ("virtual GEM") driver. # This is currently only available on Chrome OS kernels and affects code in the GBM ozone platform.
https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:9: import("//ui/ozone/ozone.gni") On 2015/08/24 21:53:38, brettw wrote: > A better way to do this would be to add a config > //ui/ozone:vgem_map > > and defines USE_VGEM_MAP or not depending on the value of the flag. That way you > don't need to import this file and logic about these defines isn't duplicated. AFAIK, there is no way to inherit DEFINE value from dependencies. https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/80001/ui/ozone/ozone.gni#newc... ui/ozone/ozone.gni:11: use_vgem_map = false On 2015/08/26 12:02:43, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/24 22:09:09, spang wrote: > > On 2015/08/24 21:53:38, brettw wrote: > > > Can you call this ozone_use_vgem_map instead? Does this ozone name make > sense > > in > > > the way that you use it? I'd like to give hints about what flags affect in > > their > > > name. If this is easy (i.e. bots don't depend on the old name now) then it > > would > > > be nice to rename even if we keep it in the BUILD.gn file according to my > > other > > > comment. > > > > The scope is narrower than ozone, it applies to Chrome OS kernel in > particular. > > > > If we want some more descriptive words, maybe add "linux kernel" or "chromeos > > kernel" ? > > got it. I'll rename it to CHROMEOS_USE_VGEM_MAP with comments > # This enables memory-mapped access to accelerated graphics buffers via the VGEM > ("virtual GEM") driver. > # This is currently only available on Chrome OS kernels and affects code in the > GBM ozone platform. I didn't rename it. done to update comments. https://codereview.chromium.org/1253363004/diff/100001/content/content_common... File content/content_common.gypi (right): https://codereview.chromium.org/1253363004/diff/100001/content/content_common... content/content_common.gypi:39: 'use_vgem_map%': 0, to avoid this duplication, we can define it in build/common.gypi spang, wdyt?
On 2015/08/26 12:02:43, dshwang_ooo_5.9-27.9 wrote: > Tiago is trying to make it via mmap-msync-munmap flow. not really. I'm just exploring this option.
https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:9: import("//ui/ozone/ozone.gni") In ui/ozone/BUILD.gn: config("vgem_map") { if (use_vgem_map) { defines = [ "USE_VGEM_MAP" ] } } and in the targets you want to define this define in: configs += [ "//ui/ozone:vgem_map" ] in all cases. Now the conditional logic and define value is in one place. https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:31: namespace { Blank line after here. https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:50: } Blank line before here, and this line should look like: } // namespace
On 2015/08/26 12:02:43, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > > Do we have any influence in how these kernel interfaces are designed? This > > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > > file descriptors. Because of collisions in ioctl values between drivers we > don't > > really know what this could mean. > > +skyguru > skyguru explained in offline that sane driver must not conflict with drm ioctl > code. > drm ioctl is assigned to ioctl base 'd'; > https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... > so drm header defines all ioctl code with offset by 'd'; > http://lxr.free-electrons.com/source/include/uapi/drm/drm.h#L684 > As you may concern pcmcia/ds.h is conflicted with drm ioctl in > ioctl-number.txt#L210, but pcmcia ioctl is removed since 2008; > https://wiki.linuxfoundation.org/en/PCMCIA > "F0-FF linux/digi1.h" can be conflicted but at least this CL doesn't whitelist > range FO-FF. > So if we can make sure there aren't any insane thirdparty drivers which use the > ioctl code range illegally, it's safe. > > This CL tries to whitelist three codes; DRM_IOCTL_GEM_CLOSE, > DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE > DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are defined in drm.h > DRM_IOCTL_VGEM_MODE_MAP_DUMB is typedef of DRM_VGEM_MODE_MAP_DUMB which is > defined in drm.h > https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/d5... > > ChromeOS linux kernel is under control of chromium development. If we are sure > there is not insane drivers, IMHO this CL is fine. > Jln, spang, WDYT? Jln, could I listen to your opinion? https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/BUILD.gn... content/common/BUILD.gn:9: import("//ui/ozone/ozone.gni") On 2015/08/26 22:27:49, brettw wrote: > In ui/ozone/BUILD.gn: > config("vgem_map") { > if (use_vgem_map) { > defines = [ "USE_VGEM_MAP" ] > } > } > > and in the targets you want to define this define in: > configs += [ "//ui/ozone:vgem_map" ] > in all cases. > > Now the conditional logic and define value is in one place. Oh, there is a way. Thank you for explaining! Now define USE_VGEM_MAP only once in both gn and gyp https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:31: namespace { On 2015/08/26 22:27:49, brettw wrote: > Blank line after here. Done. https://codereview.chromium.org/1253363004/diff/80001/content/common/sandbox_... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:50: } On 2015/08/26 22:27:49, brettw wrote: > Blank line before here, and this line should look like: > > } // namespace Done.
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/1253363004/diff/160001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/160001/ui/ozone/ozone.gni#new... ui/ozone/ozone.gni:14: use_vgem_map = false You can move this back to the old place now.
On 2015/08/27 12:47:09, dshwang_ooo_5.9-27.9 wrote: > On 2015/08/26 12:02:43, dshwang_ooo_5.9-27.9 wrote: > > On 2015/08/21 17:30:17, jln (slow on Chromium) wrote: > > > Do we have any influence in how these kernel interfaces are designed? This > > > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > > > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, > terminal > > > file descriptors. Because of collisions in ioctl values between drivers we > > don't > > > really know what this could mean. > > > > +skyguru > > skyguru explained in offline that sane driver must not conflict with drm ioctl > > code. > > drm ioctl is assigned to ioctl base 'd'; > > > https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... > > so drm header defines all ioctl code with offset by 'd'; > > http://lxr.free-electrons.com/source/include/uapi/drm/drm.h#L684 > > As you may concern pcmcia/ds.h is conflicted with drm ioctl in > > ioctl-number.txt#L210, but pcmcia ioctl is removed since 2008; > > https://wiki.linuxfoundation.org/en/PCMCIA > > "F0-FF linux/digi1.h" can be conflicted but at least this CL doesn't whitelist > > range FO-FF. > > So if we can make sure there aren't any insane thirdparty drivers which use > the > > ioctl code range illegally, it's safe. > > > > This CL tries to whitelist three codes; DRM_IOCTL_GEM_CLOSE, > > DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE > > DRM_IOCTL_GEM_CLOSE and DRM_IOCTL_PRIME_FD_TO_HANDLE are defined in drm.h > > DRM_IOCTL_VGEM_MODE_MAP_DUMB is typedef of DRM_VGEM_MODE_MAP_DUMB which is > > defined in drm.h > > > https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/d5... > > > > ChromeOS linux kernel is under control of chromium development. If we are sure > > there is not insane drivers, IMHO this CL is fine. > > Jln, spang, WDYT? > > Jln, could I listen to your opinion? Jln, ping. https://codereview.chromium.org/1253363004/diff/160001/ui/ozone/ozone.gni File ui/ozone/ozone.gni (right): https://codereview.chromium.org/1253363004/diff/160001/ui/ozone/ozone.gni#new... ui/ozone/ozone.gni:14: use_vgem_map = false On 2015/08/28 19:21:43, brettw wrote: > You can move this back to the old place now. If I move this to ui/ozone/platform/drm/BUILD.gn (i.e. old place), I encounter following error. ERROR at //ui/ozone/BUILD.gn:49:7: Undefined identifier if (use_vgem_map) { ^-----------
This is the right place to add the ioctl checks, so LGTM for that. I briefly skimmed the kernel code for the three DRM_IOCTLs, and by themselves they didn't seem too scary. Relatively straight forward without any fancy flags/knobs to worry about. I didn't fully grok what they do though. https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:35: // The type of DRM_IOCTL_XXX macro is long unsigned int. For simplicity, I would suggest writing this as just: const Arg<unsigned long> request(1); return Switch(request) .CASES((static_cast<unsigned long>(TCGETS), FIONREAD), Allow()) #if defined(USE_VGEM_MAP) .CASES((DRM_IOCTL_GEM_CLOSE, ...), Allow()) #endif .Default(sandbox::CrashSIGSYSIoctl()); There's no need to vary the Arg type parameter based on USE_VGEM_MAP. (The static_cast is annoying, but unavoidable until we're allowed to use <initializer_list>. :-/)
dongseong.hwang@intel.com changed reviewers: + rsesek@chromium.org
On 2015/09/02 22:19:39, mdempsky wrote: > This is the right place to add the ioctl checks, so LGTM for that. > > I briefly skimmed the kernel code for the three DRM_IOCTLs, and by themselves > they didn't seem too scary. Relatively straight forward without any fancy > flags/knobs to worry about. I didn't fully grok what they do though. Thank you for surveying! jln, rsesek, could you review? DRM_IOCTL has unique code if chromeos linux kernel didn't load insane thirdparty driver according to my investigation https://codereview.chromium.org/1253363004/#msg32 DRM_IOCTL_XX doesn't have issue more than TCGETS and FIONREAD, which we allow already. In addition, TCGETS and FIONREAD is based on unique ('T'), which conflict with sound; https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... content/common/sandbox_linux/bpf_renderer_policy_linux.cc:35: // The type of DRM_IOCTL_XXX macro is long unsigned int. On 2015/09/02 22:19:38, mdempsky wrote: > For simplicity, I would suggest writing this as just: > > const Arg<unsigned long> request(1); > return Switch(request) > .CASES((static_cast<unsigned long>(TCGETS), FIONREAD), Allow()) > #if defined(USE_VGEM_MAP) > .CASES((DRM_IOCTL_GEM_CLOSE, ...), Allow()) > #endif > .Default(sandbox::CrashSIGSYSIoctl()); > > There's no need to vary the Arg type parameter based on USE_VGEM_MAP. > > (The static_cast is annoying, but unavoidable until we're allowed to use > <initializer_list>. :-/) Thx for taking your time. Done.
Patchset #7 (id:180001) has been deleted
On 2015/09/03 08:38:04, dshwang wrote: > On 2015/09/02 22:19:39, mdempsky wrote: > > This is the right place to add the ioctl checks, so LGTM for that. > > > > I briefly skimmed the kernel code for the three DRM_IOCTLs, and by themselves > > they didn't seem too scary. Relatively straight forward without any fancy > > flags/knobs to worry about. I didn't fully grok what they do though. > > Thank you for surveying! > > jln, rsesek, could you review? > > DRM_IOCTL has unique code if chromeos linux kernel didn't load insane thirdparty > driver according to my investigation > https://codereview.chromium.org/1253363004/#msg32 > > DRM_IOCTL_XX doesn't have issue more than TCGETS and FIONREAD, which we allow > already. > In addition, TCGETS and FIONREAD is based on unique ('T'), which conflict with > sound; > https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > content/common/sandbox_linux/bpf_renderer_policy_linux.cc:35: // The type of > DRM_IOCTL_XXX macro is long unsigned int. > On 2015/09/02 22:19:38, mdempsky wrote: > > For simplicity, I would suggest writing this as just: > > > > const Arg<unsigned long> request(1); > > return Switch(request) > > .CASES((static_cast<unsigned long>(TCGETS), FIONREAD), Allow()) > > #if defined(USE_VGEM_MAP) > > .CASES((DRM_IOCTL_GEM_CLOSE, ...), Allow()) > > #endif > > .Default(sandbox::CrashSIGSYSIoctl()); > > > > There's no need to vary the Arg type parameter based on USE_VGEM_MAP. > > > > (The static_cast is annoying, but unavoidable until we're allowed to use > > <initializer_list>. :-/) > > Thx for taking your time. Done. jln, rsesek, could you review?
On 2015/09/29 11:08:48, dshwang wrote: > On 2015/09/03 08:38:04, dshwang wrote: > > On 2015/09/02 22:19:39, mdempsky wrote: > > > This is the right place to add the ioctl checks, so LGTM for that. > > > > > > I briefly skimmed the kernel code for the three DRM_IOCTLs, and by > themselves > > > they didn't seem too scary. Relatively straight forward without any fancy > > > flags/knobs to worry about. I didn't fully grok what they do though. > > > > Thank you for surveying! > > > > jln, rsesek, could you review? > > > > DRM_IOCTL has unique code if chromeos linux kernel didn't load insane > thirdparty > > driver according to my investigation > > https://codereview.chromium.org/1253363004/#msg32 > > > > DRM_IOCTL_XX doesn't have issue more than TCGETS and FIONREAD, which we allow > > already. > > In addition, TCGETS and FIONREAD is based on unique ('T'), which conflict with > > sound; > > > https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... > > > > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > > File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): > > > > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > > content/common/sandbox_linux/bpf_renderer_policy_linux.cc:35: // The type of > > DRM_IOCTL_XXX macro is long unsigned int. > > On 2015/09/02 22:19:38, mdempsky wrote: > > > For simplicity, I would suggest writing this as just: > > > > > > const Arg<unsigned long> request(1); > > > return Switch(request) > > > .CASES((static_cast<unsigned long>(TCGETS), FIONREAD), Allow()) > > > #if defined(USE_VGEM_MAP) > > > .CASES((DRM_IOCTL_GEM_CLOSE, ...), Allow()) > > > #endif > > > .Default(sandbox::CrashSIGSYSIoctl()); > > > > > > There's no need to vary the Arg type parameter based on USE_VGEM_MAP. > > > > > > (The static_cast is annoying, but unavoidable until we're allowed to use > > > <initializer_list>. :-/) > > > > Thx for taking your time. Done. > > jln, rsesek, could you review? friendly ping please? :)
On 2015/10/02 19:59:11, vignatti wrote: > On 2015/09/29 11:08:48, dshwang wrote: > > On 2015/09/03 08:38:04, dshwang wrote: > > > On 2015/09/02 22:19:39, mdempsky wrote: > > > > This is the right place to add the ioctl checks, so LGTM for that. > > > > > > > > I briefly skimmed the kernel code for the three DRM_IOCTLs, and by > > themselves > > > > they didn't seem too scary. Relatively straight forward without any fancy > > > > flags/knobs to worry about. I didn't fully grok what they do though. > > > > > > Thank you for surveying! > > > > > > jln, rsesek, could you review? > > > > > > DRM_IOCTL has unique code if chromeos linux kernel didn't load insane > > thirdparty > > > driver according to my investigation > > > https://codereview.chromium.org/1253363004/#msg32 > > > > > > DRM_IOCTL_XX doesn't have issue more than TCGETS and FIONREAD, which we > allow > > > already. > > > In addition, TCGETS and FIONREAD is based on unique ('T'), which conflict > with > > > sound; > > > > > > https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe... > > > > > > > > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > > > File content/common/sandbox_linux/bpf_renderer_policy_linux.cc (right): > > > > > > > > > https://codereview.chromium.org/1253363004/diff/160001/content/common/sandbox... > > > content/common/sandbox_linux/bpf_renderer_policy_linux.cc:35: // The type of > > > DRM_IOCTL_XXX macro is long unsigned int. > > > On 2015/09/02 22:19:38, mdempsky wrote: > > > > For simplicity, I would suggest writing this as just: > > > > > > > > const Arg<unsigned long> request(1); > > > > return Switch(request) > > > > .CASES((static_cast<unsigned long>(TCGETS), FIONREAD), Allow()) > > > > #if defined(USE_VGEM_MAP) > > > > .CASES((DRM_IOCTL_GEM_CLOSE, ...), Allow()) > > > > #endif > > > > .Default(sandbox::CrashSIGSYSIoctl()); > > > > > > > > There's no need to vary the Arg type parameter based on USE_VGEM_MAP. > > > > > > > > (The static_cast is annoying, but unavoidable until we're allowed to use > > > > <initializer_list>. :-/) > > > > > > Thx for taking your time. Done. > > > > jln, rsesek, could you review? > > friendly ping please? :) Sorry, missed that and I can't look right now. Matthew you reviewed, didn't you? If yes, then lgtm from me. (Matthew: you should be in these owner files).
jln, thx for reviewing! Matthew, could you review? Let me summarize what is the issue. jln concerned DRM_IOCTL_GEM_CLOSE, DRM_IOCTL_VGEM_MODE_MAP_DUMB, DRM_IOCTL_PRIME_FD_TO_HANDLE ioctl code could collide to other driver. We investigated if there is collision. We concludes the 3 code is unique and there is not collision. Finally, jln think it's lgtm. On 2015/08/21 17:30:17, jln (OOO til Oct. 18th) wrote: > Do we have any influence in how these kernel interfaces are designed? This > design is very sandbox-unfriendly: we can't filter ioctls per fd type. So > allowing, say DRM_IOCTL_GEM_CLOSE will allow this same ioctl on, say, terminal > file descriptors. Because of collisions in ioctl values between drivers we don't > really know what this could mean. On 2015/09/02 22:19:39, mdempsky wrote: > This is the right place to add the ioctl checks, so LGTM for that. > > I briefly skimmed the kernel code for the three DRM_IOCTLs, and by themselves > they didn't seem too scary. Relatively straight forward without any fancy > flags/knobs to worry about. I didn't fully grok what they do though. On 2015/10/03 02:27:26, jln (OOO til Oct. 18th) wrote: > Sorry, missed that and I can't look right now. Matthew you reviewed, didn't you? > If yes, then lgtm from me. > > (Matthew: you should be in these owner files). DRM_IOCTL has unique code if chromeos linux kernel didn't load insane thirdparty driver according to my investigation https://codereview.chromium.org/1253363004/#msg32 DRM_IOCTL_XX doesn't have issue more than TCGETS and FIONREAD, which we allow already. In addition, TCGETS and FIONREAD is based on unique ('T'), which conflict with sound; https://github.com/torvalds/linux/blob/master/Documentation/ioctl/ioctl-numbe...
spang@chromium.org changed reviewers: + jbudorick@chromium.org
+jbudorick for build/
On 2015/10/05 08:20:44, dshwang_ooo_16.Oct wrote: > Matthew, could you review? LGTM
jbudorick@chromium.org changed reviewers: + thakis@chromium.org
this is out of my area of expertise jbudorick -> thakis for build/
why is the gyp project in build/ but the corresponding gn thing somewhere in ui/? shouldn't they be in the same folder?
On 2015/10/13 18:06:22, Nico wrote: > why is the gyp project in build/ but the corresponding gn thing somewhere in > ui/? shouldn't they be in the same folder? SG. dshwang: Can you move it to ozone.gyp please.
I'm back from vacation. Thanks for re-reviewing sandbox! On 2015/10/13 18:12:01, spang wrote: > On 2015/10/13 18:06:22, Nico wrote: > > why is the gyp project in build/ but the corresponding gn thing somewhere in > > ui/? shouldn't they be in the same folder? > > SG. dshwang: Can you move it to ozone.gyp please. Done. thakis@, could you review build/? spang@, could you review ui/ozone/? https://codereview.chromium.org/1253363004/diff/240001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1253363004/diff/240001/build/common.gypi#newc... build/common.gypi:611: 'use_vgem_map%': 0, it's used in both ui/ozone/ and content/
On 2015/10/19 14:48:43, dshwang wrote: > I'm back from vacation. Thanks for re-reviewing sandbox! > > On 2015/10/13 18:12:01, spang wrote: > > On 2015/10/13 18:06:22, Nico wrote: > > > why is the gyp project in build/ but the corresponding gn thing somewhere in > > > ui/? shouldn't they be in the same folder? > > > > SG. dshwang: Can you move it to ozone.gyp please. I totally removed the redundant changes in build/ now gyp and gn have same logic. As build/ is not touched, imo thakis@ doesn't need to review. spang@, could you review ui/ozone/?
lgtm
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org, mdempsky@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1253363004/#ps280001 (title: "fix chromeos_daisy_chromium_compile_only_ng build failure")
On 2015/10/19 15:29:49, spang wrote: > lgtm thank you for reviewing!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1253363004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1253363004/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2a785aa5f32b793fb51898f82efef096abab6102 Cr-Commit-Position: refs/heads/master@{#354794} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
