[RFC 0/3] File owner follows use

[RFC 0/3] File owner follows use

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Not so long ago when I sent out my DRM cgroup controller RFC I had some pieces
in it which were tracking the real client using a specific drm_file. Christian
then suggested that should probably be extracted and improved in the DRM core
from the start, which was on his wishlist for a long period. So this mini-series
is an attempt at that.

First patch is just a logging cleanup, 2nd probably makes sense on it's own
since it replaces tracking thread names with progresses which are more
meaningful. Third one is where action is.

The benefit on it's own is rather small, especially relative to the complication
to track it, where it essentially changes the debugfs clients output from:

             command   pid dev master a   uid      magic
                Xorg  1744   0   y    y     0          0
                Xorg  1744   0   n    y     0          1
                Xorg  1744   0   n    y     0          2
                Xorg  1744   0   n    y     0          3

To something like:

             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

One ugly part is one synchronise_rcu() on the first (hopefully) only fd
handover. The latency of that could be improved by further wrapping and
kfree_rcu() if desired.

Another part I am unsure of is whether master nodes are ever handed over via
sockets. I assumed no and exluded them from ownership updates. If they need to
be then drm_master_check_perm() would break, I think. So looking for some
feedback in this area please.

Tvrtko Ursulin (3):
  drm: Replace DRM_DEBUG with drm_dbg_core in file and ioctl handling
  drm: Track clients by tgid and not tid
  drm: Update file owner during use

 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c |  6 ++-
 drivers/gpu/drm/drm_auth.c              |  3 +-
 drivers/gpu/drm/drm_debugfs.c           | 12 +++---
 drivers/gpu/drm/drm_file.c              | 53 ++++++++++++++++++++-----
 drivers/gpu/drm/drm_ioc32.c             | 13 +++---
 drivers/gpu/drm/drm_ioctl.c             | 28 +++++++------
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  5 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_gem.c     |  6 ++-
 include/drm/drm_file.h                  | 13 +++++-
 9 files changed, 97 insertions(+), 42 deletions(-)

-- 
2.34.1

[RFC 1/3] drm: Replace DRM_DEBUG with drm_dbg_core in file and ioctl handling

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Replace the deprecated macro with the per-device one.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/drm_file.c  | 21 +++++++++++----------
 drivers/gpu/drm/drm_ioc32.c | 13 +++++++------
 drivers/gpu/drm/drm_ioctl.c | 25 +++++++++++++------------
 3 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 64b4a3a87fbb..b0f24cea1e1e 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -245,10 +245,10 @@ void drm_file_free(struct drm_file *file)
 
 	dev = file->minor->dev;
 
-	DRM_DEBUG("comm=\"%s\", pid=%d, dev=0x%lx, open_count=%d\n",
-		  current->comm, task_pid_nr(current),
-		  (long)old_encode_dev(file->minor->kdev->devt),
-		  atomic_read(&dev->open_count));
+	drm_dbg_core(dev, "comm=\"%s\", pid=%d, dev=0x%lx, open_count=%d\n",
+		     current->comm, task_pid_nr(current),
+		     (long)old_encode_dev(file->minor->kdev->devt),
+		     atomic_read(&dev->open_count));
 
 #ifdef CONFIG_DRM_LEGACY
 	if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
@@ -340,8 +340,8 @@ int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	    dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
 		return -EINVAL;
 
-	DRM_DEBUG("comm=\"%s\", pid=%d, minor=%d\n", current->comm,
-		  task_pid_nr(current), minor->index);
+	drm_dbg_core(dev, "comm=\"%s\", pid=%d, minor=%d\n",
+		     current->comm, task_pid_nr(current), minor->index);
 
 	priv = drm_file_alloc(minor);
 	if (IS_ERR(priv))
