[PATCH 1/3] drm: add DRM_SET_NAME ioctl

[PATCH 1/3] drm: add DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

Giving the opportunity to userspace to associate a free-form
name with a drm_file struct is helpful for tracking and debugging.

This is similar to the existing DMA_BUF_SET_NAME ioctl.

Access to name is protected by a mutex, and the 'clients' debugfs
file has been updated to print it.

Userspace MR to use this ioctl:
   https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 drivers/gpu/drm/drm_debugfs.c | 12 ++++++++----
 drivers/gpu/drm/drm_file.c    |  5 +++++
 drivers/gpu/drm/drm_ioctl.c   | 28 ++++++++++++++++++++++++++++
 include/drm/drm_file.h        |  9 +++++++++
 include/uapi/drm/drm.h        | 14 ++++++++++++++
 5 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index 6b239a24f1df..b7492225ae88 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
 	kuid_t uid;
 
 	seq_printf(m,
-		   "%20s %5s %3s master a %5s %10s\n",
+		   "%20s %5s %3s master a %5s %10s %20s\n",
 		   "command",
 		   "tgid",
 		   "dev",
 		   "uid",
-		   "magic");
+		   "magic",
+		   "name");
 
 	/* dev->filelist is sorted youngest first, but we want to present
 	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
@@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
 		struct task_struct *task;
 		struct pid *pid;
 
+		mutex_lock(&priv->name_lock);
 		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",
+		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
 			   task ? task->comm : "<unknown>",
 			   pid_vnr(pid),
 			   priv->minor->index,
 			   is_current_master ? 'y' : 'n',
 			   priv->authenticated ? 'y' : 'n',
 			   from_kuid_munged(seq_user_ns(m), uid),
-			   priv->magic);
+			   priv->magic,
+			   priv->name ? priv->name : "");
 		rcu_read_unlock();
+		mutex_unlock(&priv->name_lock);
 	}
 	mutex_unlock(&dev->filelist_mutex);
 	return 0;
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 01fde94fe2a9..558151c3912e 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 
 	spin_lock_init(&file->master_lookup_lock);
 	mutex_init(&file->event_read_lock);
+	mutex_init(&file->name_lock);
 
 	if (drm_core_check_feature(dev, DRIVER_GEM))
 		drm_gem_open(dev, file);
@@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
 	WARN_ON(!list_empty(&file->event_list));
 
 	put_pid(rcu_access_pointer(file->pid));
+
+	mutex_destroy(&file->name_lock);
+	kvfree(file->name);
+
 	kfree(file);
 }
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 51f39912866f..ba2f2120e99b 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -540,6 +540,32 @@ int drm_version(struct drm_device *dev, void *data,
 	return err;
 }
 
+static int drm_set_name(struct drm_device *dev, void *data,
+			struct drm_file *file_priv)
+{
+	struct drm_set_name *name = data;
+	void *user_ptr;
+	char *new_name;
+
+	if (name->name_len >= NAME_MAX)
+		return -EINVAL;
+
+	user_ptr = u64_to_user_ptr(name->name);
+
+	new_name = memdup_user_nul(user_ptr, name->name_len);
+
+	if (IS_ERR(new_name))
+		return PTR_ERR(new_name);
+
+	mutex_lock(&file_priv->name_lock);
+	if (file_priv->name)
+		kvfree(file_priv->name);
+	file_priv->name = new_name;
+	mutex_unlock(&file_priv->name_lock);
+
+	return 0;
+}
+
 static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 {
 	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
@@ -610,6 +636,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
 
+	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
+
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 8c0030c77308..df26eee8f79c 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -388,6 +388,15 @@ struct drm_file {
 	 * Per-file buffer caches used by the PRIME buffer sharing code.
 	 */
 	struct drm_prime_file_private prime;
+
+	/**
+	 * @name:
+	 *
+	 * Userspace-provided name; useful for accounting and debugging.
+	 */
+	const char *name;
+	/** @name_lock: Protects @name. */
+	struct mutex name_lock;
 };
 
 /**
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 16122819edfe..fc62bb21f79e 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -1024,6 +1024,12 @@ struct drm_crtc_queue_sequence {
 	__u64 user_data;	/* user data passed to event */
 };
 
+struct drm_set_name {
+	__u64 name_len;
+	__u64 name;
+};
+
+
 #if defined(__cplusplus)
 }
 #endif
@@ -1288,6 +1294,14 @@ extern "C" {
  */
 #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
 
+/**
+ * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
+ *
+ * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
+ * and debugging.
+ */
+#define DRM_IOCTL_SET_NAME		DRM_IOWR(0xD1, struct drm_set_name)
+
 /*
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x9f.
-- 
2.40.1

[PATCH 2/3] drm: use drm_file name in fdinfo

[Pierre-Eric Pelloux-Prayer]

Add an optional drm-client-name field to drm fdinfo's output.

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 Documentation/gpu/drm-usage-stats.rst | 5 +++++
 drivers/gpu/drm/drm_file.c            | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index a80f95ca1b2f..ed1d7edbbc5f 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -73,6 +73,11 @@ scope of each device, in which case `drm-pdev` shall be present as well.
 Userspace should make sure to not double account any usage statistics by using
 the above described criteria in order to associate data to individual clients.
 
+- drm-client-name: <valstr>
+
+String optionally set by userspace using DRM_IOCTL_SET_NAME.
+
+
 Utilization
 ^^^^^^^^^^^
 
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 558151c3912e..024c7b47ab04 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -955,6 +955,11 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
 			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
 	}
 
+	mutex_lock(&file->name_lock);
+	if (file->name)
+		drm_printf(&p, "drm-client-name:\t%s\n", file->name);
+	mutex_unlock(&file->name_lock);
+
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
 }
-- 
2.40.1

[PATCH 3/3] drm/amdgpu: use drm_file name

[Pierre-Eric Pelloux-Prayer]

In debugfs gem_info/vm_info files, timeout handler and page fault reports.

This information is useful with the virtio/native-context driver: this
allows the guest applications identifier to visible in amdgpu's output.

The output in amdgpu_vm_info/amdgpu_gem_info looks like this:
   pid:12255	Process:glxgears/test-set-fd-name ----------

Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       | 11 ++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 20 +++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  4 ++--
 5 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 6d5fd371d5ce..1712feb2c238 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1577,7 +1577,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
 	if (ret)
 		return ret;
 
-	amdgpu_vm_set_task_info(avm);
+	amdgpu_vm_set_task_info(avm, NULL);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 1e475eb01417..d32dc547cc80 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -310,7 +310,7 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
 	kvfree(chunk_array);
 
 	/* Use this opportunity to fill in task info for the vm */
-	amdgpu_vm_set_task_info(vm);
+	amdgpu_vm_set_task_info(vm, p->filp);
 
 	return 0;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 0e617dff8765..0e0d49060ca8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -1012,8 +1012,15 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file *m, void *unused)
 		rcu_read_lock();
 		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>");
