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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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?

But in terms of where my series is going next I would need some clarification in the other sub-thread.

Regards,

Tvrtko

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?

Yup, and the compositor is _not_ allowed to call these. Despite that it's
the exact sime struct file - it has to be the same struct file in both
loging and compositor, otherwise logind cannot orchestratet the vt switch
dance for the compositors. Which unlike non-logind vt switching has the
nice property that if a compositor dies/goes rogue, logind can still force
the switch. With vt-only switching you need the sysrq to reset the console
to text and kill the foreground process for the same effect.
-Daniel


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






[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux