[RFC 3/3] drm: Update file owner during use
Christian König
christian.koenig at amd.com
Fri Jan 6 14:53:13 UTC 2023
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!" :)
So basically this is a valid use case where logind set/get the master
status of a fd while the compositor uses the master functionality?
Christian.
> -Daniel
>
>> Christian.
>>
>>> -Daniel
>>>
>>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>> -Daniel
>>>>>>
>>>>>>
>>>>>>> return 0;
>>>>>>> if (!capable(CAP_SYS_ADMIN))
>>>>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c
>>>>>>> b/drivers/gpu/drm/drm_debugfs.c
>>>>>>> index 42f657772025..9d4e3146a2b8 100644
>>>>>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>>>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>>>>>> @@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file
>>>>>>> *m, void *data)
>>>>>>> */
>>>>>>> mutex_lock(&dev->filelist_mutex);
>>>>>>> list_for_each_entry_reverse(priv, &dev->filelist, lhead) {
>>>>>>> - struct task_struct *task;
>>>>>>> bool is_current_master = drm_is_current_master(priv);
>>>>>>> + struct task_struct *task;
>>>>>>> + struct pid *pid;
>>>>>>> - rcu_read_lock(); /* locks pid_task()->comm */
>>>>>>> - task = pid_task(priv->pid, PIDTYPE_TGID);
>>>>>>> + rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
>>>>>>> + pid = rcu_dereference(priv->pid);
>>>>>>> + task = pid_task(pid, PIDTYPE_TGID);
>>>>>>> uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
>>>>>>> seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n",
>>>>>>> task ? task->comm : "<unknown>",
>>>>>>> - pid_vnr(priv->pid),
>>>>>>> + pid_vnr(pid),
>>>>>>> priv->minor->index,
>>>>>>> is_current_master ? 'y' : 'n',
>>>>>>> priv->authenticated ? 'y' : 'n',
>>>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>>>> index 20a9aef2b398..3433f9610dba 100644
>>>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>>>> @@ -156,7 +156,7 @@ struct drm_file *drm_file_alloc(struct
>>>>>>> drm_minor *minor)
>>>>>>> if (!file)
>>>>>>> return ERR_PTR(-ENOMEM);
>>>>>>> - file->pid = get_pid(task_tgid(current));
>>>>>>> + rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>>>>>>> file->minor = minor;
>>>>>>> /* for compatibility root is always authenticated */
>>>>>>> @@ -502,6 +502,36 @@ int drm_release(struct inode *inode, struct
>>>>>>> file *filp)
>>>>>>> }
>>>>>>> EXPORT_SYMBOL(drm_release);
>>>>>>> +void drm_file_update_pid(struct drm_file *filp)
>>>>>>> +{
>>>>>>> + struct drm_device *dev;
>>>>>>> + struct pid *pid, *old;
>>>>>>> +
>>>>>>> + /* Master nodes are not expected to be passed between
>>>>>>> processes. */
>>>>>>> + if (filp->was_master)
>>>>>>> + return;
>>>>>>> +
>>>>>>> + pid = task_tgid(current);
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * Quick unlocked check since the model is a single
>>>>>>> handover followed by
>>>>>>> + * exclusive repeated use.
>>>>>>> + */
>>>>>>> + if (pid == rcu_access_pointer(filp->pid))
>>>>>>> + return;
>>>>>>> +
>>>>>>> + dev = filp->minor->dev;
>>>>>>> + mutex_lock(&dev->filelist_mutex);
>>>>>>> + old = rcu_replace_pointer(filp->pid, pid, 1);
>>>>>>> + mutex_unlock(&dev->filelist_mutex);
>>>>>>> +
>>>>>>> + if (pid != old) {
>>>>>>> + get_pid(pid);
>>>>>>> + synchronize_rcu();
>>>>>>> + put_pid(old);
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>> /**
>>>>>>> * drm_release_noglobal - release method for DRM file
>>>>>>> * @inode: device inode
>>>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>>>>>> index 7c9d66ee917d..305b18d9d7b6 100644
>>>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>>>>>> @@ -775,6 +775,9 @@ long drm_ioctl_kernel(struct file *file,
>>>>>>> drm_ioctl_t *func, void *kdata,
>>>>>>> struct drm_device *dev = file_priv->minor->dev;
>>>>>>> int retcode;
>>>>>>> + /* Update drm_file owner if fd was passed along. */
>>>>>>> + drm_file_update_pid(file_priv);
>>>>>>> +
>>>>>>> if (drm_dev_is_unplugged(dev))
>>>>>>> return -ENODEV;
>>>>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
>>>>>>> b/drivers/gpu/drm/nouveau/nouveau_drm.c
>>>>>>> index 80f154b6adab..a763d3ee61fb 100644
>>>>>>> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
>>>>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
>>>>>>> @@ -1097,7 +1097,10 @@ nouveau_drm_open(struct drm_device *dev,
>>>>>>> struct drm_file *fpriv)
>>>>>>> }
>>>>>>> get_task_comm(tmpname, current);
>>>>>>> - snprintf(name, sizeof(name), "%s[%d]", tmpname,
>>>>>>> pid_nr(fpriv->pid));
>>>>>>> + rcu_read_lock();
>>>>>>> + snprintf(name, sizeof(name), "%s[%d]",
>>>>>>> + tmpname, pid_nr(rcu_dereference(fpriv->pid)));
>>>>>>> + rcu_read_unlock();
>>>>>>> if (!(cli = kzalloc(sizeof(*cli), GFP_KERNEL))) {
>>>>>>> ret = -ENOMEM;
>>>>>>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
>>>>>>> b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
>>>>>>> index f2985337aa53..3853d9bb9ab8 100644
>>>>>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
>>>>>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
>>>>>>> @@ -251,6 +251,7 @@ static int vmw_debugfs_gem_info_show(struct
>>>>>>> seq_file *m, void *unused)
>>>>>>> list_for_each_entry(file, &dev->filelist, lhead) {
>>>>>>> struct task_struct *task;
>>>>>>> struct drm_gem_object *gobj;
>>>>>>> + struct pid *pid;
>>>>>>> int id;
>>>>>>> /*
>>>>>>> @@ -260,8 +261,9 @@ static int vmw_debugfs_gem_info_show(struct
>>>>>>> seq_file *m, void *unused)
>>>>>>> * Therefore, we need to protect this ->comm access
>>>>>>> using RCU.
>>>>>>> */
>>>>>>> rcu_read_lock();
>>>>>>> - task = pid_task(file->pid, PIDTYPE_TGID);
>>>>>>> - seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid),
>>>>>>> + pid = rcu_dereference(file->pid);
>>>>>>> + task = pid_task(pid, PIDTYPE_TGID);
>>>>>>> + seq_printf(m, "pid %8d command %s:\n", pid_nr(pid),
>>>>>>> task ? task->comm : "<unknown>");
>>>>>>> rcu_read_unlock();
>>>>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>>>>> index 0d1f853092ab..27d545131d4a 100644
>>>>>>> --- a/include/drm/drm_file.h
>>>>>>> +++ b/include/drm/drm_file.h
>>>>>>> @@ -255,8 +255,15 @@ struct drm_file {
>>>>>>> /** @master_lookup_lock: Serializes @master. */
>>>>>>> spinlock_t master_lookup_lock;
>>>>>>> - /** @pid: Process that opened this file. */
>>>>>>> - struct pid *pid;
>>>>>>> + /**
>>>>>>> + * @pid: Process that is using this file.
>>>>>>> + *
>>>>>>> + * Must only be dereferenced under a rcu_read_lock or equivalent.
>>>>>>> + *
>>>>>>> + * Updates are guarded with dev->filelist_mutex and
>>>>>>> reference must be
>>>>>>> + * dropped after a RCU grace period to accommodate lockless
>>>>>>> readers.
>>>>>>> + */
>>>>>>> + struct pid __rcu *pid;
>>>>>>> /** @magic: Authentication magic, see @authenticated. */
>>>>>>> drm_magic_t magic;
>>>>>>> @@ -415,6 +422,8 @@ static inline bool drm_is_accel_client(const
>>>>>>> struct drm_file *file_priv)
>>>>>>> return file_priv->minor->type == DRM_MINOR_ACCEL;
>>>>>>> }
>>>>>>> +void drm_file_update_pid(struct drm_file *);
>>>>>>> +
>>>>>>> int drm_open(struct inode *inode, struct file *filp);
>>>>>>> int drm_open_helper(struct file *filp, struct drm_minor *minor);
>>>>>>> ssize_t drm_read(struct file *filp, char __user *buffer,
>>>>>>> --
>>>>>>> 2.34.1
>>>>>>>
More information about the dri-devel
mailing list