| Message ID | 20240920090920.1342694-2-pierre-eric.pelloux-prayer@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DRM_SET_NAME ioctl | expand |
On 20/09/2024 10:06, 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 > > The string passed by userspace is filtered a bit, to avoid messing > output when it's going to be printed (in dmesg, fdinfo, etc): > * all chars failing isgraph() are replaced by '-' > * if a 0-length string is passed the name is cleared > > 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 | 48 +++++++++++++++++++++++++++++++++++ > include/drm/drm_file.h | 9 +++++++ > include/uapi/drm/drm.h | 17 +++++++++++++ > 5 files changed, 87 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index 6b239a24f1df..482e71160544 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", Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential either way. > "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 ?: ""); > 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..e9dd0e90a1f9 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); > + kfree(file->name); > + > kfree(file); > } > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 51f39912866f..46dadbd1bb35 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -540,6 +540,52 @@ 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 *user_ptr; > + char *new_name; > + size_t i, len; > + > + if (name->name_len > DRM_NAME_MAX_LEN) > + 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); > + > + len = strlen(new_name); > + > + if (len != name->name_len) { > + kfree(new_name); > + return -EINVAL; > + } > + > + /* > + * Filter out control char / spaces / new lines etc in the name > + * since it's going to be used in dmesg or fdinfo's output. > + */ > + for (i = 0; i < len; i++) { > + if (!isgraph(new_name[i])) > + new_name[i] = '-'; > + } > + > + mutex_lock(&file_priv->name_lock); > + kfree(file_priv->name); > + if (len > 0) { > + file_priv->name = new_name; > + } else { > + kfree(new_name); > + file_priv->name = NULL; > + } > + 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 +656,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..f5e92e4f909b 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { > __u64 user_data; /* user data passed to event */ > }; > > +#define DRM_NAME_MAX_LEN 64 > +struct drm_set_name { > + __u64 name_len; > + __u64 name; > +}; > + > + > #if defined(__cplusplus) > } > #endif > @@ -1288,6 +1295,16 @@ 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. > + * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are > + * non-printable or whitespaces will be replaced by -. > + */ > +#define DRM_IOCTL_SET_NAME DRM_IOWR(0xD1, struct drm_set_name) > + A comment, nice! :) Overal looks good to me. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> I do however wish for more opinions (before merging) on whether strings with invalid characters should perhaps instead be rejected. I don't currently have a solid argument either way. Perhaps the only argument against silent transformation is if someone sets some wild string, then greps for it somewhere, which would be a false negative without the understanding of what kind of remapping kernel does. It is weak but it is uapi so worth discussing every crazy possibility I think. On the other hand it would create another annoying source of EINVAL. :shrug: Also, how are with with testing the DRM core features? Add something for the uapi in IGT/tests/drm_client_name, or some such? Regards, Tvrtko > /* > * Device specific ioctls should only be in their respective headers > * The device specific ioctl range is from 0x40 to 0x9f.
On 9/20/24 12:06, 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 > > The string passed by userspace is filtered a bit, to avoid messing > output when it's going to be printed (in dmesg, fdinfo, etc): > * all chars failing isgraph() are replaced by '-' > * if a 0-length string is passed the name is cleared > > 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 | 48 +++++++++++++++++++++++++++++++++++ > include/drm/drm_file.h | 9 +++++++ > include/uapi/drm/drm.h | 17 +++++++++++++ > 5 files changed, 87 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index 6b239a24f1df..482e71160544 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 ?: ""); There should be a default name similar to task->comm, like "<undefined>" when not set. Perhaps also set name to task->comm by default. > 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..e9dd0e90a1f9 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); > + kfree(file->name); > + > kfree(file); > } > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 51f39912866f..46dadbd1bb35 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -540,6 +540,52 @@ 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 *user_ptr; > + char *new_name; > + size_t i, len; > + > + if (name->name_len > DRM_NAME_MAX_LEN) > + 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); > + > + len = strlen(new_name); strnlen > + if (len != name->name_len) { > + kfree(new_name); > + return -EINVAL; > + } > + > + /* > + * Filter out control char / spaces / new lines etc in the name > + * since it's going to be used in dmesg or fdinfo's output. > + */ > + for (i = 0; i < len; i++) { > + if (!isgraph(new_name[i])) > + new_name[i] = '-'; > + } > + > + mutex_lock(&file_priv->name_lock); > + kfree(file_priv->name); > + if (len > 0) { > + file_priv->name = new_name; > + } else { > + kfree(new_name); > + file_priv->name = NULL; > + } > + 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 +656,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; I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1]. That will prevent pointer deref troubles and no additional MM code bloating will be needed. > + /** @name_lock: Protects @name. */ > + struct mutex name_lock; And then this lock isn't strictly needed anymore and can be removed if "name" string is static. > }; > > /** > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 16122819edfe..f5e92e4f909b 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { > __u64 user_data; /* user data passed to event */ > }; > > +#define DRM_NAME_MAX_LEN 64 What about 63, to align data size to 64 bytes including the NULL byte. > +struct drm_set_name { drm_set_name sounds very generic, IMO. Maybe drm_context_set_name? > + __u64 name_len; > + __u64 name; > +}; > + > + > #if defined(__cplusplus) > } > #endif > @@ -1288,6 +1295,16 @@ 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. Don't refer to DMA_BUF_SET_NAME, explain what DRM_IOCTL_SET_NAME actually do. Tell that it sets the DRM context name and that chars are filtered.
On 9/23/24 13:28, Dmitry Osipenko wrote: ... >> /** >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index 16122819edfe..f5e92e4f909b 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { >> __u64 user_data; /* user data passed to event */ >> }; >> >> +#define DRM_NAME_MAX_LEN 64 > > What about 63, to align data size to 64 bytes including the NULL byte. On the other hand, the string is copied without NULL byte, so it doesn't matter. This leads to another question, why not using strndup_user like it's done for dma_buf_set_name?
Hi Dmitry, Le 23/09/2024 à 12:28, Dmitry Osipenko a écrit : > On 9/20/24 12:06, 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 >> >> The string passed by userspace is filtered a bit, to avoid messing >> output when it's going to be printed (in dmesg, fdinfo, etc): >> * all chars failing isgraph() are replaced by '-' >> * if a 0-length string is passed the name is cleared >> >> 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 | 48 +++++++++++++++++++++++++++++++++++ >> include/drm/drm_file.h | 9 +++++++ >> include/uapi/drm/drm.h | 17 +++++++++++++ >> 5 files changed, 87 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c >> index 6b239a24f1df..482e71160544 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 ?: ""); > > There should be a default name similar to task->comm, like "<undefined>" > when not set. Perhaps also set name to task->comm by default. Honestly I don't see much value in printing "<undefined>" or any other default value (+ task->comm is already printed above). > >> 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..e9dd0e90a1f9 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); >> + kfree(file->name); >> + >> kfree(file); >> } >> >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index 51f39912866f..46dadbd1bb35 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -540,6 +540,52 @@ 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 *user_ptr; >> + char *new_name; >> + size_t i, len; >> + >> + if (name->name_len > DRM_NAME_MAX_LEN) >> + 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); >> + >> + len = strlen(new_name); > > strnlen memdup_user_nul returns a NUL-terminated string so I don't see much need for using strnlen. > >> + if (len != name->name_len) { >> + kfree(new_name); >> + return -EINVAL; >> + } >> + >> + /* >> + * Filter out control char / spaces / new lines etc in the name >> + * since it's going to be used in dmesg or fdinfo's output. >> + */ >> + for (i = 0; i < len; i++) { >> + if (!isgraph(new_name[i])) >> + new_name[i] = '-'; >> + } >> + >> + mutex_lock(&file_priv->name_lock); >> + kfree(file_priv->name); >> + if (len > 0) { >> + file_priv->name = new_name; >> + } else { >> + kfree(new_name); >> + file_priv->name = NULL; >> + } >> + 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 +656,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; > > I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1]. > That will prevent pointer deref troubles and no additional MM code > bloating will be needed. > Sure, I can do that if others prefer this way too. >> + /** @name_lock: Protects @name. */ >> + struct mutex name_lock; > > And then this lock isn't strictly needed anymore and can be removed if > "name" string is static. The locking also prevents concurrent modification. > >> }; >> >> /** >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index 16122819edfe..f5e92e4f909b 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { >> __u64 user_data; /* user data passed to event */ >> }; >> >> +#define DRM_NAME_MAX_LEN 64 > > What about 63, to align data size to 64 bytes including the NULL byte. Same as "const char *name;" vs "char name[...]": I don't mind updating the code as long as there's a consensus. > >> +struct drm_set_name { > > drm_set_name sounds very generic, IMO. Maybe drm_context_set_name? drm_client_set_name? (since other places use client, like drm_clients_info()) > >> + __u64 name_len; >> + __u64 name; >> +}; >> + >> + >> #if defined(__cplusplus) >> } >> #endif >> @@ -1288,6 +1295,16 @@ 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. > > Don't refer to DMA_BUF_SET_NAME, explain what DRM_IOCTL_SET_NAME > actually do. Tell that it sets the DRM context name and that chars are > filtered. > OK, I'll update based on your suggestion. Thanks, Pierre-Eric
On 9/23/24 19:29, Pierre-Eric Pelloux-Prayer wrote: ... >>> @@ -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 ?: ""); >> >> There should be a default name similar to task->comm, like "<undefined>" >> when not set. Perhaps also set name to task->comm by default. > > Honestly I don't see much value in printing "<undefined>" or any other > default value (+ task->comm is already printed above). For a machine-parsed string in userspace there should be a value, otherwise it won't be parseable if you'll add another parameter after the name, AFAICT. ... >>> +static int drm_set_name(struct drm_device *dev, void *data, >>> + struct drm_file *file_priv) >>> +{ >>> + struct drm_set_name *name = data; >>> + void __user *user_ptr; >>> + char *new_name; >>> + size_t i, len; >>> + >>> + if (name->name_len > DRM_NAME_MAX_LEN) >>> + 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); >>> + >>> + len = strlen(new_name); >> >> strnlen > > memdup_user_nul returns a NUL-terminated string so I don't see much need > for using strnlen. Indeed ... >>> static int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) >>> { >>> /* ROOT_ONLY is only for CAP_SYS_ADMIN */ >>> @@ -610,6 +656,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; >> >> I'd make the "name" string static, i.e. char name[DRM_NAME_MAX_LEN + 1]. >> That will prevent pointer deref troubles and no additional MM code >> bloating will be needed. >> > > Sure, I can do that if others prefer this way too. Note that in the other email I suggested to use strndup_user(), that will remove the name-length limitation, but then the name var will remain to be a string pointer. To me best option would be to replicate how dma_buf_set_name works. >>> + /** @name_lock: Protects @name. */ >>> + struct mutex name_lock; >> >> And then this lock isn't strictly needed anymore and can be removed if >> "name" string is static. > > The locking also prevents concurrent modification. Right, locking still will be needed >>> }; >>> /** >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>> index 16122819edfe..f5e92e4f909b 100644 >>> --- a/include/uapi/drm/drm.h >>> +++ b/include/uapi/drm/drm.h >>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { >>> __u64 user_data; /* user data passed to event */ >>> }; >>> +#define DRM_NAME_MAX_LEN 64 >> >> What about 63, to align data size to 64 bytes including the NULL byte. > > Same as "const char *name;" vs "char name[...]": I don't mind updating > the code as long as there's a consensus. > >> >>> +struct drm_set_name { >> >> drm_set_name sounds very generic, IMO. Maybe drm_context_set_name? > > drm_client_set_name? > (since other places use client, like drm_clients_info()) drm_client_set_name sounds good
On 9/23/24 21:09, Dmitry Osipenko wrote: >> Sure, I can do that if others prefer this way too. > Note that in the other email I suggested to use strndup_user(), that > will remove the name-length limitation, but then the name var will > remain to be a string pointer. To me best option would be to replicate > how dma_buf_set_name works. My bad, strndup_user() is also size-limited. Then the point about static string remains.
Le 23/09/2024 à 12:06, Tvrtko Ursulin a écrit : > > On 20/09/2024 10:06, 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 >> >> The string passed by userspace is filtered a bit, to avoid messing >> output when it's going to be printed (in dmesg, fdinfo, etc): >> * all chars failing isgraph() are replaced by '-' >> * if a 0-length string is passed the name is cleared >> >> 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 | 48 +++++++++++++++++++++++++++++++++++ >> include/drm/drm_file.h | 9 +++++++ >> include/uapi/drm/drm.h | 17 +++++++++++++ >> 5 files changed, 87 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c >> index 6b239a24f1df..482e71160544 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", > > Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential either way. I'll switch to: seq_printf(m, "%20s %5s %3s master a %5s %10s %*s\n", "command", "tgid", "dev", "uid", "magic", DRM_CLIENT_NAME_MAX_LEN, "name"); And: seq_printf(m, "%20s %5d %3d %c %c %5d %10u %*s\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, DRM_CLIENT_NAME_MAX_LEN, priv->client_name ? priv->client_name : "<unset>"); > >> "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 ?: ""); >> 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..e9dd0e90a1f9 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); >> + kfree(file->name); >> + >> kfree(file); >> } >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index 51f39912866f..46dadbd1bb35 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -540,6 +540,52 @@ 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 *user_ptr; >> + char *new_name; >> + size_t i, len; >> + >> + if (name->name_len > DRM_NAME_MAX_LEN) >> + 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); >> + >> + len = strlen(new_name); >> + >> + if (len != name->name_len) { >> + kfree(new_name); >> + return -EINVAL; >> + } >> + >> + /* >> + * Filter out control char / spaces / new lines etc in the name >> + * since it's going to be used in dmesg or fdinfo's output. >> + */ >> + for (i = 0; i < len; i++) { >> + if (!isgraph(new_name[i])) >> + new_name[i] = '-'; >> + } >> + >> + mutex_lock(&file_priv->name_lock); >> + kfree(file_priv->name); >> + if (len > 0) { >> + file_priv->name = new_name; >> + } else { >> + kfree(new_name); >> + file_priv->name = NULL; >> + } >> + 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 +656,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..f5e92e4f909b 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { >> __u64 user_data; /* user data passed to event */ >> }; >> +#define DRM_NAME_MAX_LEN 64 >> +struct drm_set_name { >> + __u64 name_len; >> + __u64 name; >> +}; >> + >> + >> #if defined(__cplusplus) >> } >> #endif >> @@ -1288,6 +1295,16 @@ 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. >> + * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are >> + * non-printable or whitespaces will be replaced by -. >> + */ >> +#define DRM_IOCTL_SET_NAME DRM_IOWR(0xD1, struct drm_set_name) >> + > > A comment, nice! :) Overal looks good to me. > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > I do however wish for more opinions (before merging) on whether strings with invalid characters > should perhaps instead be rejected. I don't currently have a solid argument either way. > > Perhaps the only argument against silent transformation is if someone sets some wild string, then > greps for it somewhere, which would be a false negative without the understanding of what kind of > remapping kernel does. It is weak but it is uapi so worth discussing every crazy possibility I think. > > On the other hand it would create another annoying source of EINVAL. :shrug: No strong feelings either, but I agree that gathering more opinions before merging would be useful. > > Also, how are with with testing the DRM core features? Add something for the uapi in > IGT/tests/drm_client_name, or some such? Something like using the ioctl and then reading back the name using /sys/kernel/debug/dri/0/clients? I can do that if it's required / useful. Thanks, Pierre-Eric > > Regards, > > Tvrtko > >> /* >> * Device specific ioctls should only be in their respective headers >> * The device specific ioctl range is from 0x40 to 0x9f.
On 24/09/2024 09:22, Pierre-Eric Pelloux-Prayer wrote: > > > Le 23/09/2024 à 12:06, Tvrtko Ursulin a écrit : >> >> On 20/09/2024 10:06, 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 >>> >>> The string passed by userspace is filtered a bit, to avoid messing >>> output when it's going to be printed (in dmesg, fdinfo, etc): >>> * all chars failing isgraph() are replaced by '-' >>> * if a 0-length string is passed the name is cleared >>> >>> 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 | 48 +++++++++++++++++++++++++++++++++++ >>> include/drm/drm_file.h | 9 +++++++ >>> include/uapi/drm/drm.h | 17 +++++++++++++ >>> 5 files changed, 87 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_debugfs.c >>> b/drivers/gpu/drm/drm_debugfs.c >>> index 6b239a24f1df..482e71160544 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", >> >> Allow full DRM_NAME_MAX_LEN? Not sure, feels not very consequential >> either way. > > I'll switch to: > > seq_printf(m, > "%20s %5s %3s master a %5s %10s %*s\n", > "command", > "tgid", > "dev", > "uid", > "magic", > DRM_CLIENT_NAME_MAX_LEN, > "name"); > That works. > And: > > seq_printf(m, "%20s %5d %3d %c %c %5d %10u %*s\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, > DRM_CLIENT_NAME_MAX_LEN, > priv->client_name ? priv->client_name : > "<unset>"); Also works for me although it will look a bit busy by default since every line will contain it. I don't immediately see "parseability" is a concern (what Dmitry raised) because new code can detect if there is something there or not. For old code, or future changes, we do not care in debugfs. Equally we don't care that much if it looks busy, hence why I said "<unset>" works for me. I'd also be okay with repeating task->comm, but that perhaps complicates things too much when task is not available. >> >>> "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 ?: ""); >>> 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..e9dd0e90a1f9 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); >>> + kfree(file->name); >>> + >>> kfree(file); >>> } >>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >>> index 51f39912866f..46dadbd1bb35 100644 >>> --- a/drivers/gpu/drm/drm_ioctl.c >>> +++ b/drivers/gpu/drm/drm_ioctl.c >>> @@ -540,6 +540,52 @@ 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 *user_ptr; >>> + char *new_name; >>> + size_t i, len; >>> + >>> + if (name->name_len > DRM_NAME_MAX_LEN) >>> + 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); >>> + >>> + len = strlen(new_name); >>> + >>> + if (len != name->name_len) { >>> + kfree(new_name); >>> + return -EINVAL; >>> + } >>> + >>> + /* >>> + * Filter out control char / spaces / new lines etc in the name >>> + * since it's going to be used in dmesg or fdinfo's output. >>> + */ >>> + for (i = 0; i < len; i++) { >>> + if (!isgraph(new_name[i])) >>> + new_name[i] = '-'; >>> + } >>> + >>> + mutex_lock(&file_priv->name_lock); >>> + kfree(file_priv->name); >>> + if (len > 0) { >>> + file_priv->name = new_name; >>> + } else { >>> + kfree(new_name); >>> + file_priv->name = NULL; >>> + } >>> + 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 +656,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..f5e92e4f909b 100644 >>> --- a/include/uapi/drm/drm.h >>> +++ b/include/uapi/drm/drm.h >>> @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { >>> __u64 user_data; /* user data passed to event */ >>> }; >>> +#define DRM_NAME_MAX_LEN 64 >>> +struct drm_set_name { >>> + __u64 name_len; >>> + __u64 name; >>> +}; >>> + >>> + >>> #if defined(__cplusplus) >>> } >>> #endif >>> @@ -1288,6 +1295,16 @@ 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. >>> + * The length of the name must <= DRM_NAME_MAX_LEN. All characters >>> that are >>> + * non-printable or whitespaces will be replaced by -. >>> + */ >>> +#define DRM_IOCTL_SET_NAME DRM_IOWR(0xD1, struct drm_set_name) >>> + >> >> A comment, nice! :) Overal looks good to me. >> >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> >> I do however wish for more opinions (before merging) on whether >> strings with invalid characters should perhaps instead be rejected. I >> don't currently have a solid argument either way. >> >> Perhaps the only argument against silent transformation is if someone >> sets some wild string, then greps for it somewhere, which would be a >> false negative without the understanding of what kind of remapping >> kernel does. It is weak but it is uapi so worth discussing every crazy >> possibility I think. >> >> On the other hand it would create another annoying source of EINVAL. >> :shrug: > > No strong feelings either, but I agree that gathering more opinions > before merging would be useful. Right. >> >> Also, how are with with testing the DRM core features? Add something >> for the uapi in IGT/tests/drm_client_name, or some such? > > Something like using the ioctl and then reading back the name using > /sys/kernel/debug/dri/0/clients? > I can do that if it's required / useful. That and making sure various invalid and evil inputs are rejected/handled. Would be nice, but looking at what's currently in IGT I don't see that it is a requirement. Regards, Tvrtko > > Thanks, > Pierre-Eric > >> >> Regards, >> >> Tvrtko >> >>> /* >>> * Device specific ioctls should only be in their respective headers >>> * The device specific ioctl range is from 0x40 to 0x9f.
On 9/23/24 21:18, Dmitry Osipenko wrote: > On 9/23/24 21:09, Dmitry Osipenko wrote: >>> Sure, I can do that if others prefer this way too. >> Note that in the other email I suggested to use strndup_user(), that >> will remove the name-length limitation, but then the name var will >> remain to be a string pointer. To me best option would be to replicate >> how dma_buf_set_name works. > > My bad, strndup_user() is also size-limited. Then the point about static > string remains. To clarify a bit further, I'm fine with both variants. Having a consistent solution across kernel is also good if it's good enough. I.e. replicating the whole dma_buf_set_name using the name pointer is okay to me, though not having string pointers is more robust in general. Choose what you like more.
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 6b239a24f1df..482e71160544 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 ?: ""); 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..e9dd0e90a1f9 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); + kfree(file->name); + kfree(file); } diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 51f39912866f..46dadbd1bb35 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -540,6 +540,52 @@ 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 *user_ptr; + char *new_name; + size_t i, len; + + if (name->name_len > DRM_NAME_MAX_LEN) + 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); + + len = strlen(new_name); + + if (len != name->name_len) { + kfree(new_name); + return -EINVAL; + } + + /* + * Filter out control char / spaces / new lines etc in the name + * since it's going to be used in dmesg or fdinfo's output. + */ + for (i = 0; i < len; i++) { + if (!isgraph(new_name[i])) + new_name[i] = '-'; + } + + mutex_lock(&file_priv->name_lock); + kfree(file_priv->name); + if (len > 0) { + file_priv->name = new_name; + } else { + kfree(new_name); + file_priv->name = NULL; + } + 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 +656,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..f5e92e4f909b 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -1024,6 +1024,13 @@ struct drm_crtc_queue_sequence { __u64 user_data; /* user data passed to event */ }; +#define DRM_NAME_MAX_LEN 64 +struct drm_set_name { + __u64 name_len; + __u64 name; +}; + + #if defined(__cplusplus) } #endif @@ -1288,6 +1295,16 @@ 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. + * The length of the name must <= DRM_NAME_MAX_LEN. All characters that are + * non-printable or whitespaces will be replaced by -. + */ +#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.
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 The string passed by userspace is filtered a bit, to avoid messing output when it's going to be printed (in dmesg, fdinfo, etc): * all chars failing isgraph() are replaced by '-' * if a 0-length string is passed the name is cleared 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 | 48 +++++++++++++++++++++++++++++++++++ include/drm/drm_file.h | 9 +++++++ include/uapi/drm/drm.h | 17 +++++++++++++ 5 files changed, 87 insertions(+), 4 deletions(-)