+		seq_printf(m, "pid %8d command %s", pid_nr(pid),
+				   task ? task->comm : "<unknown>");
+		if (file->name) {
+			mutex_lock(&file->name_lock);
+			seq_putc(m, '/');
+			seq_puts(m, file->name);
+			mutex_unlock(&file->name_lock);
+		}
+		seq_puts(m, ":\n");
 		rcu_read_unlock();
 
 		spin_lock(&file->table_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index e20d19ae01b2..385211846ae3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2370,7 +2370,7 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
  *
  * @vm: vm for which to set the info
  */
-void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
+void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file)
 {
 	if (!vm->task_info)
 		return;
@@ -2385,7 +2385,23 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
 		return;
 
 	vm->task_info->tgid = current->group_leader->pid;
-	get_task_comm(vm->task_info->process_name, current->group_leader);
+	__get_task_comm(vm->task_info->process_name, TASK_COMM_LEN,
+			current->group_leader);
+	/* Append drm_client_name if set. */
+	if (file && file->name) {
+		int n;
+
+		mutex_lock(&file->name_lock);
+		n = strlen(vm->task_info->process_name);
+		if (n < NAME_MAX) {
+			if (file->name) {
+				vm->task_info->process_name[n] = '/';
+				strscpy_pad(&vm->task_info->process_name[n + 1],
+					    file->name, NAME_MAX - (n + 1));
+			}
+		}
+		mutex_unlock(&file->name_lock);
+	}
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index d12d66dca8e9..cabec384b4d4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -232,7 +232,7 @@ struct amdgpu_vm_pte_funcs {
 };
 
 struct amdgpu_task_info {
-	char		process_name[TASK_COMM_LEN];
+	char		process_name[NAME_MAX];
 	char		task_name[TASK_COMM_LEN];
 	pid_t		pid;
 	pid_t		tgid;
@@ -561,7 +561,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
 			    u32 vmid, u32 node_id, uint64_t addr, uint64_t ts,
 			    bool write_fault);
 
-void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
+void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file);
 
 void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
-- 
2.40.1

Re: [PATCH 1/3] drm: add DRM_SET_NAME ioctl

[Tvrtko Ursulin]

On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
> Giving the opportunity to userspace to associate a free-form
> name with a drm_file struct is helpful for tracking and debugging.
> 
> This is similar to the existing DMA_BUF_SET_NAME ioctl.
> 
> Access to name is protected by a mutex, and the 'clients' debugfs
> file has been updated to print it.
> 
> Userspace MR to use this ioctl:
>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428

Idea seems useful to me. Various classes of comments/questions below:

> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++++----
>   drivers/gpu/drm/drm_file.c    |  5 +++++
>   drivers/gpu/drm/drm_ioctl.c   | 28 ++++++++++++++++++++++++++++
>   include/drm/drm_file.h        |  9 +++++++++
>   include/uapi/drm/drm.h        | 14 ++++++++++++++
>   5 files changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 6b239a24f1df..b7492225ae88 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>   	kuid_t uid;
>   
>   	seq_printf(m,
> -		   "%20s %5s %3s master a %5s %10s\n",
> +		   "%20s %5s %3s master a %5s %10s %20s\n",
>   		   "command",
>   		   "tgid",
>   		   "dev",
>   		   "uid",
> -		   "magic");
> +		   "magic",
> +		   "name");
>   
>   	/* dev->filelist is sorted youngest first, but we want to present
>   	 * oldest first (i.e. kernel, servers, clients), so walk backwardss.
> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>   		struct task_struct *task;
>   		struct pid *pid;
>   
> +		mutex_lock(&priv->name_lock);
>   		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",
> +		seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>   			   task ? task->comm : "<unknown>",
>   			   pid_vnr(pid),
>   			   priv->minor->index,
>   			   is_current_master ? 'y' : 'n',
>   			   priv->authenticated ? 'y' : 'n',
>   			   from_kuid_munged(seq_user_ns(m), uid),
> -			   priv->magic);
> +			   priv->magic,
> +			   priv->name ? priv->name : "");
>   		rcu_read_unlock();
> +		mutex_unlock(&priv->name_lock);

FWIW it is possible you could get away without the need for a lock on 
the read side if you make the pointer RCU managed and stick a 
synchronize_rcu before kfree in the ioctl update path.

Not because this lock would be a contentended one per se, but mostly to 
avoid complications such as amdgpu_debugfs_gem_info_show() where 3/3 has 
it broken - cannot take the mutex in rcu locked section. Just something 
to consider in case it would end up simpler code.

>   	}
>   	mutex_unlock(&dev->filelist_mutex);
>   	return 0;
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 01fde94fe2a9..558151c3912e 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   
>   	spin_lock_init(&file->master_lookup_lock);
>   	mutex_init(&file->event_read_lock);
> +	mutex_init(&file->name_lock);
>   
>   	if (drm_core_check_feature(dev, DRIVER_GEM))
>   		drm_gem_open(dev, file);
> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>   	WARN_ON(!list_empty(&file->event_list));
>   
>   	put_pid(rcu_access_pointer(file->pid));
> +
> +	mutex_destroy(&file->name_lock);
> +	kvfree(file->name);

I think kfree is correct here.

> +
>   	kfree(file);
>   }
>   
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 51f39912866f..ba2f2120e99b 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -540,6 +540,32 @@ int drm_version(struct drm_device *dev, void *data,
>   	return err;
>   }
>   
> +static int drm_set_name(struct drm_device *dev, void *data,
> +			struct drm_file *file_priv)
> +{
> +	struct drm_set_name *name = data;
> +	void *user_ptr;
> +	char *new_name;
> +
> +	if (name->name_len >= NAME_MAX)
> +		return -EINVAL;

Any special reason to use the filesystem NAME_MAX?

> +
> +	user_ptr = u64_to_user_ptr(name->name);
> +
> +	new_name = memdup_user_nul(user_ptr, name->name_len);
> +
> +	if (IS_ERR(new_name))
> +		return PTR_ERR(new_name);
> +
> +	mutex_lock(&file_priv->name_lock);
> +	if (file_priv->name)
> +		kvfree(file_priv->name);
> +	file_priv->name = new_name;
> +	mutex_unlock(&file_priv->name_lock);

One question is whether allowing name changes is interesting or it 
should be one shot?

Second issue is to avoid any Little Bobby Tables situations and somewhat 
validate the input. At least when kernel dumps in in debugfs and fdinfo, 
we probably don't want to allow userspace to be too creative. Like 
output newlines or some other special characters.

Regards,

Tvrtko

> +
> +	return 0;
> +}
> +
>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>   {
>   	/* ROOT_ONLY is only for CAP_SYS_ADMIN */
> @@ -610,6 +636,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>   	DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>   
> +	DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
> +
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>   	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8c0030c77308..df26eee8f79c 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -388,6 +388,15 @@ struct drm_file {
>   	 * Per-file buffer caches used by the PRIME buffer sharing code.
>   	 */
>   	struct drm_prime_file_private prime;
> +
> +	/**
> +	 * @name:
> +	 *
> +	 * Userspace-provided name; useful for accounting and debugging.
> +	 */
> +	const char *name;
> +	/** @name_lock: Protects @name. */
> +	struct mutex name_lock;
>   };
>   
>   /**
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 16122819edfe..fc62bb21f79e 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1024,6 +1024,12 @@ struct drm_crtc_queue_sequence {
>   	__u64 user_data;	/* user data passed to event */
>   };
>   
> +struct drm_set_name {
> +	__u64 name_len;
> +	__u64 name;
> +};
> +
> +
>   #if defined(__cplusplus)
>   }
>   #endif
> @@ -1288,6 +1294,14 @@ extern "C" {
>    */
>   #define DRM_IOCTL_MODE_CLOSEFB		DRM_IOWR(0xD0, struct drm_mode_closefb)
>   
> +/**
> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
> + *
> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
> + * and debugging.
> + */
> +#define DRM_IOCTL_SET_NAME		DRM_IOWR(0xD1, struct drm_set_name)
> +
>   /*
>    * Device specific ioctls should only be in their respective headers
>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH 3/3] drm/amdgpu: use drm_file name

[Tvrtko Ursulin]

On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
> In debugfs gem_info/vm_info files, timeout handler and page fault reports.
> 
> This information is useful with the virtio/native-context driver: this
> allows the guest applications identifier to visible in amdgpu's output.
> 
> The output in amdgpu_vm_info/amdgpu_gem_info looks like this:
>     pid:12255	Process:glxgears/test-set-fd-name ----------
> 
> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       | 11 ++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 20 +++++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  4 ++--
>   5 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 6d5fd371d5ce..1712feb2c238 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1577,7 +1577,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
>   	if (ret)
>   		return ret;
>   
> -	amdgpu_vm_set_task_info(avm);
> +	amdgpu_vm_set_task_info(avm, NULL);
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 1e475eb01417..d32dc547cc80 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -310,7 +310,7 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
>   	kvfree(chunk_array);
>   
>   	/* Use this opportunity to fill in task info for the vm */
> -	amdgpu_vm_set_task_info(vm);
> +	amdgpu_vm_set_task_info(vm, p->filp);
>   
>   	return 0;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 0e617dff8765..0e0d49060ca8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -1012,8 +1012,15 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file *m, void *unused)
>   		rcu_read_lock();
>   		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>");
> +		seq_printf(m, "pid %8d command %s", pid_nr(pid),
> +				   task ? task->comm : "<unknown>");
> +		if (file->name) {
> +			mutex_lock(&file->name_lock);

As mentioned taking a mutex under rcu_read_lock is not allowed. It will 
need to either be re-arranged or, also as mentioned, alternatively 
aligned to use the same RCU access rules.

> +			seq_putc(m, '/');
> +			seq_puts(m, file->name);
> +			mutex_unlock(&file->name_lock);
> +		}
> +		seq_puts(m, ":\n");
>   		rcu_read_unlock();
>   
>   		spin_lock(&file->table_lock);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index e20d19ae01b2..385211846ae3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2370,7 +2370,7 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>    *
>    * @vm: vm for which to set the info
>    */
> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file)
>   {
>   	if (!vm->task_info)
>   		return;
> @@ -2385,7 +2385,23 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>   		return;
>   
>   	vm->task_info->tgid = current->group_leader->pid;
> -	get_task_comm(vm->task_info->process_name, current->group_leader);
> +	__get_task_comm(vm->task_info->process_name, TASK_COMM_LEN,
> +			current->group_leader);
> +	/* Append drm_client_name if set. */
> +	if (file && file->name) {
> +		int n;
> +
> +		mutex_lock(&file->name_lock);
> +		n = strlen(vm->task_info->process_name);
> +		if (n < NAME_MAX) {

NAME_MAX because sizeof(vm->task_info->process_name) is NAME_MAX? (hint)

> +			if (file->name) {

FWIW could check before strlen.

> +				vm->task_info->process_name[n] = '/';

Can this replace the null terminator at process_name[NAME_MAX - 1] with 
a '/'?

> +				strscpy_pad(&vm->task_info->process_name[n + 1],
> +					    file->name, NAME_MAX - (n + 1));
> +			}
> +		}
> +		mutex_unlock(&file->name_lock);
> +	}
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index d12d66dca8e9..cabec384b4d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -232,7 +232,7 @@ struct amdgpu_vm_pte_funcs {
>   };
>   
>   struct amdgpu_task_info {
> -	char		process_name[TASK_COMM_LEN];
> +	char		process_name[NAME_MAX];

Would not fit the longest process name plus the longest drm_file name by 
definition so I suggest size it as TASK_COMM_LEN + 1 + NAME_MAX or so.

Regards,

Tvrtko

>   	char		task_name[TASK_COMM_LEN];
>   	pid_t		pid;
>   	pid_t		tgid;
> @@ -561,7 +561,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>   			    u32 vmid, u32 node_id, uint64_t addr, uint64_t ts,
>   			    bool write_fault);
>   
> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file);
>   
>   void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>   				struct amdgpu_vm *vm);

Re: [PATCH 3/3] drm/amdgpu: use drm_file name

[kernel test robot]

Hi Pierre-Eric,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11-rc7 next-20240912]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pierre-Eric-Pelloux-Prayer/drm-use-drm_file-name-in-fdinfo/20240911-230058
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
patch link:    https://lore.kernel.org/r/20240911145836.734080-3-pierre-eric.pelloux-prayer%40amd.com
patch subject: [PATCH 3/3] drm/amdgpu: use drm_file name
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240913/202409130526.fve4aEMs-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409130526.fve4aEMs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409130526.fve4aEMs-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2374: warning: Function parameter or struct member 'file' not described in 'amdgpu_vm_set_task_info'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2801: warning: Function parameter or struct member 'ts' not described in 'amdgpu_vm_handle_fault'


vim +2374 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c