@@ -450,11 +450,12 @@ EXPORT_SYMBOL(drm_open);
 
 void drm_lastclose(struct drm_device * dev)
 {
-	DRM_DEBUG("\n");
+	drm_dbg_core(dev, "\n");
 
-	if (dev->driver->lastclose)
+	if (dev->driver->lastclose) {
 		dev->driver->lastclose(dev);
-	DRM_DEBUG("driver lastclose completed\n");
+		drm_dbg_core(dev, "driver lastclose completed\n");
+	}
 
 	if (drm_core_check_feature(dev, DRIVER_LEGACY))
 		drm_legacy_dev_reinit(dev);
@@ -485,7 +486,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	if (drm_dev_needs_global_mutex(dev))
 		mutex_lock(&drm_global_mutex);
 
-	DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
+	drm_dbg_core(dev, "open_count = %d\n", atomic_read(&dev->open_count));
 
 	drm_close_helper(filp);
 
diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
index 5d82891c3222..49a743f62b4a 100644
--- a/drivers/gpu/drm/drm_ioc32.c
+++ b/drivers/gpu/drm/drm_ioc32.c
@@ -972,6 +972,7 @@ long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	unsigned int nr = DRM_IOCTL_NR(cmd);
 	struct drm_file *file_priv = filp->private_data;
+	struct drm_device *dev = file_priv->minor->dev;
 	drm_ioctl_compat_t *fn;
 	int ret;
 
@@ -986,14 +987,14 @@ long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (!fn)
 		return drm_ioctl(filp, cmd, arg);
 
-	DRM_DEBUG("comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, %s\n",
-		  current->comm, task_pid_nr(current),
-		  (long)old_encode_dev(file_priv->minor->kdev->devt),
-		  file_priv->authenticated,
-		  drm_compat_ioctls[nr].name);
+	drm_dbg_core(dev, "comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, %s\n",
+		     current->comm, task_pid_nr(current),
+		     (long)old_encode_dev(file_priv->minor->kdev->devt),
+		     file_priv->authenticated,
+		     drm_compat_ioctls[nr].name);
 	ret = (*fn)(filp, cmd, arg);
 	if (ret)
-		DRM_DEBUG("ret = %d\n", ret);
+		drm_dbg_core(dev, "ret = %d\n", ret);
 	return ret;
 }
 EXPORT_SYMBOL(drm_compat_ioctl);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index ca2a6e6101dc..7c9d66ee917d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -440,7 +440,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
 int drm_noop(struct drm_device *dev, void *data,
 	     struct drm_file *file_priv)
 {
-	DRM_DEBUG("\n");
+	drm_dbg_core(dev, "\n");
 	return 0;
 }
 EXPORT_SYMBOL(drm_noop);
