[RFC] flink_to

[RFC] flink_to

[Kristian Høgsberg]

[ Let's try this again... ]

Ok, so the flink_to discussion rat-holed a bit on the binary blob
attachment issue.  But before we even get to that, there are a lot of
issues that I'd some feedback on as well.  There were a few bugs in
the patch that I've fixed, but I don't see the point in sending it out
again just yet, as I'd like to see if we can agree on some of the
higher level issues.

First of, I'm hoping we can get this ready for the next merge window.
The patch is written and I've tested it here with a libdrm test case
and am currently finishing the EGL support for this.  One change I
might want to do is to add a blob type argument to the ioctls so
userspace has a standardized way of indication what the format of the
data is (what Keith pointed out).  That's a fairly simple change and
the patch itself is simple enough to begin with, so I don't expect a
lot of tricky issues with the implementation.

Along with the flink_to ioctl, I'm proposing that we drop the DRM_AUTH
requirement for accessing the gem ioctls.  Specifically, for intel,
I'm suggesting that we drop the DRM_AUTH requirement on

  DRM_IOCTL_GEM_FLINK,
  DRM_IOCTL_GEM_OPEN,
  DRM_I915_GEM_EXECBUFFER,
  DRM_I915_GEM_EXECBUFFER2,
  DRM_I915_GEM_BUSY,
  DRM_I915_GEM_THROTTLE,
  DRM_I915_GEM_CREATE,
  DRM_I915_GEM_PREAD,
  DRM_I915_GEM_PWRITE,
  DRM_I915_GEM_MMAP,
  DRM_I915_GEM_MMAP_GTT,
  DRM_I915_GEM_SET_DOMAIN,
  DRM_I915_GEM_SW_FINISH,
  DRM_I915_GEM_SET_TILING,
  DRM_I915_GEM_GET_TILING,
  DRM_I915_GEM_GET_APERTURE,
  DRM_I915_GEM_MADVISE,

which should allow clients to create buffers, submit rendering against
them and share them with a priviledged (in the sense that it controls
scanout) display server.  Access to these ioctls will then only be
guarded by the permissions on /dev/dri/cardX, which all distros
restrict to the 'local' user (that is, excluding ssh and similar) in
one way or another.  How do we feel about that?  Maybe it's something
the master needs to request, so that only X servers that use flink_to
will activate this mode?

flink_to doesn't in itself solve the security problem, since user
space can still submit a batch buffer that reads or writes to an
absolute gtt offset (that is, no relocation).  The X front buffer
location is typically pretty predictable, for example.  flink_to does
give us the infrastructure to implement a secure system though.  There
are several ways this could be done: use a sw command checker to
reject absolute gtt offsets, unbind buffers from all other clients
before running executing the commands or use per-process gtt or
similar hw support.

Then there's the data passing mechanism part of flink_to.  I'm
suggesting that we allow applications to attach a blob to an flink_to
name, which will be passed to the process calling open_with_data on
the name.  The format of the blob is defined by userspace, typically
libdrm or mesa, and lets us marshal meta data about the buffer along
with granting access to the buffer.  And just to be clear, the kernel
has no need for this meta data, it doesn't even understand the format.
But it will make protocoles and user level APIs simpler, and it's not
going to be a resource drain in the kernel.  There's a 2k max size on
the attached data, and a buffer can only have one flink_to name
pending per file_priv.  I didn't see any strong objections in the
thread, but I understand the concerns.  We're debating a minimal
kernel API with a kludgey userspace vs kernel API with convenience
features and much simpler userspace.

Anyway, I'll post an update when I get a working userspace on top of
this proposal.  In the meantime I would much appreciate feedback on
the above issues.

Kristian

Re: [RFC] flink_to

[Dave Airlie]

On Mon, 2010-07-12 at 11:55 -0400, Kristian Høgsberg wrote:
> [ Let's try this again... ]
> 
> Ok, so the flink_to discussion rat-holed a bit on the binary blob
> attachment issue.  But before we even get to that, there are a lot of
> issues that I'd some feedback on as well.  There were a few bugs in
> the patch that I've fixed, but I don't see the point in sending it out
> again just yet, as I'd like to see if we can agree on some of the
> higher level issues.
> 
> First of, I'm hoping we can get this ready for the next merge window.
> The patch is written and I've tested it here with a libdrm test case
> and am currently finishing the EGL support for this.  One change I
> might want to do is to add a blob type argument to the ioctls so
> userspace has a standardized way of indication what the format of the
> data is (what Keith pointed out).  That's a fairly simple change and
> the patch itself is simple enough to begin with, so I don't expect a
> lot of tricky issues with the implementation.
> 
> Along with the flink_to ioctl, I'm proposing that we drop the DRM_AUTH
> requirement for accessing the gem ioctls.  Specifically, for intel,
> I'm suggesting that we drop the DRM_AUTH requirement on
> 
>   DRM_IOCTL_GEM_FLINK,
>   DRM_IOCTL_GEM_OPEN,
>   DRM_I915_GEM_EXECBUFFER,
>   DRM_I915_GEM_EXECBUFFER2,
>   DRM_I915_GEM_BUSY,
>   DRM_I915_GEM_THROTTLE,
>   DRM_I915_GEM_CREATE,
>   DRM_I915_GEM_PREAD,
>   DRM_I915_GEM_PWRITE,
>   DRM_I915_GEM_MMAP,
>   DRM_I915_GEM_MMAP_GTT,
>   DRM_I915_GEM_SET_DOMAIN,
>   DRM_I915_GEM_SW_FINISH,
>   DRM_I915_GEM_SET_TILING,
>   DRM_I915_GEM_GET_TILING,
>   DRM_I915_GEM_GET_APERTURE,
>   DRM_I915_GEM_MADVISE,
> 
> which should allow clients to create buffers, submit rendering against
> them and share them with a priviledged (in the sense that it controls
> scanout) display server.  Access to these ioctls will then only be
> guarded by the permissions on /dev/dri/cardX, which all distros
> restrict to the 'local' user (that is, excluding ssh and similar) in
> one way or another.  How do we feel about that?  Maybe it's something
> the master needs to request, so that only X servers that use flink_to
> will activate this mode?

I'd rather we only did this if we knew everyone was going to use
flink_to, and then make sure normal flink doesn't work at all once
someone started using flink_to perhaps.

But otherwise it all sounds good.

> 
> flink_to doesn't in itself solve the security problem, since user
> space can still submit a batch buffer that reads or writes to an
> absolute gtt offset (that is, no relocation).  The X front buffer
> location is typically pretty predictable, for example.  flink_to does
> give us the infrastructure to implement a secure system though.  There
> are several ways this could be done: use a sw command checker to
> reject absolute gtt offsets, unbind buffers from all other clients
> before running executing the commands or use per-process gtt or
> similar hw support.

Doesn't solve the security problem for *Intel*. On radeon for example
we've always provided this type of security, GEM's interface is the only
hole in that case (apart from the sw checker maybe missing some cases).
So I'm quite happy that this is what we'd prefer.

> Then there's the data passing mechanism part of flink_to.  I'm
> suggesting that we allow applications to attach a blob to an flink_to
> name, which will be passed to the process calling open_with_data on
> the name.  The format of the blob is defined by userspace, typically
> libdrm or mesa, and lets us marshal meta data about the buffer along
> with granting access to the buffer.  And just to be clear, the kernel
> has no need for this meta data, it doesn't even understand the format.
> But it will make protocoles and user level APIs simpler, and it's not
> going to be a resource drain in the kernel.  There's a 2k max size on
> the attached data, and a buffer can only have one flink_to name
> pending per file_priv.  I didn't see any strong objections in the
> thread, but I understand the concerns.  We're debating a minimal
> kernel API with a kludgey userspace vs kernel API with convenience
> features and much simpler userspace.

Its really ugly, and its really going to end up as ABI, except the
people hacking on the X server will forget that the people hacking on
mesa need to have the same struct or some such as they will think they
are giving the data to the kernel. I really get the feeling this would
work better in userspace, or with at least a format that works in the
kernel. Is the data going to be per-GPU? per-driver? per-what? Who is
expecting to interpret it in userspace? what happens if in a few months
you realise you need 4k. (2k is pointless, since it'll eat a page
anyways). Yes its meta-data to the kernel, but flink_to is a generic
userspace interface and attaching a bunch of non-generic data to it
sounds hackish.

Dave.


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] flink_to

[Kristian Høgsberg]

2010/7/12 Dave Airlie <airlied@redhat.com>:
> On Mon, 2010-07-12 at 11:55 -0400, Kristian Høgsberg wrote:
>> [ Let's try this again... ]
>>
>> Ok, so the flink_to discussion rat-holed a bit on the binary blob
>> attachment issue.  But before we even get to that, there are a lot of
>> issues that I'd some feedback on as well.  There were a few bugs in
>> the patch that I've fixed, but I don't see the point in sending it out
>> again just yet, as I'd like to see if we can agree on some of the
>> higher level issues.
>>
>> First of, I'm hoping we can get this ready for the next merge window.
>> The patch is written and I've tested it here with a libdrm test case
>> and am currently finishing the EGL support for this.  One change I
>> might want to do is to add a blob type argument to the ioctls so
>> userspace has a standardized way of indication what the format of the
>> data is (what Keith pointed out).  That's a fairly simple change and
>> the patch itself is simple enough to begin with, so I don't expect a
>> lot of tricky issues with the implementation.
>>
>> Along with the flink_to ioctl, I'm proposing that we drop the DRM_AUTH
>> requirement for accessing the gem ioctls.  Specifically, for intel,
>> I'm suggesting that we drop the DRM_AUTH requirement on
>>
>>   DRM_IOCTL_GEM_FLINK,
>>   DRM_IOCTL_GEM_OPEN,
>>   DRM_I915_GEM_EXECBUFFER,
>>   DRM_I915_GEM_EXECBUFFER2,
>>   DRM_I915_GEM_BUSY,
>>   DRM_I915_GEM_THROTTLE,
>>   DRM_I915_GEM_CREATE,
>>   DRM_I915_GEM_PREAD,
>>   DRM_I915_GEM_PWRITE,
>>   DRM_I915_GEM_MMAP,
>>   DRM_I915_GEM_MMAP_GTT,
>>   DRM_I915_GEM_SET_DOMAIN,
>>   DRM_I915_GEM_SW_FINISH,
>>   DRM_I915_GEM_SET_TILING,
>>   DRM_I915_GEM_GET_TILING,
>>   DRM_I915_GEM_GET_APERTURE,
>>   DRM_I915_GEM_MADVISE,
>>
>> which should allow clients to create buffers, submit rendering against
>> them and share them with a priviledged (in the sense that it controls
>> scanout) display server.  Access to these ioctls will then only be
>> guarded by the permissions on /dev/dri/cardX, which all distros
>> restrict to the 'local' user (that is, excluding ssh and similar) in
>> one way or another.  How do we feel about that?  Maybe it's something
>> the master needs to request, so that only X servers that use flink_to
>> will activate this mode?
>
> I'd rather we only did this if we knew everyone was going to use
> flink_to, and then make sure normal flink doesn't work at all once
> someone started using flink_to perhaps.

It's only the master (and its clients) that have anything to lose from
dropping auth, so if the master can say "I'm using flink_to, go ahead"
shouldn't that be sufficient?  Additionally we could just make flink a
master only ioctl. In our current stack, only the X server is using
flink, so it wont make a difference.

Besides, isn't the only concern new gpgpu type clients using flink and
making their buffers available to every body?  And isn't that a case
of "dont do that"?  What other clients might use flink, and who's at
risk if they do?

> But otherwise it all sounds good.
>
>>
>> flink_to doesn't in itself solve the security problem, since user
>> space can still submit a batch buffer that reads or writes to an
>> absolute gtt offset (that is, no relocation).  The X front buffer
>> location is typically pretty predictable, for example.  flink_to does
>> give us the infrastructure to implement a secure system though.  There
>> are several ways this could be done: use a sw command checker to
>> reject absolute gtt offsets, unbind buffers from all other clients
>> before running executing the commands or use per-process gtt or
>> similar hw support.
>
> Doesn't solve the security problem for *Intel*. On radeon for example
> we've always provided this type of security, GEM's interface is the only
> hole in that case (apart from the sw checker maybe missing some cases).
> So I'm quite happy that this is what we'd prefer.

Oh, I know, I'm just saying that flink_to alone doesn't solve the
problem, there has to be something else enforcing the buffer access.

>> Then there's the data passing mechanism part of flink_to.  I'm
>> suggesting that we allow applications to attach a blob to an flink_to
>> name, which will be passed to the process calling open_with_data on
>> the name.  The format of the blob is defined by userspace, typically
>> libdrm or mesa, and lets us marshal meta data about the buffer along
>> with granting access to the buffer.  And just to be clear, the kernel
>> has no need for this meta data, it doesn't even understand the format.
>> But it will make protocoles and user level APIs simpler, and it's not
>> going to be a resource drain in the kernel.  There's a 2k max size on
>> the attached data, and a buffer can only have one flink_to name
>> pending per file_priv.  I didn't see any strong objections in the
>> thread, but I understand the concerns.  We're debating a minimal
>> kernel API with a kludgey userspace vs kernel API with convenience
>> features and much simpler userspace.
>
> Its really ugly, and its really going to end up as ABI, except the
> people hacking on the X server will forget that the people hacking on
> mesa need to have the same struct or some such as they will think they
> are giving the data to the kernel.

It will definitely be ABI, I'm not suggesting otherwise, just not
kernel ABI.  The blob will have a 32 bit type code that allows
userspace to check if it's the format it expects. I imagine the lower
16 bits could be a minor number to indicate backwards compatible
extension of the data and the upper 16 bits indicate incompatible,
different types of data.  It will be defined in libdrm as a struct
that only libdrm knows how to encode and decode (like how we
encapsulate ioctl structs).

> I really get the feeling this would
> work better in userspace, or with at least a format that works in the
> kernel. Is the data going to be per-GPU? per-driver? per-what? Who is
> expecting to interpret it in userspace? what happens if in a few months
> you realise you need 4k. (2k is pointless, since it'll eat a page
> anyways).

It will be a chipset specific struct.  That's a key part of the idea,
since then we don't have to generalize the contents or define generic
descriptions of format and tiling details etc.  Making the struct
chipset specific means we can describe everything the driver needs to
communicate about the buffer, in the native tokens used by the
hardware.  For intel, that means we can define the buffer format as
BRW_SURFACEFORMAT_B8G8R8A8_UNORM instead of trying to define DRM level
codes for buffer formats etc.

As for the size limit, I made it PAGE_SIZE - sizeof (struct
drm_gem_name) originally, but went with 2048 in case we needed to grow
the struct size kernel side.  And if you need more than 2048 bytes of
metadata, I think it's safe to say that you should use a different
mechanism.

If we did this in userspace, we would have all the same issues, except
we would have to pass a blob of data around in protocol and APIs.  The
ABI and version, encoding and decoding would still be managed in
libdrm, but with flink_to the kernel helps pass the data around.

> Yes its meta-data to the kernel, but flink_to is a generic
> userspace interface and attaching a bunch of non-generic data to it
> sounds hackish.

But when you think about how we use this generic interface
(flink+open) today, it's always only from chipset specific code.  It
can't ever be generic anyway, flink needs to mark the bo shared in the
bufmgr and take it out of caching lists, open needs to set up the
drm_$chipset_bo struct and do chipset specific ioctls anyway to get
tiling and other metadata.

My first take on this idea was indeed to make flink_to chipset
specific and use a good old struct defined in i915_drm.h for passing
the data in and out of the kernel.  This has the advantage that we
define the format in the kernel header and all clients have to agree
on the format whether or not they use libdrm.  However that seems a
little academic (since all clients use libdrm), and the blob approach
lets us define the format of the data used only by userspace in
userspace and keep the ioctl generic.  But we can go back to that if
that's a more acceptable approach.

Kristian
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] flink_to

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2119 bytes --]