b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2367  
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2368  /**
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2369   * amdgpu_vm_set_task_info - Sets VMs task info.
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2370   *
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2371   * @vm: vm for which to set the info
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2372   */
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2373  void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file)
b8f67b9ddf4f8f Shashank Sharma            2024-01-18 @2374  {
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2375  	if (!vm->task_info)
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2376  		return;
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2377  
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2378  	if (vm->task_info->pid == current->pid)
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2379  		return;
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2380  
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2381  	vm->task_info->pid = current->pid;
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2382  	get_task_comm(vm->task_info->task_name, current);
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2383  
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2384  	if (current->group_leader->mm != current->mm)
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2385  		return;
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2386  
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2387  	vm->task_info->tgid = current->group_leader->pid;
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2388  	__get_task_comm(vm->task_info->process_name, TASK_COMM_LEN,
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2389  			current->group_leader);
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2390  	/* Append drm_client_name if set. */
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2391  	if (file && file->name) {
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2392  		int n;
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2393  
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2394  		mutex_lock(&file->name_lock);
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2395  		n = strlen(vm->task_info->process_name);
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2396  		if (n < NAME_MAX) {
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2397  			if (file->name) {
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2398  				vm->task_info->process_name[n] = '/';
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2399  				strscpy_pad(&vm->task_info->process_name[n + 1],
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2400  					    file->name, NAME_MAX - (n + 1));
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2401  			}
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2402  		}
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2403  		mutex_unlock(&file->name_lock);
cd1125e8edc565 Pierre-Eric Pelloux-Prayer 2024-09-11  2404  	}
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2405  }
b8f67b9ddf4f8f Shashank Sharma            2024-01-18  2406  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] drm: add DRM_SET_NAME ioctl

[Pierre-Eric Pelloux-Prayer]

Hi Tvrtko,