@@ -856,16 +856,16 @@ long drm_ioctl(struct file *filp,
 		out_size = 0;
 	ksize = max(max(in_size, out_size), drv_size);
 
-	DRM_DEBUG("comm=\"%s\" pid=%d, dev=0x%lx, auth=%d, %s\n",
-		  current->comm, task_pid_nr(current),
-		  (long)old_encode_dev(file_priv->minor->kdev->devt),
-		  file_priv->authenticated, ioctl->name);
+	drm_dbg_core(dev, "comm=\"%s\" pid=%d, dev=0x%lx, auth=%d, %s\n",
+		     current->comm, task_pid_nr(current),
+		     (long)old_encode_dev(file_priv->minor->kdev->devt),
+		     file_priv->authenticated, ioctl->name);
 
 	/* Do not trust userspace, use our own definition */
 	func = ioctl->func;
 
 	if (unlikely(!func)) {
-		DRM_DEBUG("no function\n");
+		drm_dbg_core(dev, "no function\n");
 		retcode = -EINVAL;
 		goto err_i1;
 	}
@@ -894,16 +894,17 @@ long drm_ioctl(struct file *filp,
 
       err_i1:
 	if (!ioctl)
-		DRM_DEBUG("invalid ioctl: comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n",
-			  current->comm, task_pid_nr(current),
-			  (long)old_encode_dev(file_priv->minor->kdev->devt),
-			  file_priv->authenticated, cmd, nr);
+		drm_dbg_core(dev,
+			     "invalid ioctl: comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n",
+			     current->comm, task_pid_nr(current),
+			     (long)old_encode_dev(file_priv->minor->kdev->devt),
+			     file_priv->authenticated, cmd, nr);
 
 	if (kdata != stack_kdata)
 		kfree(kdata);
 	if (retcode)
-		DRM_DEBUG("comm=\"%s\", pid=%d, ret=%d\n", current->comm,
-			  task_pid_nr(current), retcode);
+		drm_dbg_core(dev, "comm=\"%s\", pid=%d, ret=%d\n",
+			     current->comm, task_pid_nr(current), retcode);
 	return retcode;
 }
 EXPORT_SYMBOL(drm_ioctl);
-- 
2.34.1

Re: [RFC 1/3] drm: Replace DRM_DEBUG with drm_dbg_core in file and ioctl handling

[Christian König]

Am 30.11.22 um 14:34 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Replace the deprecated macro with the per-device one.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_file.c  | 21 +++++++++++----------
>   drivers/gpu/drm/drm_ioc32.c | 13 +++++++------
>   drivers/gpu/drm/drm_ioctl.c | 25 +++++++++++++------------
>   3 files changed, 31 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 64b4a3a87fbb..b0f24cea1e1e 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -245,10 +245,10 @@ void drm_file_free(struct drm_file *file)
>   
>   	dev = file->minor->dev;
>   
> -	DRM_DEBUG("comm=\"%s\", pid=%d, dev=0x%lx, open_count=%d\n",
> -		  current->comm, task_pid_nr(current),
> -		  (long)old_encode_dev(file->minor->kdev->devt),
> -		  atomic_read(&dev->open_count));
> +	drm_dbg_core(dev, "comm=\"%s\", pid=%d, dev=0x%lx, open_count=%d\n",
> +		     current->comm, task_pid_nr(current),
> +		     (long)old_encode_dev(file->minor->kdev->devt),
> +		     atomic_read(&dev->open_count));
>   
>   #ifdef CONFIG_DRM_LEGACY
>   	if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> @@ -340,8 +340,8 @@ int drm_open_helper(struct file *filp, struct drm_minor *minor)
>   	    dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
>   		return -EINVAL;
>   
> -	DRM_DEBUG("comm=\"%s\", pid=%d, minor=%d\n", current->comm,
> -		  task_pid_nr(current), minor->index);
> +	drm_dbg_core(dev, "comm=\"%s\", pid=%d, minor=%d\n",
> +		     current->comm, task_pid_nr(current), minor->index);
>   
>   	priv = drm_file_alloc(minor);
>   	if (IS_ERR(priv))
> @@ -450,11 +450,12 @@ EXPORT_SYMBOL(drm_open);
>   
>   void drm_lastclose(struct drm_device * dev)
>   {
> -	DRM_DEBUG("\n");
> +	drm_dbg_core(dev, "\n");
>   
> -	if (dev->driver->lastclose)
> +	if (dev->driver->lastclose) {
>   		dev->driver->lastclose(dev);
> -	DRM_DEBUG("driver lastclose completed\n");
> +		drm_dbg_core(dev, "driver lastclose completed\n");
> +	}
>   
>   	if (drm_core_check_feature(dev, DRIVER_LEGACY))
>   		drm_legacy_dev_reinit(dev);
> @@ -485,7 +486,7 @@ int drm_release(struct inode *inode, struct file *filp)
>   	if (drm_dev_needs_global_mutex(dev))
>   		mutex_lock(&drm_global_mutex);
>   
> -	DRM_DEBUG("open_count = %d\n", atomic_read(&dev->open_count));
> +	drm_dbg_core(dev, "open_count = %d\n", atomic_read(&dev->open_count));
>   
>   	drm_close_helper(filp);
>   
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index 5d82891c3222..49a743f62b4a 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -972,6 +972,7 @@ long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   {
>   	unsigned int nr = DRM_IOCTL_NR(cmd);
>   	struct drm_file *file_priv = filp->private_data;
> +	struct drm_device *dev = file_priv->minor->dev;
>   	drm_ioctl_compat_t *fn;
>   	int ret;
>   
> @@ -986,14 +987,14 @@ long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	if (!fn)
>   		return drm_ioctl(filp, cmd, arg);
>   
> -	DRM_DEBUG("comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, %s\n",
> -		  current->comm, task_pid_nr(current),
> -		  (long)old_encode_dev(file_priv->minor->kdev->devt),
> -		  file_priv->authenticated,
> -		  drm_compat_ioctls[nr].name);
> +	drm_dbg_core(dev, "comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, %s\n",
> +		     current->comm, task_pid_nr(current),
> +		     (long)old_encode_dev(file_priv->minor->kdev->devt),
> +		     file_priv->authenticated,
> +		     drm_compat_ioctls[nr].name);
>   	ret = (*fn)(filp, cmd, arg);
>   	if (ret)
> -		DRM_DEBUG("ret = %d\n", ret);
> +		drm_dbg_core(dev, "ret = %d\n", ret);
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_compat_ioctl);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index ca2a6e6101dc..7c9d66ee917d 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -440,7 +440,7 @@ static int drm_setversion(struct drm_device *dev, void *data, struct drm_file *f
>   int drm_noop(struct drm_device *dev, void *data,
>   	     struct drm_file *file_priv)
>   {
> -	DRM_DEBUG("\n");
> +	drm_dbg_core(dev, "\n");
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_noop);
> @@ -856,16 +856,16 @@ long drm_ioctl(struct file *filp,
>   		out_size = 0;
>   	ksize = max(max(in_size, out_size), drv_size);
>   
> -	DRM_DEBUG("comm=\"%s\" pid=%d, dev=0x%lx, auth=%d, %s\n",
> -		  current->comm, task_pid_nr(current),
> -		  (long)old_encode_dev(file_priv->minor->kdev->devt),
> -		  file_priv->authenticated, ioctl->name);
> +	drm_dbg_core(dev, "comm=\"%s\" pid=%d, dev=0x%lx, auth=%d, %s\n",
> +		     current->comm, task_pid_nr(current),
> +		     (long)old_encode_dev(file_priv->minor->kdev->devt),
> +		     file_priv->authenticated, ioctl->name);
>   
>   	/* Do not trust userspace, use our own definition */
>   	func = ioctl->func;
>   
>   	if (unlikely(!func)) {
> -		DRM_DEBUG("no function\n");
> +		drm_dbg_core(dev, "no function\n");
>   		retcode = -EINVAL;
>   		goto err_i1;
>   	}
> @@ -894,16 +894,17 @@ long drm_ioctl(struct file *filp,
>   
>         err_i1:
>   	if (!ioctl)
> -		DRM_DEBUG("invalid ioctl: comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n",
> -			  current->comm, task_pid_nr(current),
> -			  (long)old_encode_dev(file_priv->minor->kdev->devt),
> -			  file_priv->authenticated, cmd, nr);
> +		drm_dbg_core(dev,
> +			     "invalid ioctl: comm=\"%s\", pid=%d, dev=0x%lx, auth=%d, cmd=0x%02x, nr=0x%02x\n",
> +			     current->comm, task_pid_nr(current),
> +			     (long)old_encode_dev(file_priv->minor->kdev->devt),
> +			     file_priv->authenticated, cmd, nr);
>   
>   	if (kdata != stack_kdata)
>   		kfree(kdata);
>   	if (retcode)
> -		DRM_DEBUG("comm=\"%s\", pid=%d, ret=%d\n", current->comm,
> -			  task_pid_nr(current), retcode);
> +		drm_dbg_core(dev, "comm=\"%s\", pid=%d, ret=%d\n",
> +			     current->comm, task_pid_nr(current), retcode);
>   	return retcode;
>   }
>   EXPORT_SYMBOL(drm_ioctl);

[RFC 2/3] drm: Track clients by tgid and not tid

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thread group id (aka pid from userspace point of view) is a more
interesting thing to show as an owner of a DRM fd, so track and show that
instead of the thread id.

In the next patch we will make the owner updated post file descriptor
handover, which will also be tgid based to avoid ping-pong when multiple
threads access the fd.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Zack Rusin <zackr@vmware.com>
Cc: linux-graphics-maintainer@vmware.com
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Reviewed-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
 drivers/gpu/drm/drm_debugfs.c           | 4 ++--
 drivers/gpu/drm/drm_file.c              | 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_gem.c     | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index a0780a4e3e61..30e24da1f398 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -968,7 +968,7 @@ 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_PID);
+		task = pid_task(file->pid, PIDTYPE_TGID);
 		seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid),
 			   task ? task->comm : "<unknown>");
 		rcu_read_unlock();
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index ee445f4605ba..42f657772025 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -80,7 +80,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
 	seq_printf(m,
 		   "%20s %5s %3s master a %5s %10s\n",
 		   "command",
-		   "pid",
+		   "tgid",
 		   "dev",
 		   "uid",
 		   "magic");
@@ -94,7 +94,7 @@ static int drm_clients_info(struct seq_file *m, void *data)
 		bool is_current_master = drm_is_current_master(priv);
 
 		rcu_read_lock(); /* locks pid_task()->comm */
-		task = pid_task(priv->pid, PIDTYPE_PID);
+		task = pid_task(priv->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>",
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index b0f24cea1e1e..20a9aef2b398 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_pid(current));
+	file->pid = get_pid(task_tgid(current));
 	file->minor = minor;
 
 	/* for compatibility root is always authenticated */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
index ce609e7d758f..f2985337aa53 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -260,7 +260,7 @@ 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_PID);
+		task = pid_task(file->pid, PIDTYPE_TGID);
 		seq_printf(m, "pid %8d command %s:\n", pid_nr(file->pid),
 			   task ? task->comm : "<unknown>");
 		rcu_read_unlock();
