Skip to contentSkip to conversation

drm: serialize access to debugs_nodes.list

PATCH In review Marcin Slusarz 2 participants 6 messages Oct 30, 2011 – Nov 9, 2011

Subsystems (3)
DRM drivers DRM misc Intel i915
lore
Share & tools
lore

Marcin Slusarz

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
---
 drivers/gpu/drm/drm_debugfs.c |    5 +++++
 include/drm/drmP.h            |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_debugfs.c +5 -0
index 9d2668a..1144fbe 100644
@@ -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);
include/drm/drmP.h +1 -0
index 9b7c2bb..c70c943 100644
@@ -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;
Signature · 1 line
1.7.7
#1 lore raw
Quoted 57 lines · Marcin Slusarz

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 <[email protected]>
---
 drivers/gpu/drm/drm_debugfs.c |    5 +++++
 include/drm/drmP.h            |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)
drivers/gpu/drm/drm_debugfs.c +5 -0
index 9d2668a..1144fbe 100644
@@ -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);
include/drm/drmP.h +0 -0
index 9b7c2bb..c70c943 100644

include/drm/drmP.h struct drm_info_list { · line 976

Quoted 7 lines
include/drm/drmP.h +1 -0
@@ -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

Signature · 3 lines
Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48
author
#2 lore raw
Quoted 7 lines · Daniel Vetter

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.

Yes, you are right. I don't know what the heck I was thinking.

Quoted 1 line

Also, you've forgotten to add the locking to i915/i915_debugfs.c

Yeah. Will fix it too.

Marcin

author
#3 lore raw

PATCH 1/1 drm: serialize access to list of debugfs files

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 <[email protected]>
---
 drivers/gpu/drm/drm_debugfs.c       |   12 +++++++++---
 drivers/gpu/drm/i915/i915_debugfs.c |    5 ++++-
 include/drm/drmP.h                  |    4 +++-
 3 files changed, 16 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_debugfs.c +9 -3
index 9d2668a..d34d4fc 100644
@@ -120,7 +120,10 @@ int drm_debugfs_create_files(struct drm_info_list *files, int count,
 		tmp->minor = minor;
 		tmp->dent = ent;
 		tmp->info_ent = &files[i];
-		list_add(&(tmp->list), &(minor->debugfs_nodes.list));
+
+		mutex_lock(&minor->debugfs_lock);
+		list_add(&tmp->list, &minor->debugfs_list);
+		mutex_unlock(&minor->debugfs_lock);
 	}
 	return 0;
 

@@ -148,7 +151,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	char name[64];
 	int ret;
 
-	INIT_LIST_HEAD(&minor->debugfs_nodes.list);
+	INIT_LIST_HEAD(&minor->debugfs_list);
+	mutex_init(&minor->debugfs_lock);
 	sprintf(name, "%d", minor_id);
 	minor->debugfs_root = debugfs_create_dir(name, root);
 	if (!minor->debugfs_root) {

@@ -194,8 +198,9 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
 	struct drm_info_node *tmp;
 	int i;
 
+	mutex_lock(&minor->debugfs_lock);
 	for (i = 0; i < count; i++) {
-		list_for_each_safe(pos, q, &minor->debugfs_nodes.list) {
+		list_for_each_safe(pos, q, &minor->debugfs_list) {
 			tmp = list_entry(pos, struct drm_info_node, list);
 			if (tmp->info_ent == &files[i]) {
 				debugfs_remove(tmp->dent);

@@ -204,6 +209,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
 			}
 		}
 	}
+	mutex_unlock(&minor->debugfs_lock);
 	return 0;
 }
 EXPORT_SYMBOL(drm_debugfs_remove_files);
drivers/gpu/drm/i915/i915_debugfs.c +4 -1
index 3c395a5..fdad293 100644
@@ -1505,7 +1505,10 @@ drm_add_fake_info_node(struct drm_minor *minor,
 	node->minor = minor;
 	node->dent = ent;
 	node->info_ent = (void *) key;
-	list_add(&node->list, &minor->debugfs_nodes.list);
+
+	mutex_lock(&minor->debugfs_lock);
+	list_add(&node->list, &minor->debugfs_list);
+	mutex_unlock(&minor->debugfs_lock);
 
 	return 0;
 }
include/drm/drmP.h +3 -1
index 9b7c2bb..40e9ea3 100644
@@ -989,7 +989,9 @@ struct drm_minor {
 	struct proc_dir_entry *proc_root;  /**< proc directory entry */
 	struct drm_info_node proc_nodes;
 	struct dentry *debugfs_root;
-	struct drm_info_node debugfs_nodes;
+
+	struct list_head debugfs_list;
+	struct mutex debugfs_lock; /* Protects debugfs_list. */
 
 	struct drm_master *master; /* currently active master for this node */
 	struct list_head master_list;
Signature · 1 line
1.7.7
Reviewed-by on v1 1/1
#4 lore raw

Re: [PATCH] drm: serialize access to list of debugfs files

Quoted 9 lines · Marcin Slusarz

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 <[email protected]>

And v2 also includes a nice comment in the header explaining the locking. Awesome!

Reviewed-by Daniel Vetter <[email protected]>
Signature · 3 lines
Daniel Vetter
Mail: [email protected]
Mobile: +41 (0)79 365 57 48
author
#5 lore raw

PATCH 1/1 drm: serialize access to list of debugfs files

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 <[email protected]>
Reviewed-by Daniel Vetter <[email protected]>

---
Updated changelog.

---
 drivers/gpu/drm/drm_debugfs.c       |   12 +++++++++---
 drivers/gpu/drm/i915/i915_debugfs.c |    5 ++++-
 include/drm/drmP.h                  |    4 +++-
 3 files changed, 16 insertions(+), 5 deletions(-)
drivers/gpu/drm/drm_debugfs.c +9 -3
index d067c12..1c7a1c0 100644
@@ -118,7 +118,10 @@ int drm_debugfs_create_files(struct drm_info_list *files, int count,
 		tmp->minor = minor;
 		tmp->dent = ent;
 		tmp->info_ent = &files[i];
-		list_add(&(tmp->list), &(minor->debugfs_nodes.list));
+
+		mutex_lock(&minor->debugfs_lock);
+		list_add(&tmp->list, &minor->debugfs_list);
+		mutex_unlock(&minor->debugfs_lock);
 	}
 	return 0;
 

@@ -146,7 +149,8 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	char name[64];
 	int ret;
 
-	INIT_LIST_HEAD(&minor->debugfs_nodes.list);
+	INIT_LIST_HEAD(&minor->debugfs_list);
+	mutex_init(&minor->debugfs_lock);
 	sprintf(name, "%d", minor_id);
 	minor->debugfs_root = debugfs_create_dir(name, root);
 	if (!minor->debugfs_root) {

@@ -192,8 +196,9 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
 	struct drm_info_node *tmp;
 	int i;
 
+	mutex_lock(&minor->debugfs_lock);
 	for (i = 0; i < count; i++) {
-		list_for_each_safe(pos, q, &minor->debugfs_nodes.list) {
+		list_for_each_safe(pos, q, &minor->debugfs_list) {
 			tmp = list_entry(pos, struct drm_info_node, list);
 			if (tmp->info_ent == &files[i]) {
 				debugfs_remove(tmp->dent);

@@ -202,6 +207,7 @@ int drm_debugfs_remove_files(struct drm_info_list *files, int count,
 			}
 		}
 	}
+	mutex_unlock(&minor->debugfs_lock);
 	return 0;
 }
 EXPORT_SYMBOL(drm_debugfs_remove_files);
drivers/gpu/drm/i915/i915_debugfs.c +4 -1
index d14b44e..4f40f1c 100644
@@ -1506,7 +1506,10 @@ drm_add_fake_info_node(struct drm_minor *minor,
 	node->minor = minor;
 	node->dent = ent;
 	node->info_ent = (void *) key;
-	list_add(&node->list, &minor->debugfs_nodes.list);
+
+	mutex_lock(&minor->debugfs_lock);
+	list_add(&node->list, &minor->debugfs_list);
+	mutex_unlock(&minor->debugfs_lock);
 
 	return 0;
 }
include/drm/drmP.h +3 -1
index cf39949..1f9e951 100644
@@ -990,7 +990,9 @@ struct drm_minor {
 	struct proc_dir_entry *proc_root;  /**< proc directory entry */
 	struct drm_info_node proc_nodes;
 	struct dentry *debugfs_root;
-	struct drm_info_node debugfs_nodes;
+
+	struct list_head debugfs_list;
+	struct mutex debugfs_lock; /* Protects debugfs_list. */
 
 	struct drm_master *master; /* currently active master for this node */
 	struct list_head master_list;
Signature · 1 line
1.7.7.2