Marcin Slusarz
2011-Oct-30 22:04 UTC
[Nouveau] [PATCH] drm: serialize access to debugs_nodes.list
Nouveau, when configured with debugfs, creates debugfs files for every channel, so structure holding list of files needs to be protected from simultaneous changes by multiple threads. Without this patch it's possible to hit kernel oops in drm_debugfs_remove_files just by running a couple of xterms with looped glxinfo. Signed-off-by: Marcin Slusarz <marcin.slusarz at gmail.com> --- drivers/gpu/drm/drm_debugfs.c | 5 +++++ include/drm/drmP.h | 1 + 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index 9d2668a..1144fbe 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -120,7 +120,9 @@ int drm_debugfs_create_files(struct drm_info_list *files, int count, tmp->minor = minor; tmp->dent = ent; tmp->info_ent = &files[i]; + mutex_lock(&minor->debugfs_nodes.mutex); list_add(&(tmp->list), &(minor->debugfs_nodes.list)); + mutex_unlock(&minor->debugfs_nodes.mutex); } return 0; @@ -149,6 +151,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, int ret; INIT_LIST_HEAD(&minor->debugfs_nodes.list); + mutex_init(&minor->debugfs_nodes.mutex); sprintf(name, "%d", minor_id); minor->debugfs_root = debugfs_create_dir(name, root); if (!minor->debugfs_root) { @@ -194,6 +197,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count, struct drm_info_node *tmp; int i; + mutex_lock(&minor->debugfs_nodes.mutex); for (i = 0; i < count; i++) { list_for_each_safe(pos, q, &minor->debugfs_nodes.list) { tmp = list_entry(pos, struct drm_info_node, list); @@ -204,6 +208,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count, } } } + mutex_unlock(&minor->debugfs_nodes.mutex); return 0; } EXPORT_SYMBOL(drm_debugfs_remove_files); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 9b7c2bb..c70c943 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -970,6 +970,7 @@ struct drm_info_list { * debugfs node structure. This structure represents a debugfs file. */ struct drm_info_node { + struct mutex mutex; struct list_head list; struct drm_minor *minor; struct drm_info_list *info_ent; -- 1.7.7
Daniel Vetter
2011-Oct-31 08:06 UTC
[Nouveau] [PATCH] drm: serialize access to debugs_nodes.list
On Sun, Oct 30, 2011 at 11:04:48PM +0100, Marcin Slusarz wrote:> Nouveau, when configured with debugfs, creates debugfs files for every > channel, so structure holding list of files needs to be protected from > simultaneous changes by multiple threads. > > Without this patch it's possible to hit kernel oops in > drm_debugfs_remove_files just by running a couple of xterms with > looped glxinfo. > > Signed-off-by: Marcin Slusarz <marcin.slusarz at gmail.com> > --- > drivers/gpu/drm/drm_debugfs.c | 5 +++++ > include/drm/drmP.h | 1 + > 2 files changed, 6 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c > index 9d2668a..1144fbe 100644 > --- a/drivers/gpu/drm/drm_debugfs.c > +++ b/drivers/gpu/drm/drm_debugfs.c > @@ -120,7 +120,9 @@ int drm_debugfs_create_files(struct drm_info_list *files, int count, > tmp->minor = minor; > tmp->dent = ent; > tmp->info_ent = &files[i]; > + mutex_lock(&minor->debugfs_nodes.mutex); > list_add(&(tmp->list), &(minor->debugfs_nodes.list)); > + mutex_unlock(&minor->debugfs_nodes.mutex); > } > return 0; > > @@ -149,6 +151,7 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id, > int ret; > > INIT_LIST_HEAD(&minor->debugfs_nodes.list); > + mutex_init(&minor->debugfs_nodes.mutex); > sprintf(name, "%d", minor_id); > minor->debugfs_root = debugfs_create_dir(name, root); > if (!minor->debugfs_root) { > @@ -194,6 +197,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count, > struct drm_info_node *tmp; > int i; > > + mutex_lock(&minor->debugfs_nodes.mutex); > for (i = 0; i < count; i++) { > list_for_each_safe(pos, q, &minor->debugfs_nodes.list) { > tmp = list_entry(pos, struct drm_info_node, list); > @@ -204,6 +208,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count, > } > } > } > + mutex_unlock(&minor->debugfs_nodes.mutex); > return 0; > } > EXPORT_SYMBOL(drm_debugfs_remove_files); > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 9b7c2bb..c70c943 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -970,6 +970,7 @@ struct drm_info_list { > * debugfs node structure. This structure represents a debugfs file. > */ > struct drm_info_node { > + struct mutex mutex; > struct list_head list; > struct drm_minor *minor; > struct drm_info_list *info_ent;This is just ugly, you're adding a mutex to every drm_info_node, but only use the one embedded into the minor. On a quick grep we're only ever using the list in there, so I suggest to - replace minor->debugfs_node.list with minor->debugfs_list and kill ->debugfs_node - add the mutex as minor->debugfs_lock That way it's clear what's going on. Also, you've forgotten to add the locking to i915/i915_debugfs.c Yours, Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
Possibly Parallel Threads
- [PATCH v2 1/4] drm/debugfs: Add support for dynamic debugfs initialization
- [PATCH v2 0/4] drm/dp_mst: Add DP MST debugfs nodes for all drivers
- [PATCH v2 1/5] debugfs: add infrastructure to add files with other fops than only read
- [PATCH drm-misc-next v8 02/12] drm: debugfs: provide infrastructure to dump a DRM GPU VA space
- [PATCH v2 0/5] move pstate interface to debugfs