| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <04182f67-2c98-add4-be60-539ffe2e9d6a@amd.com>
Date: Thu, 20 Oct 2022 13:33:14 +0200
From: Christian König <christian.koenig@....com>
To: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
Intel-gfx@...ts.freedesktop.org
Cc: cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
Tejun Heo <tj@...nel.org>,
Johannes Weiner <hannes@...xchg.org>,
Zefan Li <lizefan.x@...edance.com>,
Dave Airlie <airlied@...hat.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Rob Clark <robdclark@...omium.org>,
Stéphane Marchesin <marcheu@...omium.org>,
"T . J . Mercier" <tjmercier@...gle.com>, Kenny.Ho@....com,
Brian Welty <brian.welty@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...el.com>
Subject: Re: [RFC 02/17] drm: Track clients per owning process
Am 20.10.22 um 09:34 schrieb Tvrtko Ursulin:
>
> On 20/10/2022 07:40, Christian König wrote:
>> Am 19.10.22 um 19:32 schrieb Tvrtko Ursulin:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@...el.com>
>>>
>>> To enable propagation of settings from the cgroup drm controller to
>>> drm we
>>> need to start tracking which processes own which drm clients.
>>>
>>> Implement that by tracking the struct pid pointer of the owning
>>> process in
>>> a new XArray, pointing to a structure containing a list of associated
>>> struct drm_file pointers.
>>>
>>> Clients are added and removed under the filelist mutex and RCU list
>>> operations are used below it to allow for lockless lookup.
>>
>> That won't work easily like this. The problem is that file_priv->pid
>> is usually not accurate these days:
>>
>> From the debugfs clients file:
>>
>> systemd-logind 773 0 y y 0 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> firefox 2945 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> chrome 35940 128 n n 1000 0
>> chrome 35940 0 n y 1000 1
>> chrome 35940 0 n y 1000 2
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>> Xorg 1639 128 n n 1000 0
>>
>> This is with glxgears and a bunch other OpenGL applications running.
>>
>> The problem is that for most applications the X/Wayland server is now
>> opening the render node. The only exceptions in this case are apps
>> using DRI2 (VA-API?).
>>
>> I always wanted to fix this and actually track who is using the file
>> descriptor instead of who opened it, but never had the time to do this.
>
> There's a patch later in the series which allows client records to be
> migrated to a new PID, and then i915 patch to do that when fd is used
> for context create. That approach I think worked well enough in the
> past. So maybe it could be done in the DRM core at some suitable entry
> point.
Yeah, that makes some sense. I think you should wire that inside
drm_ioctl(), as far as I know more or less all uses of a file descriptor
would go through that function.
And maybe make that a stand alone patch, cause that can go upstream as a
bug fix independently if you ask me.
>> I think you need to fix this problem first. And BTW: and unsigned
>> long doesn't work as PID either with containers.
>
> This I am not familiar with so would like to hear more if you could
> point me in the right direction at least.
Uff, I'm the wrong person to ask stuff like that. I just can say from
experience because I've ran into that trap as well.
>
> My assumption was that struct pid *, which is what I store in unsigned
> long, would be unique in a system where there is a single kernel
> running, so as long as lifetimes are correct (released from tracking
> here when fd is closed, which is implicit on process exit) would work.
> You are suggesting that is not so?
I think you should have the pointer to struct pid directly here since
that is a reference counted structure IIRC. But don't ask me what the
semantics is how to get or put a reference.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@...el.com>
>>> ---
>>> drivers/gpu/drm/Makefile | 1 +
>>> drivers/gpu/drm/drm_cgroup.c | 60
>>> ++++++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/drm_file.c | 18 ++++++++---
>>> include/drm/drm_clients.h | 31 +++++++++++++++++++
>>> include/drm/drm_file.h | 4 +++
>>> 5 files changed, 110 insertions(+), 4 deletions(-)
>>> create mode 100644 drivers/gpu/drm/drm_cgroup.c
>>> create mode 100644 include/drm/drm_clients.h
>>>
>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>> index 6e55c47288e4..0719970d17ee 100644
>>> --- a/drivers/gpu/drm/Makefile
>>> +++ b/drivers/gpu/drm/Makefile
>>> @@ -59,6 +59,7 @@ drm-$(CONFIG_DRM_LEGACY) += \
>>> drm_scatter.o \
>>> drm_vm.o
>>> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
>>> +drm-$(CONFIG_CGROUP_DRM) += drm_cgroup.o
>>> drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>> drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>>> drm-$(CONFIG_OF) += drm_of.o
>>> diff --git a/drivers/gpu/drm/drm_cgroup.c
>>> b/drivers/gpu/drm/drm_cgroup.c
>>> new file mode 100644
>>> index 000000000000..a31ff1d593ab
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/drm_cgroup.c
>>> @@ -0,0 +1,60 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <drm/drm_drv.h>
>>> +#include <drm/drm_clients.h>
>>> +
>>> +static DEFINE_XARRAY(drm_pid_clients);
>>> +
>>> +void drm_clients_close(struct drm_file *file_priv)
>>> +{
>>> + unsigned long pid = (unsigned long)file_priv->pid;
>>> + struct drm_device *dev = file_priv->minor->dev;
>>> + struct drm_pid_clients *clients;
>>> +
>>> + lockdep_assert_held(&dev->filelist_mutex);
>>> +
>>> + clients = xa_load(&drm_pid_clients, pid);
>>> + list_del_rcu(&file_priv->clink);
>>> + if (atomic_dec_and_test(&clients->num)) {
>>> + xa_erase(&drm_pid_clients, pid);
>>> + kfree_rcu(clients, rcu);
>>> + }
>>> +}
>>> +
>>> +int drm_clients_open(struct drm_file *file_priv)
>>> +{
>>> + unsigned long pid = (unsigned long)file_priv->pid;
>>> + struct drm_device *dev = file_priv->minor->dev;
>>> + struct drm_pid_clients *clients;
>>> + bool new_client = false;
>>> +
>>> + lockdep_assert_held(&dev->filelist_mutex);
>>> +
>>> + clients = xa_load(&drm_pid_clients, pid);
>>> + if (!clients) {
>>> + clients = kmalloc(sizeof(*clients), GFP_KERNEL);
>>> + if (!clients)
>>> + return -ENOMEM;
>>> + atomic_set(&clients->num, 0);
>>> + INIT_LIST_HEAD(&clients->file_list);
>>> + init_rcu_head(&clients->rcu);
>>> + new_client = true;
>>> + }
>>> + atomic_inc(&clients->num);
>>> + list_add_tail_rcu(&file_priv->clink, &clients->file_list);
>>> + if (new_client) {
>>> + void *xret;
>>> +
>>> + xret = xa_store(&drm_pid_clients, pid, clients, GFP_KERNEL);
>>> + if (xa_err(xret)) {
>>> + list_del_init(&file_priv->clink);
>>> + kfree(clients);
>>> + return PTR_ERR(clients);
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index a8b4d918e9a3..ce58d5c513db 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -40,6 +40,7 @@
>>> #include <linux/slab.h>
>>> #include <drm/drm_client.h>
>>> +#include <drm/drm_clients.h>
>>> #include <drm/drm_drv.h>
>>> #include <drm/drm_file.h>
>>> #include <drm/drm_print.h>
>>> @@ -298,6 +299,7 @@ static void drm_close_helper(struct file *filp)
>>> mutex_lock(&dev->filelist_mutex);
>>> list_del(&file_priv->lhead);
>>> + drm_clients_close(file_priv);
>>> mutex_unlock(&dev->filelist_mutex);
>>> drm_file_free(file_priv);
>>> @@ -349,10 +351,8 @@ static int drm_open_helper(struct file *filp,
>>> struct drm_minor *minor)
>>> if (drm_is_primary_client(priv)) {
>>> ret = drm_master_open(priv);
>>> - if (ret) {
>>> - drm_file_free(priv);
>>> - return ret;
>>> - }
>>> + if (ret)
>>> + goto err_free;
>>> }
>>> filp->private_data = priv;
>>> @@ -360,6 +360,9 @@ static int drm_open_helper(struct file *filp,
>>> struct drm_minor *minor)
>>> priv->filp = filp;
>>> mutex_lock(&dev->filelist_mutex);
>>> + ret = drm_clients_open(priv);
>>> + if (ret)
>>> + goto err_unlock;
>>> list_add(&priv->lhead, &dev->filelist);
>>> mutex_unlock(&dev->filelist_mutex);
>>> @@ -387,6 +390,13 @@ static int drm_open_helper(struct file *filp,
>>> struct drm_minor *minor)
>>> #endif
>>> return 0;
>>> +
>>> +err_unlock:
>>> + mutex_unlock(&dev->filelist_mutex);
>>> +err_free:
>>> + drm_file_free(priv);
>>> +
>>> + return ret;
>>> }
>>> /**
>>> diff --git a/include/drm/drm_clients.h b/include/drm/drm_clients.h
>>> new file mode 100644
>>> index 000000000000..4ae553a03d1e
>>> --- /dev/null
>>> +++ b/include/drm/drm_clients.h
>>> @@ -0,0 +1,31 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2022 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _DRM_CLIENTS_H_
>>> +#define _DRM_CLIENTS_H_
>>> +
>>> +#include <drm/drm_file.h>
>>> +
>>> +struct drm_pid_clients {
>>> + atomic_t num;
>>> + struct list_head file_list;
>>> + struct rcu_head rcu;
>>> +};
>>> +
>>> +#if IS_ENABLED(CONFIG_CGROUP_DRM)
>>> +void drm_clients_close(struct drm_file *file_priv);
>>> +int drm_clients_open(struct drm_file *file_priv);
>>> +#else
>>> +static inline void drm_clients_close(struct drm_file *file_priv)
>>> +{
>>> +}
>>> +
>>> +static inline int drm_clients_open(struct drm_file *file_priv)
>>> +{
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> +#endif
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index d780fd151789..0965eb111f24 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -268,6 +268,10 @@ struct drm_file {
>>> /** @minor: &struct drm_minor for this file. */
>>> struct drm_minor *minor;
>>> +#if IS_ENABLED(CONFIG_CGROUP_DRM)
>>> + struct list_head clink;
>>> +#endif
>>> +
>>> /**
>>> * @object_idr:
>>> *
>>
Powered by blists - more mailing lists