[PATCH 1/2] drm: Make control nodes master-less

[Thomas Hellstrom]

On 03/12/2014 10:45 AM, David Herrmann wrote:
> Hi
>
>> You didn't miss any patches. It was I who missed the usage in drm_crtc.c.
>> I guess this, and Daniel's reply prompts a discussion about control
>> nodes and the master concept.
>>
>> First I'd like to give some background about the use-case: I'd like to
>> use the control node for a daemon that tells the vmwgfx driver about the
>> host GUI layout of the screen: What connectors are enabled, the
>> preferred mode of each output and some driver private information. The
>> daemon will run as root and only a single instance per device. Trying to
>> do this as-is will cause the vmwgfx driver to BUG() because it currently
>> assumes one active master per device. Not one active master per minor
>> (although that could be changed if needed).
> Why do you tie this to DRM-Master? Can't you just limit these special
> ioctls to the control-node and drop any master-requirements? This way
> you don't care whether the FD on the control-node is master or not,
> you just assume that access-rights on the control-node are highly
> restricted so anyone opening it is privileged to issue these ioctls.


That's exactly what I'm planning to do.
The BUG() in vmwgfx that happens when a device gets two masters (one
from legacy and one from control) has been there
for some time and is related to other functionality [1].

>> Looking at
>>
>> https://urldefense.proofpoint.com/v1/url?u=http://dvdhrm.wordpress.com/2013/09/01/splitting-drm-and-kms-device-nodes/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=V8cVVz4Hn%2FjsqGcRA9UoPCYNUTXvS%2BA21CW7xsg5Mwc%3D%0A&s=3b7c73b3db46fe8d9fdc5aa7e1f83939e84be61977db211c24b40ef81d911e28
>>
>> it doesn't really make sense to keep the master concept on the control
>> node, but it perhaps makes sense to keep it on future modesetting nodes
>> to allow multiple modesetters to open the same device node?
>>
>> In that case all master access in drm_crtc.c would be changed for now to
>> be conditioned on file_priv->minor->type == DRM_MINOR_LEGACY
> The Master-concept only really makes sense for operations affecting
> global state. On non-legacy drivers this is basically KMS. Therefore,
> I see no reason to extend the master-concept to non-modeset/primary
> nodes. So ACK on changing drm_crtc.c to check for master only if
> minor==DRM_MINOR_PRIMARY/LEGACY (there are pending patches renaming
> it..).

Great.



> The problem with the modeset-node concept described in my blog-post is
> KMS-object hotplugging. We don't support it.. and I don't see any
> proper way to extend our API in a suitable way. The only thing I still
> have planned is to allow creating multiple DRM_MINOR_PRIMARY/LEGACY
> nodes and splitting mode-objects among them. We would have to do that
> before anyone uses the device to prevent objects from disappearing,
> but this could easily be done as udev-action. Thus, setting up
> multiple KMS interfaces on a single card.

OK.

>
> Anyhow, imho we should just try to remove any master-handling from any
> core DRM code. Ideally, only drm_drv.c deals with DRM-Master when
> checking for permissions. Everything else should never attach any
> information to these things. Especially the master-related driver
> callbacks are undocumented and really weird. If we'd be able to drop
> all this, I think we can start looking forward.

[1]
I'm partly guilty of that. There is an, IMHO quite nasty, security
problem with the current master concept:
Imagine you have a master (say X server) with a number of authenticated
clients.
Then the master drops master privileges and another X server becomes master.
Now the old master's authenticated clients may still access the new X
server's (legacy) shared buffers, since drm doesn't
differentiate which master a client is authenticated with.

This is, as far as I can tell, even violating X server security policy.
 
While render nodes will make future solutions work around this, they
don't plug this security problem, and since nobody else
really seems to care, we use the driver hooks to suspend authenticated
clients when their master drops master privileges, just like DRI1 used
to do - hence the TTM lock tied to a master.

So IMHO once there is a core drm solution in place for this, we should
probably be ready to drop the driver master_set and master_drop hooks.

/Thomas


>
> Thanks
> David