[PATCH 1/3] drm: add DRM_SET_NAME ioctl
Christian König
christian.koenig at amd.com
Fri Sep 13 12:21:46 UTC 2024
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 at 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.
More information about the dri-devel
mailing list