| Message ID | 1550262252-15558-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ION per heap devices | expand |
Hi John, On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote: > This is a *very early RFC* (it builds, that's all I'll say :) > but I wanted to share it to get some initial feedback before I > go down the rabit hole of trying to adapt the Android userland > code to get this fully validated. > > This patchset tries to implement the per-heap devices for ION. > The main benefit is that it avoids multiplexing heap operations > through the /dev/ion interface, and allows for each heap to have > its own permissions/sepolicy rules. In general, I've always thought that having a device node per-heap is a bit messy for userspace. Multiplexing through the single node doesn't seem like an insurmountable problem, but the point about permissions/sepolicy is reasonably compelling if it's a real requirement. What would be the reasons for that? Thanks, -Brian > > Feedback would be greatly appreciated! > thanks > -john > > Cc: Laura Abbott <labbott@redhat.com> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Andrew F. Davis <afd@ti.com> > Cc: Alistair Strachan <astrachan@google.com> > Cc: dri-devel@lists.freedesktop.org > > John Stultz (4): > ion: Add ION_VERSION ioctl > ion: Initial hack to create per heap devices > ion: Add HEAP_INFO ioctl to be able to fetch heap type > ion: Make "legacy" /dev/ion support optional > > drivers/staging/android/ion/Kconfig | 7 +++ > drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++ > drivers/staging/android/ion/ion.c | 51 ++++++++++++++++----- > drivers/staging/android/ion/ion.h | 6 +++ > drivers/staging/android/uapi/ion.h | 57 +++++++++++++++++++++++ > 5 files changed, 191 insertions(+), 10 deletions(-) > > -- > 2.7.4 >
On Mon, Feb 18, 2019 at 3:51 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote: > > This is a *very early RFC* (it builds, that's all I'll say :) > > but I wanted to share it to get some initial feedback before I > > go down the rabit hole of trying to adapt the Android userland > > code to get this fully validated. > > > > This patchset tries to implement the per-heap devices for ION. > > The main benefit is that it avoids multiplexing heap operations > > through the /dev/ion interface, and allows for each heap to have > > its own permissions/sepolicy rules. > > In general, I've always thought that having a device node per-heap is > a bit messy for userspace. Multiplexing through the single node > doesn't seem like an insurmountable problem, but the point about Hrm. I guess for me having a custom enumeration interface over ioctl seems less ideal compared to a directory list. > permissions/sepolicy is reasonably compelling if it's a real > requirement. What would be the reasons for that? Its a bit second hand for me, so hopefully I don't have it wrong. I've cc'ed some additional google folks and Benjamin for extra context, but my sense of it is that you may have some less-trusted code that we're fine with allocating system heap dma-bufs, but don't want to to be giving access to more limited heaps like carveout or cma, or more potentially security troubling heaps like system-contig. thanks -john
On 2/19/19 9:21 AM, John Stultz wrote: > On Mon, Feb 18, 2019 at 3:51 AM Brian Starkey <Brian.Starkey@arm.com> wrote: >> On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote: >>> This is a *very early RFC* (it builds, that's all I'll say :) >>> but I wanted to share it to get some initial feedback before I >>> go down the rabit hole of trying to adapt the Android userland >>> code to get this fully validated. >>> >>> This patchset tries to implement the per-heap devices for ION. >>> The main benefit is that it avoids multiplexing heap operations >>> through the /dev/ion interface, and allows for each heap to have >>> its own permissions/sepolicy rules. >> >> In general, I've always thought that having a device node per-heap is >> a bit messy for userspace. Multiplexing through the single node >> doesn't seem like an insurmountable problem, but the point about > > Hrm. I guess for me having a custom enumeration interface over ioctl > seems less ideal compared to a directory list. > >> permissions/sepolicy is reasonably compelling if it's a real >> requirement. What would be the reasons for that? > > Its a bit second hand for me, so hopefully I don't have it wrong. I've > cc'ed some additional google folks and Benjamin for extra context, but > my sense of it is that you may have some less-trusted code that we're > fine with allocating system heap dma-bufs, but don't want to to be > giving access to more limited heaps like carveout or cma, or more > potentially security troubling heaps like system-contig. > > thanks > -john > Yes, the discussion was always based on being able to set separate security policy for each heap. It's not clear to me how strong a requirement is these days or if there's other options to enforce the same thing. Thanks, Laura
On 2/15/19 12:24 PM, John Stultz wrote: > This is a *very early RFC* (it builds, that's all I'll say :) > but I wanted to share it to get some initial feedback before I > go down the rabit hole of trying to adapt the Android userland > code to get this fully validated. > > This patchset tries to implement the per-heap devices for ION. > The main benefit is that it avoids multiplexing heap operations > through the /dev/ion interface, and allows for each heap to have > its own permissions/sepolicy rules. > > Feedback would be greatly appreciated! > thanks > -john > > Cc: Laura Abbott <labbott@redhat.com> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Liam Mark <lmark@codeaurora.org> > Cc: Brian Starkey <Brian.Starkey@arm.com> > Cc: Andrew F. Davis <afd@ti.com> > Cc: Alistair Strachan <astrachan@google.com> > Cc: dri-devel@lists.freedesktop.org > > John Stultz (4): > ion: Add ION_VERSION ioctl > ion: Initial hack to create per heap devices > ion: Add HEAP_INFO ioctl to be able to fetch heap type > ion: Make "legacy" /dev/ion support optional > > drivers/staging/android/ion/Kconfig | 7 +++ > drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++ > drivers/staging/android/ion/ion.c | 51 ++++++++++++++++----- > drivers/staging/android/ion/ion.h | 6 +++ > drivers/staging/android/uapi/ion.h | 57 +++++++++++++++++++++++ > 5 files changed, 191 insertions(+), 10 deletions(-) > So it occurs to me if this is going to be a new ABI all together maybe we should just declare a new allocation ioctl to be used with it. We can keep the old ioctls around for legacy use cases and maybe eventually delete them and just use the new allocation ioctl with the new split heaps. Thanks, Laura
On 2/19/19 3:25 PM, Laura Abbott wrote: > On 2/15/19 12:24 PM, John Stultz wrote: >> This is a *very early RFC* (it builds, that's all I'll say :) >> but I wanted to share it to get some initial feedback before I >> go down the rabit hole of trying to adapt the Android userland >> code to get this fully validated. >> >> This patchset tries to implement the per-heap devices for ION. >> The main benefit is that it avoids multiplexing heap operations >> through the /dev/ion interface, and allows for each heap to have >> its own permissions/sepolicy rules. >> >> Feedback would be greatly appreciated! >> thanks >> -john >> >> Cc: Laura Abbott <labbott@redhat.com> >> Cc: Sumit Semwal <sumit.semwal@linaro.org> >> Cc: Liam Mark <lmark@codeaurora.org> >> Cc: Brian Starkey <Brian.Starkey@arm.com> >> Cc: Andrew F. Davis <afd@ti.com> >> Cc: Alistair Strachan <astrachan@google.com> >> Cc: dri-devel@lists.freedesktop.org >> >> John Stultz (4): >> ion: Add ION_VERSION ioctl >> ion: Initial hack to create per heap devices >> ion: Add HEAP_INFO ioctl to be able to fetch heap type >> ion: Make "legacy" /dev/ion support optional >> >> drivers/staging/android/ion/Kconfig | 7 +++ >> drivers/staging/android/ion/ion-ioctl.c | 80 >> +++++++++++++++++++++++++++++++++ >> drivers/staging/android/ion/ion.c | 51 ++++++++++++++++----- >> drivers/staging/android/ion/ion.h | 6 +++ >> drivers/staging/android/uapi/ion.h | 57 +++++++++++++++++++++++ >> 5 files changed, 191 insertions(+), 10 deletions(-) >> > > So it occurs to me if this is going to be a new ABI > all together maybe we should just declare a new allocation ioctl > to be used with it. We can keep the old ioctls around > for legacy use cases and maybe eventually delete them > and just use the new allocation ioctl with the new > split heaps. > Why keep the old ones, this is staging, there are no legacy users (that matter to kernel).. Slowing progress for the sake of backwards compat with staging just slows the de-staging down. Andrew > Thanks, > Laura
On 2/19/19 1:30 PM, Andrew F. Davis wrote: > On 2/19/19 3:25 PM, Laura Abbott wrote: >> On 2/15/19 12:24 PM, John Stultz wrote: >>> This is a *very early RFC* (it builds, that's all I'll say :) >>> but I wanted to share it to get some initial feedback before I >>> go down the rabit hole of trying to adapt the Android userland >>> code to get this fully validated. >>> >>> This patchset tries to implement the per-heap devices for ION. >>> The main benefit is that it avoids multiplexing heap operations >>> through the /dev/ion interface, and allows for each heap to have >>> its own permissions/sepolicy rules. >>> >>> Feedback would be greatly appreciated! >>> thanks >>> -john >>> >>> Cc: Laura Abbott <labbott@redhat.com> >>> Cc: Sumit Semwal <sumit.semwal@linaro.org> >>> Cc: Liam Mark <lmark@codeaurora.org> >>> Cc: Brian Starkey <Brian.Starkey@arm.com> >>> Cc: Andrew F. Davis <afd@ti.com> >>> Cc: Alistair Strachan <astrachan@google.com> >>> Cc: dri-devel@lists.freedesktop.org >>> >>> John Stultz (4): >>> ion: Add ION_VERSION ioctl >>> ion: Initial hack to create per heap devices >>> ion: Add HEAP_INFO ioctl to be able to fetch heap type >>> ion: Make "legacy" /dev/ion support optional >>> >>> drivers/staging/android/ion/Kconfig | 7 +++ >>> drivers/staging/android/ion/ion-ioctl.c | 80 >>> +++++++++++++++++++++++++++++++++ >>> drivers/staging/android/ion/ion.c | 51 ++++++++++++++++----- >>> drivers/staging/android/ion/ion.h | 6 +++ >>> drivers/staging/android/uapi/ion.h | 57 +++++++++++++++++++++++ >>> 5 files changed, 191 insertions(+), 10 deletions(-) >>> >> >> So it occurs to me if this is going to be a new ABI >> all together maybe we should just declare a new allocation ioctl >> to be used with it. We can keep the old ioctls around >> for legacy use cases and maybe eventually delete them >> and just use the new allocation ioctl with the new >> split heaps. >> > > Why keep the old ones, this is staging, there are no legacy users (that > matter to kernel).. Slowing progress for the sake of backwards compat > with staging just slows the de-staging down. > I think we just fundamentally disagree here. I don't see keeping legacy users as slowing anything down. We're still getting the new ABI that we actually like and we get the chance to easily go back and test. Having a non broken ABI makes it much easier to do testing and validation and comparison. We can remove the last ABI before we move it out of staging. Thanks, Laura > Andrew > >> Thanks, >> Laura
On Tue, Feb 19, 2019 at 1:25 PM Laura Abbott <labbott@redhat.com> wrote: > > On 2/15/19 12:24 PM, John Stultz wrote: > > This is a *very early RFC* (it builds, that's all I'll say :) > > but I wanted to share it to get some initial feedback before I > > go down the rabit hole of trying to adapt the Android userland > > code to get this fully validated. > > > > This patchset tries to implement the per-heap devices for ION. > > The main benefit is that it avoids multiplexing heap operations > > through the /dev/ion interface, and allows for each heap to have > > its own permissions/sepolicy rules. > > > > Feedback would be greatly appreciated! > > thanks > > -john > > > > Cc: Laura Abbott <labbott@redhat.com> > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > Cc: Liam Mark <lmark@codeaurora.org> > > Cc: Brian Starkey <Brian.Starkey@arm.com> > > Cc: Andrew F. Davis <afd@ti.com> > > Cc: Alistair Strachan <astrachan@google.com> > > Cc: dri-devel@lists.freedesktop.org > > > > John Stultz (4): > > ion: Add ION_VERSION ioctl > > ion: Initial hack to create per heap devices > > ion: Add HEAP_INFO ioctl to be able to fetch heap type > > ion: Make "legacy" /dev/ion support optional > > > > drivers/staging/android/ion/Kconfig | 7 +++ > > drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++ > > drivers/staging/android/ion/ion.c | 51 ++++++++++++++++----- > > drivers/staging/android/ion/ion.h | 6 +++ > > drivers/staging/android/uapi/ion.h | 57 +++++++++++++++++++++++ > > 5 files changed, 191 insertions(+), 10 deletions(-) > > > > So it occurs to me if this is going to be a new ABI > all together maybe we should just declare a new allocation ioctl > to be used with it. We can keep the old ioctls around > for legacy use cases and maybe eventually delete them > and just use the new allocation ioctl with the new > split heaps. So... I did add ION_IOC_HEAP_ALLOC in my patchset. Or are you suggesting we use a new ION_IOC_MAGIC value for the new ABI? Or some larger rework? thanks -john