Message-ID: <a3hakatontuh535xkvwz3txbaocjx44ajkarmiigt3prekqysc@hp2zayht4hbh>
Date: Thu, 16 May 2024 23:12:19 +0100
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, kernel@...labora.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Rob Clark <robdclark@...il.com>, 
	Tvrtko Ursulin <tursulin@...ulin.net>
Subject: Re: [PATCH v2 1/1] drm: Add ioctl for querying a DRM device's list
 of open client PIDs

Hi Daniel,

On 02.05.2024 10:09, Daniel Vetter wrote:
> On Wed, May 01, 2024 at 07:50:43PM +0100, Adrián Larumbe wrote:
> > Up to this day, all fdinfo-based GPU profilers must traverse the entire
> > /proc directory structure to find open DRM clients with fdinfo file
> > descriptors. This is inefficient and time-consuming.
> > 
> > This patch adds a new DRM ioctl that allows users to obtain a list of PIDs
> > for clients who have opened the DRM device. Output from the ioctl isn't
> > human-readable, and it's meant to be retrieved only by GPU profilers like
> > gputop and nvtop.
> > 
> > Cc: Rob Clark <robdclark@...il.com>
> > Cc: Tvrtko Ursulin <tursulin@...ulin.net>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@...labora.com>
> > ---
> >  drivers/gpu/drm/drm_internal.h |  1 +
> >  drivers/gpu/drm/drm_ioctl.c    | 89 ++++++++++++++++++++++++++++++++++
> >  include/uapi/drm/drm.h         |  7 +++
> >  3 files changed, 97 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index 690505a1f7a5..6f78954cae16 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -243,6 +243,7 @@ static inline void drm_debugfs_encoder_remove(struct drm_encoder *encoder)
> >  drm_ioctl_t drm_version;
> >  drm_ioctl_t drm_getunique;
> >  drm_ioctl_t drm_getclient;
> > +drm_ioctl_t drm_getclients;
> >  
> >  /* drm_syncobj.c */
> >  void drm_syncobj_open(struct drm_file *file_private);
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index e368fc084c77..da7057376581 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -207,6 +207,93 @@ int drm_getclient(struct drm_device *dev, void *data,
> >  	}
> >  }
> >  
> > +/*
> > + * Get list of client PIDs who have opened a DRM file
> > + *
> > + * \param dev DRM device we are querying
> > + * \param data IOCTL command input.
> > + * \param file_priv DRM file private.
> > + *
> > + * \return zero on success or a negative number on failure.
> > + *
> > + * Traverses list of open clients for the given DRM device, and
> > + * copies them into userpace as an array of PIDs
> > + */
> > +int drm_getclients(struct drm_device *dev, void *data,
> > +		   struct drm_file *file_priv)
> > +
> > +{
> > +	struct drm_get_clients *get_clients = data;
> > +	ssize_t size = get_clients->len;
> > +	char __user *pid_buf;
> > +	ssize_t offset = 0;
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * We do not want to show clients of display only devices so
> > +	 * as to avoid confusing UM GPU profilers
> > +	 */
> > +	if (!dev->render) {
> > +		get_clients->len = 0;
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * An input size of zero means UM wants to know the size of the PID buffer
> > +	 * We round it up to the nearest multiple of the page size so that we can have
> > +	 * some spare headroom in case more clients came in between successive calls
> > +	 * of this ioctl, and also to simplify parsing of the PIDs buffer, because
> > +	 * sizeof(pid_t) will hopefully always divide PAGE_SIZE
> > +	 */
> > +	if (size == 0) {
> > +		get_clients->len =
> > +			roundup(atomic_read(&dev->open_count) * sizeof(pid_t), PAGE_SIZE);
> > +		return 0;
> > +	}
> > +
> > +	pid_buf = (char *)(void *)get_clients->user_data;
> > +
> > +	if (!pid_buf)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&dev->filelist_mutex);
> > +	list_for_each_entry_reverse(file_priv, &dev->filelist, lhead) {
> > +		pid_t pid_num;
> > +
> > +		if ((size - offset) < sizeof(pid_t))
> > +			break;
> > +
> > +		rcu_read_lock();
> > +		pid_num = pid_vnr(rcu_dereference(file_priv->pid));
> > +		rcu_read_unlock();
> > +
> > +		/* We do not want to return the profiler's PID */
> > +		if (pid_vnr(task_tgid(current)) == pid_num)
> > +			continue;
> > +
> > +		ret = copy_to_user(pid_buf + offset, &pid_num, sizeof(pid_t));
> > +		if (ret)
> > +			break;
> > +
> > +		offset += sizeof(pid_t);
> > +	}
> > +	mutex_unlock(&dev->filelist_mutex);
> > +
> > +	if (ret)
> > +		return -EFAULT;
> > +
> > +	if ((size - offset) >= sizeof(pid_t)) {
> > +		pid_t pid_zero = 0;
> > +
> > +		ret = copy_to_user(pid_buf + offset,
> > +				   &pid_zero, sizeof(pid_t));
> > +		if (ret)
> > +			return -EFAULT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Get statistics information.
> >   *
> > @@ -672,6 +759,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> >  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, DRM_MASTER),
> >  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, DRM_MASTER),
> >  	DRM_IOCTL_DEF(DRM_IOCTL_MODE_REVOKE_LEASE, drm_mode_revoke_lease_ioctl, DRM_MASTER),
> > +
> > +	DRM_IOCTL_DEF(DRM_IOCTL_GET_CLIENTS, drm_getclients, DRM_RENDER_ALLOW),
> 
> Uh RENDER_ALLOW sounds like a very bad idea for this, flat out leaks
> information that really paranoid people don't want to have leaked.

