| Message ID | 1373980455-17119-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 16, 2013 at 03:14:15PM +0200, Daniel Vetter wrote: > We not only have debugfs files to do pretty much the equivalent of > lsof, we also have an ioctl. Not that compared to lsof this dumps a > wee bit more information, but we can still get at that from debugfs > easily. Hmm, why are returning information about other clients to a non-root user? Including the magic required to authorize oneself... This ioctl has to be neutered. -Chris
Hi On Tue, Jul 16, 2013 at 3:14 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > We not only have debugfs files to do pretty much the equivalent of > lsof, we also have an ioctl. Not that compared to lsof this dumps a > wee bit more information, but we can still get at that from debugfs > easily. > > I've dug around in mesa, libdrm and ddx histories and the only users > seem to be drm/tests/dristat.c and drm/tests/getclients.c. The later > is a testcase for the ioctl itself since up to > > commit b018fcdaa5e8b4eabb8cffda687d00004a3c4785 > Author: Eric Anholt <eric@anholt.net> > Date: Thu Nov 22 18:46:54 2007 +1000 > > drm: Make DRM_IOCTL_GET_CLIENT return EINVAL when it can't find client #idx > > there was actually no way at all for userspace to enumerate all > clients since the kernel just wouldn't tell it when to stop. Which > completely broke it's only user, dristat -c. > > So obviously that ioctl wasn't much use for debugging. Hence I don't > see any point in keeping support for a tool which was pretty obviously > never really used, and while we have good replacements in the form of > equivalent debugfs files. > > Still, to keep dristat -c from looping forever again stop it early by > returning an unconditional -EINVAL. Also add a comment in the code > about why. > > v2: Slightly less hollowed-out implementation. libva uses GET_CLIENTS > to figure out whether the fd it has is already authenticated or not. > So we need to keep that part of things working. Simplest way is to > just return one entry to keep va_drm_is_authenticated in > libva/va/drm/va_drm_auth.c working. > > Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com> > Cc: David Herrmann <dh.herrmann@gmail.com> > Cc: Eric Anholt <eric@anholt.net> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_ioctl.c | 43 ++++++++++++++++++++++--------------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 0acf080..ac8ca5c 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -217,29 +217,30 @@ int drm_getclient(struct drm_device *dev, void *data, > struct drm_file *file_priv) > { > struct drm_client *client = data; > - struct drm_file *pt; > - int idx; > - int i; > - > - idx = client->idx; > - i = 0; > > - mutex_lock(&dev->struct_mutex); > - list_for_each_entry(pt, &dev->filelist, lhead) { > - if (i++ >= idx) { > - client->auth = pt->authenticated; > - client->pid = pid_vnr(pt->pid); > - client->uid = from_kuid_munged(current_user_ns(), pt->uid); > - client->magic = pt->magic; > - client->iocs = pt->ioctl_count; > - mutex_unlock(&dev->struct_mutex); > - > - return 0; > - } > + /* > + * Hollowed-out getclient ioctl to keep some dead old drm tests/tools > + * not breaking completely. Userspace tools stop enumerating one they > + * get -EINVAL, hence this is the return value we need to hand back for > + * no clients tracked. > + * > + * Unfortunately some clients (*cough* libva *cough*) use this in a fun > + * attempt to figure out whether they're authenticated or not. Since > + * that's the only thing they care about, give it to the directly > + * instead of walking one giant list. > + */ > + if (client->idx == 0) { > + client->auth = file_priv->authenticated; > + client->pid = pid_vnr(file_priv->pid); > + client->uid = from_kuid_munged(current_user_ns(), > + file_priv->uid); > + client->magic = 0; > + client->iocs = 0; magic = 0 is the only thing that bugs me as "0" is an invalid magic-number for drmAuth. I hope no-one uses it. But we can fix that if we see any regression. I cannot imagine anyone really using it. Reviewed-by: David Herrmann <dh.herrmann@gmail.com> > + > + return 0; > + } else { > + return -EINVAL; > } > - mutex_unlock(&dev->struct_mutex); > - > - return -EINVAL; > } > > /** > -- > 1.8.3.2 >
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 0acf080..ac8ca5c 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -217,29 +217,30 @@ int drm_getclient(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_client *client = data; - struct drm_file *pt; - int idx; - int i; - - idx = client->idx; - i = 0; - mutex_lock(&dev->struct_mutex); - list_for_each_entry(pt, &dev->filelist, lhead) { - if (i++ >= idx) { - client->auth = pt->authenticated; - client->pid = pid_vnr(pt->pid); - client->uid = from_kuid_munged(current_user_ns(), pt->uid); - client->magic = pt->magic; - client->iocs = pt->ioctl_count; - mutex_unlock(&dev->struct_mutex); - - return 0; - } + /* + * Hollowed-out getclient ioctl to keep some dead old drm tests/tools + * not breaking completely. Userspace tools stop enumerating one they + * get -EINVAL, hence this is the return value we need to hand back for + * no clients tracked. + * + * Unfortunately some clients (*cough* libva *cough*) use this in a fun + * attempt to figure out whether they're authenticated or not. Since + * that's the only thing they care about, give it to the directly + * instead of walking one giant list. + */ + if (client->idx == 0) { + client->auth = file_priv->authenticated; + client->pid = pid_vnr(file_priv->pid); + client->uid = from_kuid_munged(current_user_ns(), + file_priv->uid); + client->magic = 0; + client->iocs = 0; + + return 0; + } else { + return -EINVAL; } - mutex_unlock(&dev->struct_mutex); - - return -EINVAL; } /**
We not only have debugfs files to do pretty much the equivalent of lsof, we also have an ioctl. Not that compared to lsof this dumps a wee bit more information, but we can still get at that from debugfs easily. I've dug around in mesa, libdrm and ddx histories and the only users seem to be drm/tests/dristat.c and drm/tests/getclients.c. The later is a testcase for the ioctl itself since up to commit b018fcdaa5e8b4eabb8cffda687d00004a3c4785 Author: Eric Anholt <eric@anholt.net> Date: Thu Nov 22 18:46:54 2007 +1000 drm: Make DRM_IOCTL_GET_CLIENT return EINVAL when it can't find client #idx there was actually no way at all for userspace to enumerate all clients since the kernel just wouldn't tell it when to stop. Which completely broke it's only user, dristat -c. So obviously that ioctl wasn't much use for debugging. Hence I don't see any point in keeping support for a tool which was pretty obviously never really used, and while we have good replacements in the form of equivalent debugfs files. Still, to keep dristat -c from looping forever again stop it early by returning an unconditional -EINVAL. Also add a comment in the code about why. v2: Slightly less hollowed-out implementation. libva uses GET_CLIENTS to figure out whether the fd it has is already authenticated or not. So we need to keep that part of things working. Simplest way is to just return one entry to keep va_drm_is_authenticated in libva/va/drm/va_drm_auth.c working. Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Cc: David Herrmann <dh.herrmann@gmail.com> Cc: Eric Anholt <eric@anholt.net> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_ioctl.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)