Le 12/09/2024 à 10:13, Tvrtko Ursulin a écrit :
> 
> On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
>> Giving the opportunity to userspace to associate a free-form
>> name with a drm_file struct is helpful for tracking and debugging.
>>
>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>
>> Access to name is protected by a mutex, and the 'clients' debugfs
>> file has been updated to print it.
>>
>> Userspace MR to use this ioctl:
>>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
> 
> Idea seems useful to me. Various classes of comments/questions below:
> 
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++++----
>>   drivers/gpu/drm/drm_file.c    |  5 +++++
>>   drivers/gpu/drm/drm_ioctl.c   | 28 ++++++++++++++++++++++++++++
>>   include/drm/drm_file.h        |  9 +++++++++
>>   include/uapi/drm/drm.h        | 14 ++++++++++++++
>>   5 files changed, 64 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
>> index 6b239a24f1df..b7492225ae88 100644
>> --- a/drivers/gpu/drm/drm_debugfs.c
>> +++ b/drivers/gpu/drm/drm_debugfs.c
>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>       kuid_t uid;
>>       seq_printf(m,
>> -           "%20s %5s %3s master a %5s %10s\n",
>> +           "%20s %5s %3s master a %5s %10s %20s\n",
>>              "command",
>>              "tgid",
>>              "dev",
>>              "uid",
>> -           "magic");
>> +           "magic",
>> +           "name");
>>       /* dev->filelist is sorted youngest first, but we want to present
>>        * oldest first (i.e. kernel, servers, clients), so walk backwardss.
>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, void *data)
>>           struct task_struct *task;
>>           struct pid *pid;
>> +        mutex_lock(&priv->name_lock);
>>           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",
>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>                  task ? task->comm : "<unknown>",
>>                  pid_vnr(pid),
>>                  priv->minor->index,
>>                  is_current_master ? 'y' : 'n',
>>                  priv->authenticated ? 'y' : 'n',
>>                  from_kuid_munged(seq_user_ns(m), uid),
>> -               priv->magic);
>> +               priv->magic,
>> +               priv->name ? priv->name : "");
>>           rcu_read_unlock();
>> +        mutex_unlock(&priv->name_lock);
> 
> FWIW it is possible you could get away without the need for a lock on the read side if you make the 
> pointer RCU managed and stick a synchronize_rcu before kfree in the ioctl update path.
> 
> Not because this lock would be a contentended one per se, but mostly to avoid complications such as 
> amdgpu_debugfs_gem_info_show() where 3/3 has it broken - cannot take the mutex in rcu locked 
> section. Just something to consider in case it would end up simpler code.

I don't mind using RCU or a mutex. Christian suggested a mutex, so I used that, but I'm happy to 
switch if the RCU approach is preferred.


> 
>>       }
>>       mutex_unlock(&dev->filelist_mutex);
>>       return 0;
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 01fde94fe2a9..558151c3912e 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>>       spin_lock_init(&file->master_lookup_lock);
>>       mutex_init(&file->event_read_lock);
>> +    mutex_init(&file->name_lock);
>>       if (drm_core_check_feature(dev, DRIVER_GEM))
>>           drm_gem_open(dev, file);
>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>       WARN_ON(!list_empty(&file->event_list));
>>       put_pid(rcu_access_pointer(file->pid));
>> +
>> +    mutex_destroy(&file->name_lock);
>> +    kvfree(file->name);
> 
> I think kfree is correct here.
> 

OK, I'll update in v2.

>> +
>>       kfree(file);
>>   }
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 51f39912866f..ba2f2120e99b 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -540,6 +540,32 @@ int drm_version(struct drm_device *dev, void *data,
>>       return err;
>>   }
>> +static int drm_set_name(struct drm_device *dev, void *data,
>> +            struct drm_file *file_priv)
>> +{
>> +    struct drm_set_name *name = data;
>> +    void *user_ptr;
>> +    char *new_name;
>> +
>> +    if (name->name_len >= NAME_MAX)
>> +        return -EINVAL;
> 
> Any special reason to use the filesystem NAME_MAX?

Not really - feel free to suggest something else.

> 
>> +
>> +    user_ptr = u64_to_user_ptr(name->name);
>> +
>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>> +
>> +    if (IS_ERR(new_name))
>> +        return PTR_ERR(new_name);
>> +
>> +    mutex_lock(&file_priv->name_lock);
>> +    if (file_priv->name)
>> +        kvfree(file_priv->name);
>> +    file_priv->name = new_name;
>> +    mutex_unlock(&file_priv->name_lock);
> 
> One question is whether allowing name changes is interesting or it should be one shot?

dma_buf_set_name allows to override the name, so I re-used the same logic.

> 
> Second issue is to avoid any Little Bobby Tables situations and somewhat validate the input. At 
> least when kernel dumps in in debugfs and fdinfo, we probably don't want to allow userspace to be 
> too creative. Like output newlines or some other special characters.

You mean checking "isgraph(c)" for each char? Or even stricter "isalnum(c) || c == '_' || c == '-'"?

Thanks,
Pierre-Eric


> 
> Regards,
> 
> Tvrtko
> 
>> +
>> +    return 0;
>> +}
>> +
>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>   {
>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>> @@ -610,6 +636,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>> +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>> +
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0),
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, DRM_MASTER),
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 8c0030c77308..df26eee8f79c 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -388,6 +388,15 @@ struct drm_file {
>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>        */
>>       struct drm_prime_file_private prime;
>> +
>> +    /**
>> +     * @name:
>> +     *
>> +     * Userspace-provided name; useful for accounting and debugging.
>> +     */
>> +    const char *name;
>> +    /** @name_lock: Protects @name. */
>> +    struct mutex name_lock;
>>   };
>>   /**
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 16122819edfe..fc62bb21f79e 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -1024,6 +1024,12 @@ struct drm_crtc_queue_sequence {
>>       __u64 user_data;    /* user data passed to event */
>>   };
>> +struct drm_set_name {
>> +    __u64 name_len;
>> +    __u64 name;
>> +};
>> +
>> +
>>   #if defined(__cplusplus)
>>   }
>>   #endif
>> @@ -1288,6 +1294,14 @@ extern "C" {
>>    */
>>   #define DRM_IOCTL_MODE_CLOSEFB        DRM_IOWR(0xD0, struct drm_mode_closefb)
>> +/**
>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>> + *
>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier tracking
>> + * and debugging.
>> + */
>> +#define DRM_IOCTL_SET_NAME        DRM_IOWR(0xD1, struct drm_set_name)
>> +
>>   /*
>>    * Device specific ioctls should only be in their respective headers
>>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH 1/3] drm: add DRM_SET_NAME ioctl

[Christian König]

Am 13.09.24 um 14:17 schrieb Pierre-Eric Pelloux-Prayer:
> Hi Tvrtko,
>
> Le 12/09/2024 à 10:13, Tvrtko Ursulin a écrit :
>>
>> On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
>>> Giving the opportunity to userspace to associate a free-form
>>> name with a drm_file struct is helpful for tracking and debugging.
>>>
>>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>>
>>> Access to name is protected by a mutex, and the 'clients' debugfs
>>> file has been updated to print it.
>>>
>>> Userspace MR to use this ioctl:
>>> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428 
>>>
>>
>> Idea seems useful to me. Various classes of comments/questions below:
>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++++----
>>>   drivers/gpu/drm/drm_file.c    |  5 +++++
>>>   drivers/gpu/drm/drm_ioctl.c   | 28 ++++++++++++++++++++++++++++
>>>   include/drm/drm_file.h        |  9 +++++++++
>>>   include/uapi/drm/drm.h        | 14 ++++++++++++++
>>>   5 files changed, 64 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>>> b/drivers/gpu/drm/drm_debugfs.c
>>> index 6b239a24f1df..b7492225ae88 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>       kuid_t uid;
>>>       seq_printf(m,
>>> -           "%20s %5s %3s master a %5s %10s\n",
>>> +           "%20s %5s %3s master a %5s %10s %20s\n",
>>>              "command",
>>>              "tgid",
>>>              "dev",
>>>              "uid",
>>> -           "magic");
>>> +           "magic",
>>> +           "name");
>>>       /* dev->filelist is sorted youngest first, but we want to present
>>>        * oldest first (i.e. kernel, servers, clients), so walk 
>>> backwardss.
>>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>           struct task_struct *task;
>>>           struct pid *pid;
>>> +        mutex_lock(&priv->name_lock);
>>>           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",
>>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>>                  task ? task->comm : "<unknown>",
>>>                  pid_vnr(pid),
>>>                  priv->minor->index,
>>>                  is_current_master ? 'y' : 'n',
>>>                  priv->authenticated ? 'y' : 'n',
>>>                  from_kuid_munged(seq_user_ns(m), uid),
>>> -               priv->magic);
>>> +               priv->magic,
>>> +               priv->name ? priv->name : "");
>>>           rcu_read_unlock();
>>> +        mutex_unlock(&priv->name_lock);
>>
>> FWIW it is possible you could get away without the need for a lock on 
>> the read side if you make the pointer RCU managed and stick a 
>> synchronize_rcu before kfree in the ioctl update path.
>>
>> Not because this lock would be a contentended one per se, but mostly 
>> to avoid complications such as amdgpu_debugfs_gem_info_show() where 
>> 3/3 has it broken - cannot take the mutex in rcu locked section. Just 
>> something to consider in case it would end up simpler code.
>
> I don't mind using RCU or a mutex. Christian suggested a mutex, so I 
> used that, but I'm happy to switch if the RCU approach is preferred.

RCU on a pure string is easier said than done. So I strongly suggest to 
stick with a mutex for now.

It's not that contention is performance critical here.

Regards,
Christian.

>
>
>>
>>>       }
>>>       mutex_unlock(&dev->filelist_mutex);
>>>       return 0;
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 01fde94fe2a9..558151c3912e 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>>> *minor)
>>>       spin_lock_init(&file->master_lookup_lock);
>>>       mutex_init(&file->event_read_lock);
>>> +    mutex_init(&file->name_lock);
>>>       if (drm_core_check_feature(dev, DRIVER_GEM))
>>>           drm_gem_open(dev, file);
>>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>>       WARN_ON(!list_empty(&file->event_list));
>>>       put_pid(rcu_access_pointer(file->pid));
>>> +
>>> +    mutex_destroy(&file->name_lock);
>>> +    kvfree(file->name);
>>
>> I think kfree is correct here.
>>
>
> OK, I'll update in v2.
>
>>> +
>>>       kfree(file);
>>>   }
>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>> index 51f39912866f..ba2f2120e99b 100644
>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>> @@ -540,6 +540,32 @@ int drm_version(struct drm_device *dev, void 
>>> *data,
>>>       return err;
>>>   }
>>> +static int drm_set_name(struct drm_device *dev, void *data,
>>> +            struct drm_file *file_priv)
>>> +{
>>> +    struct drm_set_name *name = data;
>>> +    void *user_ptr;
>>> +    char *new_name;
>>> +
>>> +    if (name->name_len >= NAME_MAX)
>>> +        return -EINVAL;
>>
>> Any special reason to use the filesystem NAME_MAX?
>
> Not really - feel free to suggest something else.
>
>>
>>> +
>>> +    user_ptr = u64_to_user_ptr(name->name);
>>> +
>>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>>> +
>>> +    if (IS_ERR(new_name))
>>> +        return PTR_ERR(new_name);
>>> +
>>> +    mutex_lock(&file_priv->name_lock);
>>> +    if (file_priv->name)
>>> +        kvfree(file_priv->name);
>>> +    file_priv->name = new_name;
>>> +    mutex_unlock(&file_priv->name_lock);
>>
>> One question is whether allowing name changes is interesting or it 
>> should be one shot?
>
> dma_buf_set_name allows to override the name, so I re-used the same 
> logic.
>
>>
>> Second issue is to avoid any Little Bobby Tables situations and 
>> somewhat validate the input. At least when kernel dumps in in debugfs 
>> and fdinfo, we probably don't want to allow userspace to be too 
>> creative. Like output newlines or some other special characters.
>
> You mean checking "isgraph(c)" for each char? Or even stricter 
> "isalnum(c) || c == '_' || c == '-'"?
>
> Thanks,
> Pierre-Eric
>
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>>   {
>>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>>> @@ -610,6 +636,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
>>> drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
>>> drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>>> +
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, 
>>> drm_mode_getplane_res, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, 
>>> DRM_MASTER),
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 8c0030c77308..df26eee8f79c 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -388,6 +388,15 @@ struct drm_file {
>>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>>        */
>>>       struct drm_prime_file_private prime;
>>> +
>>> +    /**
>>> +     * @name:
>>> +     *
>>> +     * Userspace-provided name; useful for accounting and debugging.
>>> +     */
>>> +    const char *name;
>>> +    /** @name_lock: Protects @name. */
>>> +    struct mutex name_lock;
>>>   };
>>>   /**
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 16122819edfe..fc62bb21f79e 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -1024,6 +1024,12 @@ struct drm_crtc_queue_sequence {
>>>       __u64 user_data;    /* user data passed to event */
>>>   };
>>> +struct drm_set_name {
>>> +    __u64 name_len;
>>> +    __u64 name;
>>> +};
>>> +
>>> +
>>>   #if defined(__cplusplus)
>>>   }
>>>   #endif
>>> @@ -1288,6 +1294,14 @@ extern "C" {
>>>    */
>>>   #define DRM_IOCTL_MODE_CLOSEFB        DRM_IOWR(0xD0, struct 
>>> drm_mode_closefb)
>>> +/**
>>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>>> + *
>>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier 
>>> tracking
>>> + * and debugging.
>>> + */
>>> +#define DRM_IOCTL_SET_NAME        DRM_IOWR(0xD1, struct drm_set_name)
>>> +
>>>   /*
>>>    * Device specific ioctls should only be in their respective headers
>>>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH 3/3] drm/amdgpu: use drm_file name

[Pierre-Eric Pelloux-Prayer]

Hi,

Le 12/09/2024 à 10:24, Tvrtko Ursulin a écrit :
> 
> On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
>> In debugfs gem_info/vm_info files, timeout handler and page fault reports.
>>
>> This information is useful with the virtio/native-context driver: this
>> allows the guest applications identifier to visible in amdgpu's output.
>>
>> The output in amdgpu_vm_info/amdgpu_gem_info looks like this:
>>     pid:12255    Process:glxgears/test-set-fd-name ----------
>>
>> Signed-off-by: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c        |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c       | 11 ++++++++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c        | 20 +++++++++++++++++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h        |  4 ++--
>>   5 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 6d5fd371d5ce..1712feb2c238 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -1577,7 +1577,7 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct amdgpu_device *adev,
>>       if (ret)
>>           return ret;
>> -    amdgpu_vm_set_task_info(avm);
>> +    amdgpu_vm_set_task_info(avm, NULL);
>>       return 0;
>>   }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 1e475eb01417..d32dc547cc80 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -310,7 +310,7 @@ static int amdgpu_cs_pass1(struct amdgpu_cs_parser *p,
>>       kvfree(chunk_array);
>>       /* Use this opportunity to fill in task info for the vm */
>> -    amdgpu_vm_set_task_info(vm);
>> +    amdgpu_vm_set_task_info(vm, p->filp);
>>       return 0;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 0e617dff8765..0e0d49060ca8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -1012,8 +1012,15 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file *m, void *unused)
>>           rcu_read_lock();
>>           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>");
>> +        seq_printf(m, "pid %8d command %s", pid_nr(pid),
>> +                   task ? task->comm : "<unknown>");
>> +        if (file->name) {
>> +            mutex_lock(&file->name_lock);
> 
> As mentioned taking a mutex under rcu_read_lock is not allowed. It will need to either be 
> re-arranged or, also as mentioned, alternatively aligned to use the same RCU access rules.

I intended to fix this before sending the patch. It's now fixed locally (lock taken once, outside of 
the loop, like is done for dev->filelist_mutex).

