mbox series

[EARLY,RFC,0/4] ION per heap devices

Message ID 1550262252-15558-1-git-send-email-john.stultz@linaro.org (mailing list archive)
Headers show
Series ION per heap devices | expand

Message

kernel test robot via dri-devel Feb. 15, 2019, 8:24 p.m. UTC
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(-)

Comments

Brian Starkey Feb. 18, 2019, 11:51 a.m. UTC | #1
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
>
John Stultz Feb. 19, 2019, 5:21 p.m. UTC | #2
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
Laura Abbott Feb. 19, 2019, 8:51 p.m. UTC | #3
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
Laura Abbott Feb. 19, 2019, 9:25 p.m. UTC | #4
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
Andrew Davis Feb. 19, 2019, 9:30 p.m. UTC | #5
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
Laura Abbott Feb. 19, 2019, 9:54 p.m. UTC | #6
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
John Stultz Feb. 19, 2019, 9:55 p.m. UTC | #7
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