I'm curious, how is this ioctl leakier than, let's say, any of the driver
getparam ioctls, like Panfrost's panfrost_ioctl_get_param() ?

I'm asking this because I checked the way access to a drm file is being handled
in drm_ioctl_permit(), and it seems in the case of a render node (which are the
kind we're most interested in retrieving the list of client PIDs for),
DRM_RENDER_ALLOW access is mandatory.

> Natural would be to limit to ptrace ability, but ptrace is for processes
> and fd aren't tied to that. So I'm not sure ...
> 
> I think a separate file (whether in procfs or a special chardev) where you
> can set access rights that suit feels a lot better for this. Plus putting
> it into procfs would also give the natural property that you can only read
> it if you have access to procfs anyway, so imo that feels like the best
> place for this ...
> 
> And yes that means some lkml bikeshedding with procfs folks, but I do
> think that's good here since we're likely not the only ones who need a bit
> faster procfs trawling for some special use-cases. So consistency across
> subsystems would be nice to at least try to aim for.
> -Sima

I think using procfs would make sense if we were displaying process-specific
information, and such is the case of fdinfo directories therein, but this ioctl
(and the device sysfs knob that it replaces from a previous patch series) is
meant to yield all the device's client PIDs. To me that doesn't sound like
something that is tied to a single render client, but to the device as a whole.

> >  };
> >  
> >  #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE(drm_ioctls)
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 16122819edfe..c47aa9de51ab 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -1024,6 +1024,11 @@ struct drm_crtc_queue_sequence {
> >  	__u64 user_data;	/* user data passed to event */
> >  };
> >  
> > +struct drm_get_clients {
> > +	__u64 user_data;
> > +	__kernel_size_t len;
> > +};
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > @@ -1236,6 +1241,8 @@ extern "C" {
> >  #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
> >  #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
> >  
> > +#define DRM_IOCTL_GET_CLIENTS		DRM_IOWR(0xD1, struct drm_get_clients)
> > +
> >  /**
> >   * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata.
> >   *
> > -- 
> > 2.44.0
> > 

Adrian Larumbe

Powered by blists - more mailing lists