> 
>> +            seq_putc(m, '/');
>> +            seq_puts(m, file->name);
>> +            mutex_unlock(&file->name_lock);
>> +        }
>> +        seq_puts(m, ":\n");
>>           rcu_read_unlock();
>>           spin_lock(&file->table_lock);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index e20d19ae01b2..385211846ae3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2370,7 +2370,7 @@ static int amdgpu_vm_create_task_info(struct amdgpu_vm *vm)
>>    *
>>    * @vm: vm for which to set the info
>>    */
>> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file)
>>   {
>>       if (!vm->task_info)
>>           return;
>> @@ -2385,7 +2385,23 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm)
>>           return;
>>       vm->task_info->tgid = current->group_leader->pid;
>> -    get_task_comm(vm->task_info->process_name, current->group_leader);
>> +    __get_task_comm(vm->task_info->process_name, TASK_COMM_LEN,
>> +            current->group_leader);
>> +    /* Append drm_client_name if set. */
>> +    if (file && file->name) {
>> +        int n;
>> +
>> +        mutex_lock(&file->name_lock);
>> +        n = strlen(vm->task_info->process_name);
>> +        if (n < NAME_MAX) {
> 
> NAME_MAX because sizeof(vm->task_info->process_name) is NAME_MAX? (hint)

I've reworked this patch to make it clear the string formatting is correct.
Before sending it again for review, I'll wait for Christian/Alex's feedback.


> 
>> +            if (file->name) {
> 
> FWIW could check before strlen.
> 
>> +                vm->task_info->process_name[n] = '/';
> 
> Can this replace the null terminator at process_name[NAME_MAX - 1] with a '/'?
> 
>> +                strscpy_pad(&vm->task_info->process_name[n + 1],
>> +                        file->name, NAME_MAX - (n + 1));
>> +            }
>> +        }
>> +        mutex_unlock(&file->name_lock);
>> +    }
>>   }
>>   /**
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index d12d66dca8e9..cabec384b4d4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -232,7 +232,7 @@ struct amdgpu_vm_pte_funcs {
>>   };
>>   struct amdgpu_task_info {
>> -    char        process_name[TASK_COMM_LEN];
>> +    char        process_name[NAME_MAX];
> 
> Would not fit the longest process name plus the longest drm_file name by definition so I suggest 
> size it as TASK_COMM_LEN + 1 + NAME_MAX or so.

IMO the current version is ok as it only truncates userspace strings longer than 239 chars.


Thanks,
Pierre-Eric

> 
> Regards,
> 
> Tvrtko
> 
>>       char        task_name[TASK_COMM_LEN];
>>       pid_t        pid;
>>       pid_t        tgid;
>> @@ -561,7 +561,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, u32 pasid,
>>                   u32 vmid, u32 node_id, uint64_t addr, uint64_t ts,
>>                   bool write_fault);
>> -void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
>> +void amdgpu_vm_set_task_info(struct amdgpu_vm *vm, struct drm_file *file);
>>   void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>>                   struct amdgpu_vm *vm);

Re: [PATCH 1/3] drm: add DRM_SET_NAME ioctl

[Tvrtko Ursulin]

On 13/09/2024 13:17, Pierre-Eric Pelloux-Prayer wrote:
> Hi Tvrtko,
> 
> Le 12/09/2024 à 10:13, Tvrtko Ursulin a écrit :
>>
>> On 11/09/2024 15:58, Pierre-Eric Pelloux-Prayer wrote:
>>> Giving the opportunity to userspace to associate a free-form
>>> name with a drm_file struct is helpful for tracking and debugging.
>>>
>>> This is similar to the existing DMA_BUF_SET_NAME ioctl.
>>>
>>> Access to name is protected by a mutex, and the 'clients' debugfs
>>> file has been updated to print it.
>>>
>>> Userspace MR to use this ioctl:
>>>     
>>> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1428
>>
>> Idea seems useful to me. Various classes of comments/questions below:
>>
>>> Signed-off-by: Pierre-Eric Pelloux-Prayer 
>>> <pierre-eric.pelloux-prayer@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_debugfs.c | 12 ++++++++----
>>>   drivers/gpu/drm/drm_file.c    |  5 +++++
>>>   drivers/gpu/drm/drm_ioctl.c   | 28 ++++++++++++++++++++++++++++
>>>   include/drm/drm_file.h        |  9 +++++++++
>>>   include/uapi/drm/drm.h        | 14 ++++++++++++++
>>>   5 files changed, 64 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_debugfs.c 
>>> b/drivers/gpu/drm/drm_debugfs.c
>>> index 6b239a24f1df..b7492225ae88 100644
>>> --- a/drivers/gpu/drm/drm_debugfs.c
>>> +++ b/drivers/gpu/drm/drm_debugfs.c
>>> @@ -78,12 +78,13 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>       kuid_t uid;
>>>       seq_printf(m,
>>> -           "%20s %5s %3s master a %5s %10s\n",
>>> +           "%20s %5s %3s master a %5s %10s %20s\n",
>>>              "command",
>>>              "tgid",
>>>              "dev",
>>>              "uid",
>>> -           "magic");
>>> +           "magic",
>>> +           "name");
>>>       /* dev->filelist is sorted youngest first, but we want to present
>>>        * oldest first (i.e. kernel, servers, clients), so walk 
>>> backwardss.
>>> @@ -94,19 +95,22 @@ static int drm_clients_info(struct seq_file *m, 
>>> void *data)
>>>           struct task_struct *task;
>>>           struct pid *pid;
>>> +        mutex_lock(&priv->name_lock);
>>>           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",
>>> +        seq_printf(m, "%20s %5d %3d   %c    %c %5d %10u %20s\n",
>>>                  task ? task->comm : "<unknown>",
>>>                  pid_vnr(pid),
>>>                  priv->minor->index,
>>>                  is_current_master ? 'y' : 'n',
>>>                  priv->authenticated ? 'y' : 'n',
>>>                  from_kuid_munged(seq_user_ns(m), uid),
>>> -               priv->magic);
>>> +               priv->magic,
>>> +               priv->name ? priv->name : "");
>>>           rcu_read_unlock();
>>> +        mutex_unlock(&priv->name_lock);
>>
>> FWIW it is possible you could get away without the need for a lock on 
>> the read side if you make the pointer RCU managed and stick a 
>> synchronize_rcu before kfree in the ioctl update path.
>>
>> Not because this lock would be a contentended one per se, but mostly 
>> to avoid complications such as amdgpu_debugfs_gem_info_show() where 
>> 3/3 has it broken - cannot take the mutex in rcu locked section. Just 
>> something to consider in case it would end up simpler code.
> 
> I don't mind using RCU or a mutex. Christian suggested a mutex, so I 
> used that, but I'm happy to switch if the RCU approach is preferred.

Mutex is fine as I said. Just mentioning RCU since it feels trivial and 
avoids the complications in amdgpu_debugfs_gem_info_show().

>>>       mutex_unlock(&dev->filelist_mutex);
>>>       return 0;
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 01fde94fe2a9..558151c3912e 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -158,6 +158,7 @@ struct drm_file *drm_file_alloc(struct drm_minor 
>>> *minor)
>>>       spin_lock_init(&file->master_lookup_lock);
>>>       mutex_init(&file->event_read_lock);
>>> +    mutex_init(&file->name_lock);
>>>       if (drm_core_check_feature(dev, DRIVER_GEM))
>>>           drm_gem_open(dev, file);
>>> @@ -259,6 +260,10 @@ void drm_file_free(struct drm_file *file)
>>>       WARN_ON(!list_empty(&file->event_list));
>>>       put_pid(rcu_access_pointer(file->pid));
>>> +
>>> +    mutex_destroy(&file->name_lock);
>>> +    kvfree(file->name);
>>
>> I think kfree is correct here.
>>
> 
> OK, I'll update in v2.
> 
>>> +
>>>       kfree(file);
>>>   }
>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>>> index 51f39912866f..ba2f2120e99b 100644
>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>> @@ -540,6 +540,32 @@ int drm_version(struct drm_device *dev, void *data,
>>>       return err;
>>>   }
>>> +static int drm_set_name(struct drm_device *dev, void *data,
>>> +            struct drm_file *file_priv)
>>> +{
>>> +    struct drm_set_name *name = data;
>>> +    void *user_ptr;
>>> +    char *new_name;
>>> +
>>> +    if (name->name_len >= NAME_MAX)
>>> +        return -EINVAL;
>>
>> Any special reason to use the filesystem NAME_MAX?
> 
> Not really - feel free to suggest something else.

I was just curios because dma-buf uses own much shorter limit. You could 
always follow the same approach so someone else does not have to wonder 
what is the connection with NAME_MAX. :) I would also think 255 is way 
too generous but meh.

>>> +
>>> +    user_ptr = u64_to_user_ptr(name->name);
>>> +
>>> +    new_name = memdup_user_nul(user_ptr, name->name_len);
>>> +
>>> +    if (IS_ERR(new_name))
>>> +        return PTR_ERR(new_name);
>>> +
>>> +    mutex_lock(&file_priv->name_lock);
>>> +    if (file_priv->name)
>>> +        kvfree(file_priv->name);
>>> +    file_priv->name = new_name;
>>> +    mutex_unlock(&file_priv->name_lock);
>>
>> One question is whether allowing name changes is interesting or it 
>> should be one shot?
> 
> dma_buf_set_name allows to override the name, so I re-used the same logic.

No complaints per se, again just curiousity. But it could be worth to 
think whether renames are required and if not simplify the code by not 
allowing them. May remove the need for any locking on the read side.

>> Second issue is to avoid any Little Bobby Tables situations and 
>> somewhat validate the input. At least when kernel dumps in in debugfs 
>> and fdinfo, we probably don't want to allow userspace to be too 
>> creative. Like output newlines or some other special characters.
> 
> You mean checking "isgraph(c)" for each char? Or even stricter 
> "isalnum(c) || c == '_' || c == '-'"?

Hard to say apart that newlines definitely feel like a no go. As said, 
we don't want someone to do:

drm_set_name(f, "little\ndrm-client-name: bobby\ndrm-client-name: tables")

:))

Or even worse, finding creative ways to hide yourself from gputop and 
such be exploiting some userspace bugs.

Hmm.. isgraph sounds potentially good enough but hopefully someone else 
provides an opinion here. Part of me thinks we should be stricter but I 
can't really justify it on the spot. But it is the first userspace 
provided free form string in drm-usage-stats.rst so it deserves some 
care to define the rules.

Regards,

Tvrtko

>> Tvrtko
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
>>>   {
>>>       /* ROOT_ONLY is only for CAP_SYS_ADMIN */
>>> @@ -610,6 +636,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, 
>>> drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, 
>>> drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SET_NAME, drm_set_name, DRM_RENDER_ALLOW),
>>> +
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, 
>>> drm_mode_getplane_res, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
>>>       DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETCRTC, drm_mode_setcrtc, 
>>> DRM_MASTER),
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 8c0030c77308..df26eee8f79c 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -388,6 +388,15 @@ struct drm_file {
>>>        * Per-file buffer caches used by the PRIME buffer sharing code.
>>>        */
>>>       struct drm_prime_file_private prime;
>>> +
>>> +    /**
>>> +     * @name:
>>> +     *
>>> +     * Userspace-provided name; useful for accounting and debugging.
>>> +     */
>>> +    const char *name;
>>> +    /** @name_lock: Protects @name. */
>>> +    struct mutex name_lock;
>>>   };
>>>   /**
>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>> index 16122819edfe..fc62bb21f79e 100644
>>> --- a/include/uapi/drm/drm.h
>>> +++ b/include/uapi/drm/drm.h
>>> @@ -1024,6 +1024,12 @@ struct drm_crtc_queue_sequence {
>>>       __u64 user_data;    /* user data passed to event */
>>>   };
>>> +struct drm_set_name {
>>> +    __u64 name_len;
>>> +    __u64 name;
>>> +};
>>> +
>>> +
>>>   #if defined(__cplusplus)
>>>   }
>>>   #endif
>>> @@ -1288,6 +1294,14 @@ extern "C" {
>>>    */
>>>   #define DRM_IOCTL_MODE_CLOSEFB        DRM_IOWR(0xD0, struct 
>>> drm_mode_closefb)
>>> +/**
>>> + * DRM_IOCTL_SET_NAME - Attach a name to a drm_file
>>> + *
>>> + * This ioctl is similar to DMA_BUF_SET_NAME - it allows for easier 
>>> tracking
>>> + * and debugging.
>>> + */
>>> +#define DRM_IOCTL_SET_NAME        DRM_IOWR(0xD1, struct drm_set_name)
>>> +
>>>   /*
>>>    * Device specific ioctls should only be in their respective headers
>>>    * The device specific ioctl range is from 0x40 to 0x9f.

Re: [PATCH 1/3] drm: add DRM_SET_NAME ioctl

[kernel test robot]

Hi Pierre-Eric,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11-rc7 next-20240913]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Pierre-Eric-Pelloux-Prayer/drm-use-drm_file-name-in-fdinfo/20240911-230058
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
patch link:    https://lore.kernel.org/r/20240911145836.734080-1-pierre-eric.pelloux-prayer%40amd.com
patch subject: [PATCH 1/3] drm: add DRM_SET_NAME ioctl
config: x86_64-randconfig-121-20240913 (https://download.01.org/0day-ci/archive/20240914/202409140642.ZDKf0cja-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140642.ZDKf0cja-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409140642.ZDKf0cja-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/drm_ioctl.c:553:18: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *user_ptr @@     got void [noderef] __user * @@
   drivers/gpu/drm/drm_ioctl.c:553:18: sparse:     expected void *user_ptr
   drivers/gpu/drm/drm_ioctl.c:553:18: sparse:     got void [noderef] __user *
>> drivers/gpu/drm/drm_ioctl.c:555:36: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void const [noderef] __user * @@     got void *user_ptr @@
   drivers/gpu/drm/drm_ioctl.c:555:36: sparse:     expected void const [noderef] __user *
   drivers/gpu/drm/drm_ioctl.c:555:36: sparse:     got void *user_ptr

vim +553 drivers/gpu/drm/drm_ioctl.c

   542	
   543	static int drm_set_name(struct drm_device *dev, void *data,
   544				struct drm_file *file_priv)
   545	{
   546		struct drm_set_name *name = data;
   547		void *user_ptr;
   548		char *new_name;
   549	
   550		if (name->name_len >= NAME_MAX)
   551			return -EINVAL;
   552	
 > 553		user_ptr = u64_to_user_ptr(name->name);
   554	
 > 555		new_name = memdup_user_nul(user_ptr, name->name_len);
   556	
   557		if (IS_ERR(new_name))
   558			return PTR_ERR(new_name);
   559	
   560		mutex_lock(&file_priv->name_lock);
   561		if (file_priv->name)
   562			kvfree(file_priv->name);
   563		file_priv->name = new_name;
   564		mutex_unlock(&file_priv->name_lock);
   565	
   566		return 0;
   567	}
   568	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki