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

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.