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

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

 



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>

> ---
>  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
>




[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