Re: [PATCH v2] drm: Update file owner during use

Christian König <christian.koenig@xxxxxxx> · Wed, 20 Sep 2023 15:22:25 +0200

Am 20.09.23 um 15:21 schrieb Tvrtko Ursulin:

On 28/08/2023 20:58, Rob Clark wrote:
On Wed, Jun 21, 2023 at 2:48 AM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:

From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

With the typical model where the display server opens the file 
descriptor
and then hands it over to the client(*), we were showing stale data in
debugfs.

Fix it by updating the drm_file->pid on ioctl access from a different
process.

The field is also made RCU protected to allow for lockless readers. 
Update
side is protected with dev->filelist_mutex.

Before:

$ cat /sys/kernel/debug/dri/0/clients
              command   pid dev master a   uid      magic
                 Xorg  2344   0   y    y     0          0
                 Xorg  2344   0   n    y     0          2
                 Xorg  2344   0   n    y     0          3
                 Xorg  2344   0   n    y     0          4

After:

$ cat /sys/kernel/debug/dri/0/clients
              command  tgid dev master a   uid      magic
                 Xorg   830   0   y    y     0          0
        xfce4-session   880   0   n    y     0          1
                xfwm4   943   0   n    y     0          2
            neverball  1095   0   n    y     0          3

*)
More detailed and historically accurate description of various handover
implementation kindly provided by Emil Velikov:

"""
The traditional model, the server was the orchestrator managing the
primary device node. From the fd, to the master status and
authentication. But looking at the fd alone, this has varied across
the years.

IIRC in the DRI1 days, Xorg (libdrm really) would have a list of open
fd(s) and reuse those whenever needed, DRI2 the client was responsible
for open() themselves and with DRI3 the fd was passed to the client.

Around the inception of DRI3 and systemd-logind, the latter became
another possible orchestrator. Whereby Xorg and Wayland compositors
could ask it for the fd. For various reasons (hysterical and genuine
ones) Xorg has a fallback path going the open(), whereas Wayland
compositors are moving to solely relying on logind... some never had
fallback even.

Over the past few years, more projects have emerged which provide
functionality similar (be that on API level, Dbus, or otherwise) to
systemd-logind.
"""

v2:
  * Fixed typo in commit text and added a fine historical explanation
    from Emil.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Acked-by: Christian König <christian.koenig@xxxxxxx>
Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>

Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
Tested-by: Rob Clark <robdclark@xxxxxxxxx>

Thanks. If everyone else is happy with this approach I don't have the 
commit rights for drm-misc.

Going to take care of pushing this.

Regards,
Christian.


Regards,

Tvrtko


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  6 ++--
  drivers/gpu/drm/drm_auth.c              |  3 +-
  drivers/gpu/drm/drm_debugfs.c           | 10 ++++---
  drivers/gpu/drm/drm_file.c              | 40 
+++++++++++++++++++++++--
  drivers/gpu/drm/drm_ioctl.c             |  3 ++
  drivers/gpu/drm/nouveau/nouveau_drm.c   |  5 +++-
  drivers/gpu/drm/vmwgfx/vmwgfx_gem.c     |  6 ++--
  include/drm/drm_file.h                  | 13 ++++++--
  8 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 74055cba3dc9..849097dff02b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -963,6 +963,7 @@ static int amdgpu_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;

                 /*
@@ -972,8 +973,9 @@ static int amdgpu_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/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index cf92a9ae8034..2ed2585ded37 100644
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -235,7 +235,8 @@ static int drm_new_set_master(struct drm_device 
*dev, struct drm_file *fpriv)
  static int
  drm_master_check_perm(struct drm_device *dev, struct drm_file 
*file_priv)
  {
-       if (file_priv->pid == task_pid(current) && 
file_priv->was_master)
+       if (file_priv->was_master &&
+           rcu_access_pointer(file_priv->pid) == task_pid(current))
                 return 0;

         if (!capable(CAP_SYS_ADMIN))
diff --git a/drivers/gpu/drm/drm_debugfs.c 
b/drivers/gpu/drm/drm_debugfs.c
index 4855230ba2c6..b46f5ceb24c6 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 883d83bc0e3d..e692770ef6d3 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -160,7 +160,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
*minor)

         /* Get a unique identifier for fdinfo: */
         file->client_id = atomic64_inc_return(&ident);
-       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 */
@@ -200,7 +200,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
*minor)
                 drm_syncobj_release(file);
         if (drm_core_check_feature(dev, DRIVER_GEM))
                 drm_gem_release(dev, file);
-       put_pid(file->pid);
+       put_pid(rcu_access_pointer(file->pid));
         kfree(file);

         return ERR_PTR(ret);
@@ -291,7 +291,7 @@ void drm_file_free(struct drm_file *file)

         WARN_ON(!list_empty(&file->event_list));

-       put_pid(file->pid);
+       put_pid(rcu_access_pointer(file->pid));
         kfree(file);
  }

@@ -505,6 +505,40 @@ 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 need to keep the original ownership in order 
for
+        * drm_master_check_perm to keep working correctly. (See 
comment in
+        * drm_auth.c.)
+        */
+       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 51f1918b44d3..e3cb60eb0bc8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1101,7 +1101,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 c0da89e16e6f..a07e5b7e2f2f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -232,6 +232,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;

                 /*
@@ -241,8 +242,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 966912053cb0..c76249d5467e 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -256,8 +256,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;

         /** @client_id: A unique id for fdinfo */
         u64 client_id;
@@ -420,6 +427,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.39.2








