Re: [RFC 3/3] drm: Update file owner during use

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 11/01/2023 22:19, Daniel Vetter wrote:
> On Tue, Jan 10, 2023 at 01:14:51PM +0000, Tvrtko Ursulin wrote:
>>
>> On 06/01/2023 18:00, Daniel Vetter wrote:
>>> On Fri, Jan 06, 2023 at 03:53:13PM +0100, Christian König wrote:
>>>> Am 06.01.23 um 11:53 schrieb Daniel Vetter:
>>>>> On Fri, Jan 06, 2023 at 11:32:17AM +0100, Christian König wrote:
>>>>>> Am 05.01.23 um 13:32 schrieb Daniel Vetter:
>>>>>>> [SNIP]
>>>>>>>> For the case of an master fd I actually don't see the reason why we should
>>>>>>>> limit that? And fd can become master if it either was master before or has
>>>>>>>> CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here?
>>>>>>> This is just info/debug printing, I don't see the connection to
>>>>>>> drm_auth/master stuff? Aside from the patch mixes up the master opener and
>>>>>>> the current user due to fd passing or something like that.
>>>>>> That's exactly why my comment meant as well.
>>>>>>
>>>>>> The connect is that the drm_auth/master code currently the pid/tgid as
>>>>>> indicator if the "owner" of the fd has changed and so if an access should be
>>>>>> allowed or not. I find that approach a bit questionable.
>>>>>>
>>>>>>> Note that we cannot do that (I think at least, after pondering this some
>>>>>>> more) because it would break the logind master fd passing scheme - there
>>>>>>> the receiving compositor is explicitly _not_ allowed to acquire master
>>>>>>> rights on its own. So the master priviledges must not move with the fd or
>>>>>>> things can go wrong.
>>>>>> That could be the rational behind that, but why doesn't logind then just
>>>>>> pass on a normal render node to the compositor?
>>>>> Because the compositor wants the kms node. We have 3 access levels in drm
>>>>>
>>>>> - render stuff
>>>>> - modeset stuff (needs a card* node and master rights for changing things)
>>>>> - set/drop master (needs root)
>>>>>
>>>>> logind wants to give the compositor modeset access, but not master
>>>>> drop/set access (because vt switching is controlled by logind).
>>>>>
>>>>> The pid check in drm_auth is for the use-case where you start your
>>>>> compositor on a root vt (or setuid-root), and then want to make sure
>>>>> that after cred dropping, set/drop master keeps working. Because in that
>>>>> case the vt switch dance is done by the compositor.
>>>>>
>>>>> Maybe we should document this stuff a bit better :-)
>>>>
>>>> Maybe add a friendly warning? E.g. like "Don't touch it, it works!" :)
>>>
>>> I think Tvrtko just volunteered for that :-) Maybe addition in the
>>> drm-uapi.rst section would be good that fills out the gaps we have.
>>
>> I can attempt to copy, paste and tidy what you wrote here, albeit with less
>> than full degree of authority. Assuming into the existing comment above
>> drm_master_check_perm?
> 
> I'd put it into the DOC: section in drm_auth.c so it shows up in the
> drm-uapi.rst docs. Or do a new one if you want to split this out and then
> include it in the drm-uapi.rst.
> 
>> But in terms of where my series is going next I would need some
>> clarification in the other sub-thread.
> 
> Maybe I'm lost on what the leftover confusion is? This one seemed to be
> it?

The question of whether you are now okay with my approach to track 
file_priv->pid if !was_master, or your counter-proposal to have 
file_priv->pid and file_priv->"render_user_pid" is still relevant.

If the latter then what semantics have been settled at, one-shot 
transition or not?

I had an issue with one-shot and even didn't fully understand what you 
did not like about my proposal.

Regards,

Tvrtko