On Tue, 13 Jul 2010 08:44:22 +1000, Dave Airlie <airlied@redhat.com> wrote:
> On Mon, 2010-07-12 at 11:55 -0400, Kristian Høgsberg wrote:
> > [ Let's try this again... ]

...

> > flink_to doesn't in itself solve the security problem, since user
> > space can still submit a batch buffer that reads or writes to an
> > absolute gtt offset (that is, no relocation).  The X front buffer
> > location is typically pretty predictable, for example.  flink_to does
> > give us the infrastructure to implement a secure system though.  There
> > are several ways this could be done: use a sw command checker to
> > reject absolute gtt offsets, unbind buffers from all other clients
> > before running executing the commands or use per-process gtt or
> > similar hw support.
> 
> Doesn't solve the security problem for *Intel*. On radeon for example
> we've always provided this type of security, GEM's interface is the only
> hole in that case (apart from the sw checker maybe missing some cases).
> So I'm quite happy that this is what we'd prefer.

I know Radeon's been struggling a lot with the CPU overhead of their
command submission, and given that we're CPU bound for most performance
stuff I look at right now, I'm not thrilled by the idea of adding
command checking.  Particularly the part where we have to analyze the
shader kernels.  I'm assuming we would disallow stateless access mode
writes/reads entirely, because that lets you access arbitrary graphics
memory from your instructions, and to bounds-check that you'd have to do
it in shader execution and that probably means emitting an IR to the
kernel instead of actual instructions.

Given that the security story today on Intel is "anybody that's authed
once can access all the buffers", adding flink_to and letting people
basically get authed even when not VT switched away from the X Server
seems fine to me.  If we want to provide these security guarantees
later, then we should just unbind, or use PPGTT if available.  The
performance wins should be sizeable with going to PPGTT, so we need to
get that done anyway.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] flink_to

[Kristian Høgsberg]

Ok, so the flink_to discussion rat-holed a bit on the binary blob
attachment issue.  But before we even get to that, there are a lot of
issues that I'd some feedback on as well.  There were a few bugs in
the patch that I've fixed, but I don't see the point in sending it out
again just yet, as I'd like to see if we can agree on some of the
higher level issues.

First of, I'm hoping we can  get this ready for the next merge window.
 The patch is written and I've tested it here with a libdrm test case
and am currently finishing the EGL support for this.  One change I
might want to do is to add a blob type argument to the ioctls so
userspace has a standardized way of indication what the format of the
data is.  That's a fairly simple change and the patch itself is simple
enough to begin with, so I don't expect a lot of tricky issues with
the implementation.

Along with the flink_to ioctl, I'm proposing that we drop the DRM_AUTH
requirement for accessing the gem ioctls.  Specifically, for intel,
I'm suggesting that we drop the DRM_AUTH requirement on

DRM_IOCTL_GEM_FLINK, DRM_IOCTL_GEM_OPEN,
DRM_I915_GEM_EXECBUFFDRM_I915_GEM_CREATE, ER,
DRM_I915_GEM_EXECBUFFER2,