-- 
2.34.1

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

With the typical model where the display server opends 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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: "Christian König" <christian.koenig@amd.com>
---
 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              | 32 ++++++++++++++++++++++++-
 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, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 30e24da1f398..385deb044058 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -959,6 +959,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;
 
 		/*
@@ -968,8 +969,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 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

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

[Daniel Vetter]

On Wed, Nov 30, 2022 at 01:34:07PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> With the typical model where the display server opends 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.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> ---
>  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              | 32 ++++++++++++++++++++++++-
>  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, 65 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 30e24da1f398..385deb044058 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -959,6 +959,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;
>  
>  		/*
> @@ -968,8 +969,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))

This scares me, and also makes me wonder whether we really want to
conflate the original owner with the rendering owner. And also, whether we
really want to keep updating that, because for some of the "bind an fd to
a pid" use-cases like svm we really do not want to ever again allow a
change.

So sligthly different idea:
- we have a separate render pid/drm_file owner frome the open() owner that
  we track in drm_auth.c
- that one is set the first time a driver specific ioctl is called (which
  for the "pass me the fd" dri3 mode should never be the compositor)
- we start out with nothing and only set it once, which further simplifies
  the model (still need the mutex for concurrent first ioctl ofc)

Eventually we could then use that to enforce static binding to a pid,
which is what we want for svm style models, i.e. if the pid changes, the
fd does an -EACCESS or similar.

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Tvrtko Ursulin]

On 30/11/2022 14:18, Daniel Vetter wrote:
> On Wed, Nov 30, 2022 at 01:34:07PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> With the typical model where the display server opends 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.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: "Christian König" <christian.koenig@amd.com>
>> ---
>>   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              | 32 ++++++++++++++++++++++++-
>>   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, 65 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 30e24da1f398..385deb044058 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -959,6 +959,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;
>>   
>>   		/*
>> @@ -968,8 +969,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))
> 
> This scares me, and also makes me wonder whether we really want to
> conflate the original owner with the rendering owner. And also, whether we
> really want to keep updating that, because for some of the "bind an fd to
> a pid" use-cases like svm we really do not want to ever again allow a
> change.
> 
> So sligthly different idea:
> - we have a separate render pid/drm_file owner frome the open() owner that
>    we track in drm_auth.c
> - that one is set the first time a driver specific ioctl is called (which
>    for the "pass me the fd" dri3 mode should never be the compositor)
> - we start out with nothing and only set it once, which further simplifies
>    the model (still need the mutex for concurrent first ioctl ofc)

Simpler solution sounds plausible and mostly works for me. Certainly is 
attractive to simplify things. And as the disclaimer I put in the cover 
letter - I wasn't really sure at all if passing a master fd is a thing 
or not. Happy to implement your version if that will be the decision.

The only downside I can think of right now with having two owners is if 
someone is "naughty" and actually uses the fd for rendering from two 
sides. That wouldn't conceptually work for what I am doing in the cgroup 
controller, where I need to attribute GPU usage to a process, which is a 
lookup from struct pid -> list of drm_files -> etc. So in the two owners 
scheme I would just need to ignore the "open owner" and rely that 
"render ownder" truly is the only one doing the rendering. Or maybe I'd 
need to add support for multiple owners as well.. would be a bit 
annoying probably.

Hm now that I think about more.. the one shot nature of this scheme 
would have another downside. One could just send the fd back to itself 
via a throway forked helper, which only does one ioctl before sending it 
back, and then the "render owner" is forever lost. The proposal as I had 
it would be immune to this problem at least.

> Eventually we could then use that to enforce static binding to a pid,
> which is what we want for svm style models, i.e. if the pid changes, the
> fd does an -EACCESS or similar.
> 
> Thoughts?

This use case I am not familiar with at all so can't comment. Only 
intuitively I would ask - why is it something that needs to be solved at 
the DRM level? Because essentially it sounds like there is a want to 
disallow sending fds via sockets.

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

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

[Christian König]

Am 01.12.22 um 12:09 schrieb Tvrtko Ursulin:
>
> On 30/11/2022 14:18, Daniel Vetter wrote:
>> On Wed, Nov 30, 2022 at 01:34:07PM +0000, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> With the typical model where the display server opends 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.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: "Christian König" <christian.koenig@amd.com>
>>> ---
>>>   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              | 32 
>>> ++++++++++++++++++++++++-
>>>   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, 65 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 30e24da1f398..385deb044058 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -959,6 +959,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;
>>>             /*
>>> @@ -968,8 +969,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))
>>
>> This scares me, and also makes me wonder whether we really want to
>> conflate the original owner with the rendering owner. And also, 
>> whether we
>> really want to keep updating that, because for some of the "bind an 
>> fd to
>> a pid" use-cases like svm we really do not want to ever again allow a
>> change.
>>
>> So sligthly different idea:
>> - we have a separate render pid/drm_file owner frome the open() owner 
>> that
>>    we track in drm_auth.c
>> - that one is set the first time a driver specific ioctl is called 
>> (which
>>    for the "pass me the fd" dri3 mode should never be the compositor)
>> - we start out with nothing and only set it once, which further 
>> simplifies
>>    the model (still need the mutex for concurrent first ioctl ofc)
>
> Simpler solution sounds plausible and mostly works for me. Certainly 
> is attractive to simplify things. And as the disclaimer I put in the 
> cover letter - I wasn't really sure at all if passing a master fd is a 
> thing or not. Happy to implement your version if that will be the 
> decision.
>
> The only downside I can think of right now with having two owners is 
> if someone is "naughty" and actually uses the fd for rendering from 
> two sides. That wouldn't conceptually work for what I am doing in the 
> cgroup controller, where I need to attribute GPU usage to a process, 
> which is a lookup from struct pid -> list of drm_files -> etc. So in 
> the two owners scheme I would just need to ignore the "open owner" and 
> rely that "render ownder" truly is the only one doing the rendering. 
> Or maybe I'd need to add support for multiple owners as well.. would 
> be a bit annoying probably.
>
> Hm now that I think about more.. the one shot nature of this scheme 
> would have another downside. One could just send the fd back to itself 
> via a throway forked helper, which only does one ioctl before sending 
> it back, and then the "render owner" is forever lost. The proposal as 
> I had it would be immune to this problem at least.
>
>> Eventually we could then use that to enforce static binding to a pid,
>> which is what we want for svm style models, i.e. if the pid changes, the
>> fd does an -EACCESS or similar.
>>
>> Thoughts?
>
> This use case I am not familiar with at all so can't comment. Only 
> intuitively I would ask - why is it something that needs to be solved 
> at the DRM level? Because essentially it sounds like there is a want 
> to disallow sending fds via sockets.

I think we should only disallow binding an fd to a different process 
when there is a reason to do so.

For SVM it's an rather obvious security problem when we allow accessing 
the address space of another process. But this should probably be 
handled in the SVM code, not here. E.g. we check during CS if the 
mm_struct is still the same. Which pid or tgid this fd has last or 
initially used it completely irrelevant to this.

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?

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

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

[Daniel Vetter]

On Fri, Dec 02, 2022 at 10:01:01AM +0100, Christian König wrote:
> Am 01.12.22 um 12:09 schrieb Tvrtko Ursulin:
> > 
> > On 30/11/2022 14:18, Daniel Vetter wrote:
> > > On Wed, Nov 30, 2022 at 01:34:07PM +0000, Tvrtko Ursulin wrote:
> > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > 
> > > > With the typical model where the display server opends 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.
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > Cc: "Christian König" <christian.koenig@amd.com>
> > > > ---
> > > >   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              | 32
> > > > ++++++++++++++++++++++++-
> > > >   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, 65 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > > > index 30e24da1f398..385deb044058 100644
> > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > > > @@ -959,6 +959,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;
> > > >             /*
> > > > @@ -968,8 +969,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))
> > > 
> > > This scares me, and also makes me wonder whether we really want to
> > > conflate the original owner with the rendering owner. And also,
> > > whether we
> > > really want to keep updating that, because for some of the "bind an
> > > fd to
> > > a pid" use-cases like svm we really do not want to ever again allow a
> > > change.
> > > 
> > > So sligthly different idea:
> > > - we have a separate render pid/drm_file owner frome the open()
> > > owner that
> > >    we track in drm_auth.c
> > > - that one is set the first time a driver specific ioctl is called
> > > (which
> > >    for the "pass me the fd" dri3 mode should never be the compositor)
> > > - we start out with nothing and only set it once, which further
> > > simplifies
> > >    the model (still need the mutex for concurrent first ioctl ofc)
> > 
> > Simpler solution sounds plausible and mostly works for me. Certainly is
> > attractive to simplify things. And as the disclaimer I put in the cover
> > letter - I wasn't really sure at all if passing a master fd is a thing
> > or not. Happy to implement your version if that will be the decision.
> > 
> > The only downside I can think of right now with having two owners is if
> > someone is "naughty" and actually uses the fd for rendering from two
> > sides. That wouldn't conceptually work for what I am doing in the cgroup
> > controller, where I need to attribute GPU usage to a process, which is a
> > lookup from struct pid -> list of drm_files -> etc. So in the two owners
> > scheme I would just need to ignore the "open owner" and rely that
> > "render ownder" truly is the only one doing the rendering. Or maybe I'd
> > need to add support for multiple owners as well.. would be a bit
> > annoying probably.
> > 
> > Hm now that I think about more.. the one shot nature of this scheme
> > would have another downside. One could just send the fd back to itself
> > via a throway forked helper, which only does one ioctl before sending it
> > back, and then the "render owner" is forever lost. The proposal as I had
> > it would be immune to this problem at least.
> > 
> > > Eventually we could then use that to enforce static binding to a pid,
> > > which is what we want for svm style models, i.e. if the pid changes, the
> > > fd does an -EACCESS or similar.
> > > 
> > > Thoughts?
> > 
> > This use case I am not familiar with at all so can't comment. Only
> > intuitively I would ask - why is it something that needs to be solved at
> > the DRM level? Because essentially it sounds like there is a want to
> > disallow sending fds via sockets.

Disallowing passing isn't the idea I had. It's to disallow passing to more
than one recipient, once that recipient has started using the fd for real.

> I think we should only disallow binding an fd to a different process when
> there is a reason to do so.
> 
> For SVM it's an rather obvious security problem when we allow accessing the
> address space of another process. But this should probably be handled in the
> SVM code, not here. E.g. we check during CS if the mm_struct is still the
> same. Which pid or tgid this fd has last or initially used it completely
> irrelevant to this.

Yeah my idea was to also then build the SVM check on top of this as
enforcement.

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

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Christian König]

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?

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

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

[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 :-)
-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
> > > > > > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Christian König]

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

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

[Daniel Vetter]

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.

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Tvrtko Ursulin]

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

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

[Daniel Vetter]

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

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Tvrtko Ursulin]

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

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

[Tvrtko Ursulin]

On 05/01/2023 12:32, Daniel Vetter wrote:
> On Fri, Dec 02, 2022 at 10:01:01AM +0100, Christian König wrote:
>> Am 01.12.22 um 12:09 schrieb Tvrtko Ursulin:
>>>
>>> On 30/11/2022 14:18, Daniel Vetter wrote:
>>>> On Wed, Nov 30, 2022 at 01:34:07PM +0000, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> With the typical model where the display server opends 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.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: "Christian König" <christian.koenig@amd.com>
>>>>> ---
>>>>>    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              | 32
>>>>> ++++++++++++++++++++++++-
>>>>>    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, 65 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> index 30e24da1f398..385deb044058 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>>> @@ -959,6 +959,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;
>>>>>              /*
>>>>> @@ -968,8 +969,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))
>>>>
>>>> This scares me, and also makes me wonder whether we really want to
>>>> conflate the original owner with the rendering owner. And also,
>>>> whether we
>>>> really want to keep updating that, because for some of the "bind an
>>>> fd to
>>>> a pid" use-cases like svm we really do not want to ever again allow a
>>>> change.
>>>>
>>>> So sligthly different idea:
>>>> - we have a separate render pid/drm_file owner frome the open()
>>>> owner that
>>>>     we track in drm_auth.c
>>>> - that one is set the first time a driver specific ioctl is called
>>>> (which
>>>>     for the "pass me the fd" dri3 mode should never be the compositor)
>>>> - we start out with nothing and only set it once, which further
>>>> simplifies
>>>>     the model (still need the mutex for concurrent first ioctl ofc)
>>>
>>> Simpler solution sounds plausible and mostly works for me. Certainly is
>>> attractive to simplify things. And as the disclaimer I put in the cover
>>> letter - I wasn't really sure at all if passing a master fd is a thing
>>> or not. Happy to implement your version if that will be the decision.
>>>
>>> The only downside I can think of right now with having two owners is if
>>> someone is "naughty" and actually uses the fd for rendering from two
>>> sides. That wouldn't conceptually work for what I am doing in the cgroup
>>> controller, where I need to attribute GPU usage to a process, which is a
>>> lookup from struct pid -> list of drm_files -> etc. So in the two owners
>>> scheme I would just need to ignore the "open owner" and rely that
>>> "render ownder" truly is the only one doing the rendering. Or maybe I'd
>>> need to add support for multiple owners as well.. would be a bit
>>> annoying probably.
>>>
>>> Hm now that I think about more.. the one shot nature of this scheme
>>> would have another downside. One could just send the fd back to itself
>>> via a throway forked helper, which only does one ioctl before sending it
>>> back, and then the "render owner" is forever lost. The proposal as I had
>>> it would be immune to this problem at least.
>>>
>>>> Eventually we could then use that to enforce static binding to a pid,
>>>> which is what we want for svm style models, i.e. if the pid changes, the
>>>> fd does an -EACCESS or similar.
>>>>
>>>> Thoughts?
>>>
>>> This use case I am not familiar with at all so can't comment. Only
>>> intuitively I would ask - why is it something that needs to be solved at
>>> the DRM level? Because essentially it sounds like there is a want to
>>> disallow sending fds via sockets.
> 
> Disallowing passing isn't the idea I had. It's to disallow passing to more
> than one recipient, once that recipient has started using the fd for real.

And this restriction would always apply, not just for SVM?

If it would always apply the "two owners / one-shot change" approach 
would work for me. If "pass once" would not be universally applied then 
"two owners / one-shot change" does not work IMO because real user may 
not be correctly reflected.

And I have some reservations on disallow passing implemented in a driver 
framework. Need to snoop around a bit to see if someone else is doing it.

>> I think we should only disallow binding an fd to a different process when
>> there is a reason to do so.
>>
>> For SVM it's an rather obvious security problem when we allow accessing the
>> address space of another process. But this should probably be handled in the
>> SVM code, not here. E.g. we check during CS if the mm_struct is still the
>> same. Which pid or tgid this fd has last or initially used it completely
>> irrelevant to this.
> 
> Yeah my idea was to also then build the SVM check on top of this as
> enforcement.
> 
>> 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.

For now it is for info/debug printing, but the idea is to use it from 
the cgroup controller so usage can be attributed against the real user.

Where do you see the breakage? I left the master perm check as is 
deliberatly to not break that ie. it is not changing ownership ever is 
file->was_master is set.

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

Since file->was_master is AFAICT immutable I don't think I broke anything.

Regards,

Tvrtko

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