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
--
Best regards,
Dmitry
